Moodle
  1. Moodle
  2. MDL-39702

behat silently failing in some circumstance

    Details

    • Testing Instructions:
      Hide
      1. Store a new php script in dirroot with the following contents
        <?php
        
        ini_set('display_errors', true);
        ini_set('error_reporting', E_ALL);
        
        require_once(__DIR__ . '/lib/testing/generator/data_generator.php');
        $instance = new testing_data_generator();
        
        echo "Script end\n";
        
      2. Run the script through CLI
      3. The script SHOULD end and you SHOULD NOT see any output

      Repeat the test changing file and the class name for repository_generator.php and testing_repository_generator

      Show
      Store a new php script in dirroot with the following contents <?php ini_set('display_errors', true ); ini_set('error_reporting', E_ALL); require_once(__DIR__ . '/lib/testing/generator/data_generator.php'); $instance = new testing_data_generator(); echo "Script end\n" ; Run the script through CLI The script SHOULD end and you SHOULD NOT see any output Repeat the test changing file and the class name for repository_generator.php and testing_repository_generator
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-39702_master
    • Rank:
      50435

      Description

      Turns out my behat problems were caused by some code of Freds breaking behat. But the weird thing is that it didn't generate any kind of error. Behat just silently fails when including lib/tests/behat/behat_data_generators.php

      We should try and ensure that the error is reported better

      You can pull from git://github.com/danpoltawski/moodle.git failing-behat to see this.

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          David discovered this was caused by:

          defined('MOODLE_INTERNAL') || die(); 
          

          Which we can't do anything about (other than making the die more verbose or giving some info of inclusion).

          Show
          Dan Poltawski added a comment - David discovered this was caused by: defined('MOODLE_INTERNAL') || die(); Which we can't do anything about (other than making the die more verbose or giving some info of inclusion).
          Hide
          David Monllaó added a comment -

          Talking with Petr about possible workarounds, generators requires moodle libraries to be loaded to work properly so they should also require MOODLE_INTERNAL, he didn't comment anything about the initial decision of not adding them to the generators files.

          Show
          David Monllaó added a comment - Talking with Petr about possible workarounds, generators requires moodle libraries to be loaded to work properly so they should also require MOODLE_INTERNAL, he didn't comment anything about the initial decision of not adding them to the generators files.
          Hide
          Damyon Wiese added a comment -

          Discussed this with David,

          If we change the step definitions so that they require the generators from inside the functions then we can put the MOODLE_INTERNAL checks back in the generator files.

          Show
          Damyon Wiese added a comment - Discussed this with David, If we change the step definitions so that they require the generators from inside the functions then we can put the MOODLE_INTERNAL checks back in the generator files.
          Hide
          David Monllaó added a comment -

          Requesting peer review; pull branch including MDL-39716 integrated.

          I've only added a master pull branch, in case MDL-39716 is backported the patch can be cherry picked

          Show
          David Monllaó added a comment - Requesting peer review; pull branch including MDL-39716 integrated. I've only added a master pull branch, in case MDL-39716 is backported the patch can be cherry picked
          Hide
          Dan Poltawski added a comment -

          Looks OK here, makes sense, sending for integration.

          Show
          Dan Poltawski added a comment - Looks OK here, makes sense, sending for integration.
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          David Monllaó added a comment -

          Rebased including a MDL-39477 patch

          Show
          David Monllaó added a comment - Rebased including a MDL-39477 patch
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm a bit confused...

          1) MDL-39716 is a typo, I bet. What was David pointing to there?
          2) this includes MDL-39477, but that one seems to be held right now, although, overall, it seems correct to be integrated both for master and 25_STABLE. More yet, it makes a reference to MDL-38498 that was integrated last week...

          So? There are 2 solutions IMO:

          • Or we held both (only justifiable if we are going to push them to master only).
          • Or we integrate both (issues) in both (branches). With the MOODLE_INTERNAL check reintroduced.

          My soul goes to the second approach... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm a bit confused... 1) MDL-39716 is a typo, I bet. What was David pointing to there? 2) this includes MDL-39477 , but that one seems to be held right now, although, overall, it seems correct to be integrated both for master and 25_STABLE. More yet, it makes a reference to MDL-38498 that was integrated last week... So? There are 2 solutions IMO: Or we held both (only justifiable if we are going to push them to master only). Or we integrate both (issues) in both (branches). With the MOODLE_INTERNAL check reintroduced. My soul goes to the second approach... ciao
          Hide
          Dan Poltawski added a comment -

          +1 to integrate both!

          Show
          Dan Poltawski added a comment - +1 to integrate both!
          Hide
          Damyon Wiese added a comment -

          Thanks David,

          This has been integrated to 2.5 and master along with MDL-39477.

          Show
          Damyon Wiese added a comment - Thanks David, This has been integrated to 2.5 and master along with MDL-39477 .
          Hide
          Frédéric Massart added a comment -

          Passing, thanks!

          Show
          Frédéric Massart added a comment - Passing, thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has now been sent upstream and will soon be downloaded by millions of Moodlers! Regards, Damyon

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: