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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Assigning to Yuliya as she agreed to take it on, thanks!
            Hide
            ybozhko 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
            ybozhko 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Hmm, I think we could create a couple of tests of core processors which demonstrate this issue.
            Hide
            ankit_frenz 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_frenz 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
            ybozhko 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
            ybozhko 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
            ybozhko 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
            ybozhko 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_frenz 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_frenz 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
            ybozhko 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
            ybozhko 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
            poltawski Dan Poltawski added a comment -

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

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

            ps. We definitely need unit tests for this

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

            Cool. Will be working on tests then. Thanks!

            Show
            ybozhko Yuliya Bozhko added a comment - Cool. Will be working on tests then. Thanks!
            Hide
            ybozhko 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
            ybozhko 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
            poltawski 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
            poltawski 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
            ybozhko 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
            ybozhko 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
            poltawski Dan Poltawski added a comment -

            Pressing the button.

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

            Thanks Yuliya, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Yuliya, this has been integrated now.
            Hide
            samhemelryk 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
            samhemelryk 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  11/Nov/13