Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41198

Unit tests fail in core_cache_testcase (for at least two Windows systems)

    Details

    • Testing Instructions:
      Hide

      On each of linux, windows and Mac (as the PHP server)
      Using two classes of PHP on Windows (PHP > 5.4.14+, 5.5.1+ and PHP less than those versions)

      1. Run units tests ensure they pass and no errors (PHP > 5.4.14+, 5.5.1+)
      2. Login and record your sesskey.
      3. Logout and back in and confirm your sesskey is different.

      Show
      On each of linux, windows and Mac (as the PHP server) Using two classes of PHP on Windows (PHP > 5.4.14+, 5.5.1+ and PHP less than those versions) 1. Run units tests ensure they pass and no errors (PHP > 5.4.14+, 5.5.1+) 2. Login and record your sesskey. 3. Logout and back in and confirm your sesskey is different.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41198-master

      Description

      On my system, and at least one other in the wild (see this forum discussion https://moodle.org/mod/forum/discuss.php?d=233467), either 2 or 3 unit tests in the complete run always fail. Here is the output:

      There were 3 failures:
       
      1) core_cache_testcase::test_session_cache_switch_user
      Failed asserting that 'RYpeokgWPc' is not equal to <string:RYpeokgWPc>.
       
      C:\Users\sm449\workspace\core-moodle-github\cache\tests\cache_test.php:1201
      C:\Users\sm449\workspace\core-moodle-github\lib\phpunit\classes\advanced_testcase.php:76
       
      To re-run:
       c:/Users/sm449/workspace/core-moodle-github/vendor/phpunit/phpunit/composer/bin/phpunit core_cache_testcase cache\tests\cache_test.php
       
      2) core_cache_testcase::test_session_cache_switch_user_application_mapping
      Failed asserting that 'Jl8SwMDn1a' is not equal to <string:Jl8SwMDn1a>.
       
      C:\Users\sm449\workspace\core-moodle-github\cache\tests\cache_test.php:1237
      C:\Users\sm449\workspace\core-moodle-github\lib\phpunit\classes\advanced_testcase.php:76
       
      To re-run:
       c:/Users/sm449/workspace/core-moodle-github/vendor/phpunit/phpunit/composer/bin/phpunit core_cache_testcase cache\tests\cache_test.php
       
      3) core_cache_testcase::test_session_cache_switch_user_multiple
      Failed asserting that 'SSFoxtGSBH' is not equal to <string:SSFoxtGSBH>.
       
      C:\Users\sm449\workspace\core-moodle-github\cache\tests\cache_test.php:1302
      C:\Users\sm449\workspace\core-moodle-github\lib\phpunit\classes\advanced_testcase.php:76
      

      This can NOT be reproduced on Tim's system even though we run on the same infrastructure. I haven't heard of another developer seeing this issue, either. So basically not reproducible.

      It may indicate a problem with my settings in config.php in which case it could be useful to add the answer to the docs page: http://docs.moodle.org/dev/Common_unit_test_problems

        Gliffy Diagrams

          Activity

          Hide
          timhunt Tim Hunt added a comment -

          I get this too, latest master. I did not used to get it a few weeks ago.

          Show
          timhunt Tim Hunt added a comment - I get this too, latest master. I did not used to get it a few weeks ago.
          Hide
          timhunt Tim Hunt added a comment -

          I also get this in MOODLE_25_STABLE.

          Show
          timhunt Tim Hunt added a comment - I also get this in MOODLE_25_STABLE.
          Hide
          davidaylmer David Aylmer added a comment -

          Perhaps author of the test file, Sam Hemelryk, may have some ideas?

          Show
          davidaylmer David Aylmer added a comment - Perhaps author of the test file, Sam Hemelryk, may have some ideas?
          Hide
          davidaylmer David Aylmer added a comment -

          I haven't altered any of the standard moodle cache settings.
          Tried with zend opcache enabled and disabled in php.ini. Fails under both conditions.

          Windows 2008 R2, PHP 5.5.1

          Show
          davidaylmer David Aylmer added a comment - I haven't altered any of the standard moodle cache settings. Tried with zend opcache enabled and disabled in php.ini. Fails under both conditions. Windows 2008 R2, PHP 5.5.1
          Hide
          salvetore Michael de Raadt added a comment -

          I am not seeing these fails currently under Windows. I'm running PHP 5.4.7 with memcached and opcache running.

          Show
          salvetore Michael de Raadt added a comment - I am not seeing these fails currently under Windows. I'm running PHP 5.4.7 with memcached and opcache running.
          Hide
          quen Sam Marshall added a comment -

          I also tested and these failures no longer occur for me. Since that's two of us, I think it's probably okay to close the issue now as it probably was fixed by some other change. David/Tim - If it still happens for you on current master, we can reopen?

          Show
          quen Sam Marshall added a comment - I also tested and these failures no longer occur for me. Since that's two of us, I think it's probably okay to close the issue now as it probably was fixed by some other change. David/Tim - If it still happens for you on current master, we can reopen?
          Hide
          mr-russ Russell Smith added a comment -

          Well, we've now seen this twice on 64bit Ubuntu virtual machine in the last couple of days.

          Random string depends on mt_srand() at this point. Which when called is initialized to the fraction of a second that you called microtime. So on machines with microsecond resolution, there is a 1 in a million chance you will get the same value when calling it twice in the same code.

          The php manual at http://www.php.net/manual/en/function.mt-srand.php indicates that the mt_srand call is not longer required since php 4.2. As php initializes it itself. If that's the case, it will only get initialized once per session and would remove the unit testing errors. I've not investigated the implications of how PHP initializes it automatically in the case of mod_php or cli. I don't expect it can be worse than the chances we have of duplicating it.

          Given my current understanding of this issue, I'm of the opinion we remove the mt_srand call in random_string and see if we can ever reproduce the error again. I feel I'm likely to see it again as it's happening once every 1-2 days. Are there any security implications I'm not thinking of by making this change?

          All input welcome and encouraged.

          Show
          mr-russ Russell Smith added a comment - Well, we've now seen this twice on 64bit Ubuntu virtual machine in the last couple of days. Random string depends on mt_srand() at this point. Which when called is initialized to the fraction of a second that you called microtime. So on machines with microsecond resolution, there is a 1 in a million chance you will get the same value when calling it twice in the same code. The php manual at http://www.php.net/manual/en/function.mt-srand.php indicates that the mt_srand call is not longer required since php 4.2. As php initializes it itself. If that's the case, it will only get initialized once per session and would remove the unit testing errors. I've not investigated the implications of how PHP initializes it automatically in the case of mod_php or cli. I don't expect it can be worse than the chances we have of duplicating it. Given my current understanding of this issue, I'm of the opinion we remove the mt_srand call in random_string and see if we can ever reproduce the error again. I feel I'm likely to see it again as it's happening once every 1-2 days. Are there any security implications I'm not thinking of by making this change? All input welcome and encouraged.
          Hide
          davidaylmer David Aylmer added a comment -

          Interesting. I'm still able to reproduce this fail every single time in my windows environment. (moodle 2.5.2 - PHP 5.5.1) I know that windows PHP random number support historically has been a lot different to nix. (/dev/urandom etc..). I'm willing to speculate that the test case functions differently to core moodle, because otherwise - wouldn't I expect to see strange behaviour from within the application?

          Show
          davidaylmer David Aylmer added a comment - Interesting. I'm still able to reproduce this fail every single time in my windows environment. (moodle 2.5.2 - PHP 5.5.1) I know that windows PHP random number support historically has been a lot different to nix. (/dev/urandom etc..). I'm willing to speculate that the test case functions differently to core moodle, because otherwise - wouldn't I expect to see strange behaviour from within the application?
          Hide
          skodak Petr Skoda added a comment -

          Could you please try latest PHP 5.5.5? (There is no reason to use or test with outdated PHP version in Windows.)

          Show
          skodak Petr Skoda added a comment - Could you please try latest PHP 5.5.5? (There is no reason to use or test with outdated PHP version in Windows.)
          Hide
          mr-russ Russell Smith added a comment -

          This is also related and supports the seeding the same random series hypothesis; http://stackoverflow.com/questions/18889244/php-5-5-on-windows-microtime-behavior

          Show
          mr-russ Russell Smith added a comment - This is also related and supports the seeding the same random series hypothesis; http://stackoverflow.com/questions/18889244/php-5-5-on-windows-microtime-behavior
          Hide
          mr-russ Russell Smith added a comment -

          More information: https://bugs.php.net/bug.php?id=64633 indicates on windows you only get 15.6ms resolution for microtime() on php 5.4.14+ and php 5.5.1+

          I'm publishing a master branch that I think will resolve this issue.

          Show
          mr-russ Russell Smith added a comment - More information: https://bugs.php.net/bug.php?id=64633 indicates on windows you only get 15.6ms resolution for microtime() on php 5.4.14+ and php 5.5.1+ I'm publishing a master branch that I think will resolve this issue.
          Hide
          mr-russ Russell Smith added a comment -

          Please comment and post any objections to using this as a fix.

          Show
          mr-russ Russell Smith added a comment - Please comment and post any objections to using this as a fix.
          Hide
          timhunt Tim Hunt added a comment -

          Have you looked through git history to see if there was any comment when the call to seed the random number generator was added?

          Rather that just removing that call, why not either A) move it to setup.php, or B) use a static so that we do it only once each PHP process?

          Show
          timhunt Tim Hunt added a comment - Have you looked through git history to see if there was any comment when the call to seed the random number generator was added? Rather that just removing that call, why not either A) move it to setup.php, or B) use a static so that we do it only once each PHP process?
          Hide
          mr-russ Russell Smith added a comment -

          It was added as part of the function addition;

          commit 4c1ba3ffc74355f79a6114774fc81bea0a6dcd66
          Author: Petr Skoda <skodak@moodle.org>
          Date: Tue Nov 17 16:11:25 2009 +0000

          MDL-18006 MDL-18807 MDL-20853 merging sam's changes from MOODLE_19_STABLE, going to fix some more problems in this new code soon

          I do not have permissions to view any of these tracker items.

          The PHP manual indicates that it already does the work for you so there is no need for calling mt_srand at all. This means we don't need to proceed with either of A) or B). If we think it's really required, moving it to setup.php may be useful. I'm not sure how that interacts with unit tests. Either way, we want to initialize the generator once per page rather than once per call.

          We don't support PHP4, and the requirement for initialization was removed in 4.2. Is there a reason you think it's good for us to seed the generator? Is it because we don't trust PHP?

          Show
          mr-russ Russell Smith added a comment - It was added as part of the function addition; commit 4c1ba3ffc74355f79a6114774fc81bea0a6dcd66 Author: Petr Skoda <skodak@moodle.org> Date: Tue Nov 17 16:11:25 2009 +0000 MDL-18006 MDL-18807 MDL-20853 merging sam's changes from MOODLE_19_STABLE, going to fix some more problems in this new code soon I do not have permissions to view any of these tracker items. The PHP manual indicates that it already does the work for you so there is no need for calling mt_srand at all. This means we don't need to proceed with either of A) or B). If we think it's really required, moving it to setup.php may be useful. I'm not sure how that interacts with unit tests. Either way, we want to initialize the generator once per page rather than once per call. We don't support PHP4, and the requirement for initialization was removed in 4.2. Is there a reason you think it's good for us to seed the generator? Is it because we don't trust PHP?
          Hide
          skodak Petr Skoda added a comment -

          I believe there is no reason to use it any more, the linked issues above are not relevant.

          +3 to remove this line, thanks a lot for the report and patch

          please submit to integration when ready, I guess execution of unit tests is enough as testing instructions

          Show
          skodak Petr Skoda added a comment - I believe there is no reason to use it any more, the linked issues above are not relevant. +3 to remove this line, thanks a lot for the report and patch please submit to integration when ready, I guess execution of unit tests is enough as testing instructions
          Hide
          davidaylmer David Aylmer added a comment -

          Petr,

          I've now tried this on my environment with 5.5.5 and I get the same errors.

          Show
          davidaylmer David Aylmer added a comment - Petr, I've now tried this on my environment with 5.5.5 and I get the same errors.
          Hide
          skodak Petr Skoda added a comment -

          Thanks David, could you please also try to delete the following line form your lib/moodlelib.php and run the tests?

          mt_srand ((double) microtime() * 1000000);

          Show
          skodak Petr Skoda added a comment - Thanks David, could you please also try to delete the following line form your lib/moodlelib.php and run the tests? mt_srand ((double) microtime() * 1000000);
          Hide
          mr-russ Russell Smith added a comment -

          I've updated the current patch to remove all versions of mt_srand from the code.

          I've now looks at all calls to srand. I've successfully removed most of those, however there is one that I have question about. I'd like a 2nd/3rd opinion on. I've added Matt Clarkson as he wrote the code originally.

          group/autogroup.php:87   srand($data->seed)

          I'm not 100% clear if there is value in having the seed set to a specific time for each load of the form. Does the value get updated each time the form loads? My Moodle form fu is still low. If it does, it's safe to remove. But if not, I suspect there is some value it in but I'm unclear of what. I would have assumed the call would have been srand(time()) if we didn't want to keep the same seed for multiple runs.

          Feedback from others?

          Thanks.

          Show
          mr-russ Russell Smith added a comment - I've updated the current patch to remove all versions of mt_srand from the code. I've now looks at all calls to srand. I've successfully removed most of those, however there is one that I have question about. I'd like a 2nd/3rd opinion on. I've added Matt Clarkson as he wrote the code originally. group/autogroup.php:87 srand($data->seed) I'm not 100% clear if there is value in having the seed set to a specific time for each load of the form. Does the value get updated each time the form loads? My Moodle form fu is still low. If it does, it's safe to remove. But if not, I suspect there is some value it in but I'm unclear of what. I would have assumed the call would have been srand(time()) if we didn't want to keep the same seed for multiple runs. Feedback from others? Thanks.
          Hide
          mr-russ Russell Smith added a comment -

          I've decided to leave the srand call in for group allocation. The form will keep shuffling the users in the same order whenever you reload it for other unrelated changes. This seems reasonable and could be adjusted at another time if required.

          Show
          mr-russ Russell Smith added a comment - I've decided to leave the srand call in for group allocation. The form will keep shuffling the users in the same order whenever you reload it for other unrelated changes. This seems reasonable and could be adjusted at another time if required.
          Hide
          skodak Petr Skoda added a comment -

          thanks, submitting for integration

          Show
          skodak Petr Skoda added a comment - thanks, submitting for integration
          Hide
          marina Marina Glancy added a comment -

          Hi, thanks for working on this guys.
          My only comment is about a change in lib/adodb/adodb.inc.php - this is a part of an external library. We either should not change it or reflect the changes in lib/adodb/readme_moodle.txt
          Just had a chat with Petr and he said: "+1 for the readme_moodle.txt together with upstream report"
          Russell, can you please add a line in lib/adodb/readme_moodle.txt ?
          Thanks

          Show
          marina Marina Glancy added a comment - Hi, thanks for working on this guys. My only comment is about a change in lib/adodb/adodb.inc.php - this is a part of an external library. We either should not change it or reflect the changes in lib/adodb/readme_moodle.txt Just had a chat with Petr and he said: "+1 for the readme_moodle.txt together with upstream report" Russell, can you please add a line in lib/adodb/readme_moodle.txt ? Thanks
          Hide
          mr-russ Russell Smith added a comment -

          Comment has been added as requested. I also back-patched this to 2.6, 2.5 and 2.4 as it's causing failures for users working on those branches. As it's not a security issue, 2.3 has been left out.

          Show
          mr-russ Russell Smith added a comment - Comment has been added as requested. I also back-patched this to 2.6, 2.5 and 2.4 as it's causing failures for users working on those branches. As it's not a security issue, 2.3 has been left out.
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Russell, integrated to master, 26, 25 and 24

          Show
          poltawski Dan Poltawski added a comment - Thanks Russell, integrated to master, 26, 25 and 24
          Hide
          poltawski Dan Poltawski added a comment -

          Looking good on mac on 24, 25, 26 and master

          Show
          poltawski Dan Poltawski added a comment - Looking good on mac on 24, 25, 26 and master
          Hide
          poltawski Dan Poltawski added a comment -

          I've tested on linux / mac and windows on 5.3 and 5.4. I haven't tested on windows 5.5, but i'm using integrators privilege to pass it as it seems there isn't a readily available install here and i've got a lot of other things on.

          Show
          poltawski Dan Poltawski added a comment - I've tested on linux / mac and windows on 5.3 and 5.4. I haven't tested on windows 5.5, but i'm using integrators privilege to pass it as it seems there isn't a readily available install here and i've got a lot of other things on.
          Hide
          damyon Damyon Wiese added a comment -

          Twas the week before Christmas,
          And all though HQ
          Devs were scrambling to finish peer review.
          They sent all their issues,
          and rushed out the door -
          "To the beach!" someone heard them roar!

          This issue has been released upstream. Thanks!

          Show
          damyon Damyon Wiese added a comment - Twas the week before Christmas, And all though HQ Devs were scrambling to finish peer review. They sent all their issues, and rushed out the door - "To the beach!" someone heard them roar! This issue has been released upstream. Thanks!

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                13/Jan/14