Moodle
  1. Moodle
  2. MDL-38110

Using MUC in code breaks ttl unit test

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.1, 2.5
    • Fix Version/s: 2.4.2, 2.5
    • Component/s: Caching
    • Labels:
    • Rank:
      47928

      Description

      Sam, it seems like any my attempt to use MUC in code leads to breaking the ttl unit test. I will link issues being blocked by this one

      cache_phpunit_tests::test_definition_ttl
      Failed asserting that true is false.
      
      /home/marina/repositories/courserenderers/moodle/cache/tests/cache_test.php:437
      /home/marina/repositories/courserenderers/moodle/lib/phpunit/classes/advanced_testcase.php:76
      
      To re-run:
       /home/marina/pear/bin/phpunit cache_phpunit_tests cache/tests/cache_test.php
      

      If I execute just "To re-run" command, everything is ok. The error occurs only if all unittests are executed

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Looking at it now thanks Marina.

          Just to confirm I am able to replicate this.

          Show
          Sam Hemelryk added a comment - Looking at it now thanks Marina. Just to confirm I am able to replicate this.
          Hide
          Sam Hemelryk added a comment -

          I think I've identified the problem now:
          The test in question is setting up a cache with a negative time stamp of 10 seconds. It then purges the cache to be extra sure nothing is in it before running the actual tests.
          The purging of that cache is taking more than 10 seconds in the situation in which all tests are being run.
          The solution is simply to increase the negative ttl. Using something like -86400 (a day) is going to be a sure fire option. Given this is testing functionality and not performance that is probably the best solution, however there is certainly a performance issue in there as well.

          Show
          Sam Hemelryk added a comment - I think I've identified the problem now: The test in question is setting up a cache with a negative time stamp of 10 seconds. It then purges the cache to be extra sure nothing is in it before running the actual tests. The purging of that cache is taking more than 10 seconds in the situation in which all tests are being run. The solution is simply to increase the negative ttl. Using something like -86400 (a day) is going to be a sure fire option. Given this is testing functionality and not performance that is probably the best solution, however there is certainly a performance issue in there as well.
          Hide
          Sam Hemelryk added a comment -

          Marina could you please try the following patch and see if that solves the problem for you when working with your branches?

          Repo: git://github.com/samhemelryk/moodle.git
          Branch: wip-MDL-38110-m25
          Diff: http://github.com/samhemelryk/moodle/compare/master...wip-MDL-38110-m25

          Hopefully that is as simple as it is.

          Show
          Sam Hemelryk added a comment - Marina could you please try the following patch and see if that solves the problem for you when working with your branches? Repo: git://github.com/samhemelryk/moodle.git Branch: wip- MDL-38110 -m25 Diff: http://github.com/samhemelryk/moodle/compare/master...wip-MDL-38110-m25 Hopefully that is as simple as it is.
          Hide
          Marina Glancy added a comment -

          Hi Sam, I tested it with one of my issues and all tests passed all right. But the solution seems a bit strange for me.

          Can we have another ttl test with positive value - where you set ttl for, say, 2s and then sleep for 3 seconds and check the cache?

          Show
          Marina Glancy added a comment - Hi Sam, I tested it with one of my issues and all tests passed all right. But the solution seems a bit strange for me. Can we have another ttl test with positive value - where you set ttl for, say, 2s and then sleep for 3 seconds and check the cache?
          Hide
          Sam Hemelryk added a comment -

          My apologies, I was sure I had commented here and pushed this for integration.

          In summary we can't use a positive TTL as the cache API uses a timestamp taken during initialisation to avoid timing problems.
          This effectively means anything set during a request is available for the lifetime of the request. Doing this aids us in avoiding timing related issues, and will hopefully mitigate the risk of cache regeneration and syncronisation issues during long running requests (think cron etc).

          The negative ttl is works because the it sets the ttl back before that timestamp.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - My apologies, I was sure I had commented here and pushed this for integration. In summary we can't use a positive TTL as the cache API uses a timestamp taken during initialisation to avoid timing problems. This effectively means anything set during a request is available for the lifetime of the request. Doing this aids us in avoiding timing related issues, and will hopefully mitigate the risk of cache regeneration and syncronisation issues during long running requests (think cron etc). The negative ttl is works because the it sets the ttl back before that timestamp. Many thanks Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 & master), thanks!

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

          I'm passing this, tests worked ok.

          Show
          Eloy Lafuente (stronk7) added a comment - I'm passing this, tests worked ok.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: