Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Unit tests
    • Labels:
    • Testing Instructions:
      Hide

      1. Run the unit tests on a very slow computer. Verify that they don't crash out in the middle.

      (I have already done this!)

      From Petr's comments below:

      2. Uncomment the //$warning and you will get problems in stats, extlib course duplication and assignment upgrade code - all those places do change timeouts. (and means code is being executed ok).

      Show
      1. Run the unit tests on a very slow computer. Verify that they don't crash out in the middle. (I have already done this!) From Petr's comments below: 2. Uncomment the //$warning and you will get problems in stats, extlib course duplication and assignment upgrade code - all those places do change timeouts. (and means code is being executed ok).
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
    • Rank:
      49008

      Description

      I am seeing failures during the unit tests:

      PHP Fatal error: Maximum execution time of 300 seconds exceeded in C:\Users\tjh238\workspace\moodle_head\lib\moodlelib.php on line 10745

      As far as I can work out, what is going on is this:

      1. Some code somewhere calls set_time_limit supposedly to avoid time-outs.
      2. If the tests run fast enough, then there is no problem, but if your computer is slow, then some time later the tests time-out on an unrelated test.

      I think we should add code to reset_all_data to detect changes in max_execution_time, and report them as errors.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I am about to attach a patch for discussion.

          Show
          Tim Hunt added a comment - I am about to attach a patch for discussion.
          Hide
          Tim Hunt added a comment -

          This change lets me run the unit tests.

          I will probably need a second commit to fix the tests that currently call set_time_limit, that will come later.

          Show
          Tim Hunt added a comment - This change lets me run the unit tests. I will probably need a second commit to fix the tests that currently call set_time_limit, that will come later.
          Hide
          Petr Škoda added a comment - - edited

          makes sense, +1

          oh, I do not think this requires any warning, CLI phpunit execution is quite special

          Show
          Petr Škoda added a comment - - edited makes sense, +1 oh, I do not think this requires any warning, CLI phpunit execution is quite special
          Hide
          Tim Hunt added a comment -

          I think the warning is valuable. I think the warning is valuable. Our library code should never change max execution time. Imaging if later someone calls that library code somewhere in some cron code. That is bad.

          As it happens, the warning only generates three problems in core at the moment:

          1) core_textlib_testcase::test_parse_charset
          Warning: max_execution_time was changed.
          
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\advanced_testcase.php:118
          C:\Program Files (x86)\PHP\phpunit:46
          
          To re-run:
           C:\Program Files (x86)\PHP\phpunit core_textlib_testcase lib\tests\textlib_test.php
          
          2) qubaid_condition_test::test_qubaid_list_one_join
          Warning: max_execution_time was changed.
          
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\advanced_testcase.php:118
          C:\Program Files (x86)\PHP\phpunit:46
          
          To re-run:
           C:\Program Files (x86)\PHP\phpunit qubaid_condition_test question\engine\tests\datalib_test.php
          
          3) mod_lti_locallib_testcase::test_split_custom_parameters
          Warning: max_execution_time was changed.
          
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220
          C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\basic_testcase.php:76
          C:\Program Files (x86)\PHP\phpunit:46
          
          To re-run:
           C:\Program Files (x86)\PHP\phpunit mod_lti_locallib_testcase mod\lti\tests\locallib_test.php
          

          I am about to look at those in detail.

          Show
          Tim Hunt added a comment - I think the warning is valuable. I think the warning is valuable. Our library code should never change max execution time. Imaging if later someone calls that library code somewhere in some cron code. That is bad. As it happens, the warning only generates three problems in core at the moment: 1) core_textlib_testcase::test_parse_charset Warning: max_execution_time was changed. C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220 C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\advanced_testcase.php:118 C:\Program Files (x86)\PHP\phpunit:46 To re-run: C:\Program Files (x86)\PHP\phpunit core_textlib_testcase lib\tests\textlib_test.php 2) qubaid_condition_test::test_qubaid_list_one_join Warning: max_execution_time was changed. C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220 C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\advanced_testcase.php:118 C:\Program Files (x86)\PHP\phpunit:46 To re-run: C:\Program Files (x86)\PHP\phpunit qubaid_condition_test question\engine\tests\datalib_test.php 3) mod_lti_locallib_testcase::test_split_custom_parameters Warning: max_execution_time was changed. C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\util.php:220 C:\Users\tjh238\workspace\moodle_head\lib\phpunit\classes\basic_testcase.php:76 C:\Program Files (x86)\PHP\phpunit:46 To re-run: C:\Program Files (x86)\PHP\phpunit mod_lti_locallib_testcase mod\lti\tests\locallib_test.php I am about to look at those in detail.
          Hide
          Tim Hunt added a comment -

          The problem is, sometimes the test that causes the problem does not inherit from advanced_testcase, so then the test that gets blamed is not the right test. For example, in 1) it must be the previous test.

          Show
          Tim Hunt added a comment - The problem is, sometimes the test that causes the problem does not inherit from advanced_testcase, so then the test that gets blamed is not the right test. For example, in 1) it must be the previous test.
          Hide
          Petr Škoda added a comment -

          hmm, maybe you are right, the libs should not mess with timeouts and if they do tests can hack around it, please fix all warnings in separate commit before submitting for integration

          Show
          Petr Škoda added a comment - hmm, maybe you are right, the libs should not mess with timeouts and if they do tests can hack around it, please fix all warnings in separate commit before submitting for integration
          Hide
          Tim Hunt added a comment -

          I think that just resetting the time limit to 0 is useful, and should be intergrated now.

          I don't currently have time to sort out the places that are changing max_execution_time, so I created MDL-38989 for turning on the warning.

          Show
          Tim Hunt added a comment - I think that just resetting the time limit to 0 is useful, and should be intergrated now. I don't currently have time to sort out the places that are changing max_execution_time, so I created MDL-38989 for turning on the warning.
          Hide
          Tim Hunt added a comment -

          OK, submitting the set_time_limit(0); bit for integration, with the warning commented out, with a TODO.

          Show
          Tim Hunt added a comment - OK, submitting the set_time_limit(0); bit for integration, with the warning commented out, with a TODO.
          Hide
          Petr Škoda added a comment -

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Petr Škoda added a comment -

          Hmm, while working on MDL-38989 I discovered this gives bogus warnings and skips resets because you need to reset the timeout outside of the "if ($logchanges) {"

          This should resolve it for now: https://github.com/skodak/moodle/compare/6bb78ae...w15_MDL-38912_m25_testtimeouts

          Show
          Petr Škoda added a comment - Hmm, while working on MDL-38989 I discovered this gives bogus warnings and skips resets because you need to reset the timeout outside of the "if ($logchanges) {" This should resolve it for now: https://github.com/skodak/moodle/compare/6bb78ae...w15_MDL-38912_m25_testtimeouts
          Hide
          Tim Hunt added a comment -

          Just moving this to the testing failed state, so the integrators know to add Petr's extra patch.

          Thanks Petr for spotting this problem and proposing the fix. It looks good to me.

          Show
          Tim Hunt added a comment - Just moving this to the testing failed state, so the integrators know to add Petr's extra patch. Thanks Petr for spotting this problem and proposing the fix. It looks good to me.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, my fault, i should have looked a bit more in context...grrr.

          Petr's patch applied to 23, 24 and master, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, my fault, i should have looked a bit more in context...grrr. Petr's patch applied to 23, 24 and master, thanks!
          Hide
          Petr Škoda added a comment -

          To test this uncomment the //$warinng and you will get problems in stats, extlib course duplication and assignment upgrade code - all those places do change timeouts.

          Show
          Petr Škoda added a comment - To test this uncomment the //$warinng and you will get problems in stats, extlib course duplication and assignment upgrade code - all those places do change timeouts.
          Hide
          Mark Nelson added a comment -

          Works as expected, passing.

          When uncommented, I received the warning the following number of times for each version.

          2.3: 1
          2.4: 11
          master: 15

          Thanks.

          Show
          Mark Nelson added a comment - Works as expected, passing. When uncommented, I received the warning the following number of times for each version. 2.3: 1 2.4: 11 master: 15 Thanks.
          Hide
          Mark Nelson added a comment -

          I should also add that all these warnings were in the places Petr said they would be.

          Show
          Mark Nelson added a comment - I should also add that all these warnings were in the places Petr said they would be.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: