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 Master Branch:

      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.

        Gliffy Diagrams

          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 Skoda added a comment - - edited

            makes sense, +1

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

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda added a comment -

            +1

            Show
            Petr Skoda 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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: