Moodle
  1. Moodle
  2. MDL-41228

get_message_processors() has a static cache which is not reset by phpunit tests

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1
    • Fix Version/s: 2.5.3
    • Component/s: Messages, Unit tests
    • Labels:

      Description

      As Yuliya discovered, the static cache in get_message_processors() is not reset between tests creating variable results.

      We probably need to add a reset param to the function and include it in the pphunit resets.

        Gliffy Diagrams

          Activity

          Hide
          Dan Poltawski added a comment -

          Assigning to Yuliya as she agreed to take it on, thanks!

          Show
          Dan Poltawski added a comment - Assigning to Yuliya as she agreed to take it on, thanks!
          Hide
          Yuliya Bozhko added a comment -

          Not sure about testing instructions...

          I discovered the problem when I was trying to test our custom message processor. While tests running separately worked fine, it was failing when I tried to run all Moodle unit tests together. Digging into it I discovered that my message processor 'enabled' property was set to 0 when I ran all tests together.

          Show
          Yuliya Bozhko added a comment - Not sure about testing instructions... I discovered the problem when I was trying to test our custom message processor. While tests running separately worked fine, it was failing when I tried to run all Moodle unit tests together. Digging into it I discovered that my message processor 'enabled' property was set to 0 when I ran all tests together.
          Hide
          Dan Poltawski added a comment -

          Hmm, I think we could create a couple of tests of core processors which demonstrate this issue.

          Show
          Dan Poltawski added a comment - Hmm, I think we could create a couple of tests of core processors which demonstrate this issue.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Yuliya,
          Patch looks great, will it possible to add some tests for this change? We have some similar asserts in core_phpunit_advanced_testcase::test_change_detection()

          Also should we not just return after setting $processors = array(); instead of going through rest of the api?

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Yuliya, Patch looks great, will it possible to add some tests for this change? We have some similar asserts in core_phpunit_advanced_testcase::test_change_detection() Also should we not just return after setting $processors = array(); instead of going through rest of the api? Thanks
          Hide
          Yuliya Bozhko added a comment -

          Hi Ankit,

          That's a good idea about return. I will have a look at what I can do about adding some tests. Thanks!

          Show
          Yuliya Bozhko added a comment - Hi Ankit, That's a good idea about return. I will have a look at what I can do about adding some tests. Thanks!
          Hide
          Yuliya Bozhko added a comment -

          Oh wait. Actually, return won't work here. What we want is to reset an array of processors and get them from database rather than relying on the cached version of this array.

          Show
          Yuliya Bozhko added a comment - Oh wait. Actually, return won't work here. What we want is to reset an array of processors and get them from database rather than relying on the cached version of this array.
          Hide
          Ankit Agarwal added a comment -

          Hi Yuliya,
          I am not completely sure about this, shouldn't we reset it to the original state which was array()? Something similar to what events_get_handlers('reset'); does?

          However I could be wrong. I have added Andrew as watcher, who is lead of message for more feedback.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Yuliya, I am not completely sure about this, shouldn't we reset it to the original state which was array()? Something similar to what events_get_handlers('reset'); does? However I could be wrong. I have added Andrew as watcher, who is lead of message for more feedback. Thanks
          Hide
          Yuliya Bozhko added a comment -

          I don't know either... It makes sense, but would be good to get the third opinion to be sure

          Thanks!

          Show
          Yuliya Bozhko added a comment - I don't know either... It makes sense, but would be good to get the third opinion to be sure Thanks!
          Hide
          Dan Poltawski added a comment -

          I think its ok for the function to continue as normal after reseting the cache.

          Show
          Dan Poltawski added a comment - I think its ok for the function to continue as normal after reseting the cache.
          Hide
          Dan Poltawski added a comment -

          ps. We definitely need unit tests for this

          Show
          Dan Poltawski added a comment - ps. We definitely need unit tests for this
          Hide
          Yuliya Bozhko added a comment -

          Cool. Will be working on tests then. Thanks!

          Show
          Yuliya Bozhko added a comment - Cool. Will be working on tests then. Thanks!
          Hide
          Yuliya Bozhko added a comment - - edited

          Hey guys,

          Can someone have a look if this kind of unit test makes sense https://github.com/totara/openbadges/commit/9e4f8dda2ba3b1090a72690a589a1259e3ff0c23 ?

          If yes, I will submit master branch too.

          Thanks!

          Yuliya

          Show
          Yuliya Bozhko added a comment - - edited Hey guys, Can someone have a look if this kind of unit test makes sense https://github.com/totara/openbadges/commit/9e4f8dda2ba3b1090a72690a589a1259e3ff0c23 ? If yes, I will submit master branch too. Thanks! Yuliya
          Hide
          Dan Poltawski added a comment -

          Hi Yuliya,

          Rather than use the full phpunit reset, i'd just explicitly call get_message_processors with the reset param, then this test method can be explicitly testing that functionality.

          (This wasn't actually my original idea for the test, but I think its a good thing to test) +1

          Show
          Dan Poltawski added a comment - Hi Yuliya, Rather than use the full phpunit reset, i'd just explicitly call get_message_processors with the reset param, then this test method can be explicitly testing that functionality. (This wasn't actually my original idea for the test, but I think its a good thing to test) +1
          Hide
          Yuliya Bozhko added a comment -

          Just making sure that it doesn't get lost Can someone please have a look at this one again? Thanks!

          Show
          Yuliya Bozhko added a comment - Just making sure that it doesn't get lost Can someone please have a look at this one again? Thanks!
          Hide
          Dan Poltawski added a comment -

          Pressing the button.

          Show
          Dan Poltawski added a comment - Pressing the button.
          Hide
          Sam Hemelryk added a comment -

          Thanks Yuliya, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Yuliya, this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Tested during integration review - before and after.
          All tests passed correctly and before + after phpunit runtime stats were near identical so passing this now.

          Show
          Sam Hemelryk added a comment - Tested during integration review - before and after. All tests passed correctly and before + after phpunit runtime stats were near identical so passing this now.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: