Moodle
  1. Moodle
  2. MDL-38498

Add units tests for repository check_capability

    Details

      Description

      Re: the regression in MDL-38474 we really need unit tests in the repository api to prevent this type of issue sneaking in again.

      Unit test should test combinations of repository context and current context.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Frédéric Massart added a comment -

            +300! Repositories are really lacking of Unit Testing!

            Show
            Frédéric Massart added a comment - +300! Repositories are really lacking of Unit Testing!
            Hide
            Frédéric Massart added a comment -

            Pushing for peer review.

            • I did not extensively add tests to confirm the capability check, but more the logic within check_capability().
            • The master branch is BLOCKED by MDL-39477 which introduces data generators
            • I only backported to 2.4, I think it's safer this way. But 2.3 and 2.4 should be almost the same.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Pushing for peer review. I did not extensively add tests to confirm the capability check, but more the logic within check_capability(). The master branch is BLOCKED by MDL-39477 which introduces data generators I only backported to 2.4, I think it's safer this way. But 2.3 and 2.4 should be almost the same. Cheers, Fred
            Hide
            Dan Poltawski added a comment -

            Hi Fred,

            This looks great.

            1. repository_generator_testcase::test_create_type() is too clever for my liking with the logic. Its very minor but..
              1. Its a case of conditional logic smell: http://xunitpatterns.com/Conditional%20Test%20Logic.html
              2. It seems unnecessary to have the logic there like that. You don't use the $list again, so you could just exclude those plugins from the list rather than doing the conditional if.
              3. Furthermore the randomness in the unit tests isn't good. What if one of them gets disabled from the default install, then we'll get a sproadic failure. For throughness you should test that they all fail to be added.
              4. Since you've enabled all of them, shouldn't you actually be able to try and enable any plugin, not just the ones from the default install..
            2. I would put your $caughtexception = false; outside of the try{} blocks for better style. A catch is not like an if/else and from a style point of view that false could never get executed, leaving the leftovers from the last test.
            3. Comment: Do you really need to call set_role_contextlevels()? I didn't get the impression we used that for anything but the UI.
            4. Comment: I'm suprised that session_loginas() works in the unit test environemnt

            Looks good anyway, like I say.

            Show
            Dan Poltawski added a comment - Hi Fred, This looks great. repository_generator_testcase::test_create_type() is too clever for my liking with the logic. Its very minor but.. Its a case of conditional logic smell: http://xunitpatterns.com/Conditional%20Test%20Logic.html It seems unnecessary to have the logic there like that. You don't use the $list again, so you could just exclude those plugins from the list rather than doing the conditional if. Furthermore the randomness in the unit tests isn't good. What if one of them gets disabled from the default install, then we'll get a sproadic failure. For throughness you should test that they all fail to be added. Since you've enabled all of them, shouldn't you actually be able to try and enable any plugin, not just the ones from the default install.. I would put your $caughtexception = false; outside of the try{} blocks for better style. A catch is not like an if/else and from a style point of view that false could never get executed, leaving the leftovers from the last test. Comment: Do you really need to call set_role_contextlevels()? I didn't get the impression we used that for anything but the UI. Comment: I'm suprised that session_loginas() works in the unit test environemnt Looks good anyway, like I say.
            Hide
            Dan Poltawski added a comment -

            Oh and the unit tests failed for me with generator error. Guess thats because of the blocker, surprised they are not inherented from the same branch though.

            Show
            Dan Poltawski added a comment - Oh and the unit tests failed for me with generator error. Guess thats because of the blocker, surprised they are not inherented from the same branch though.
            Hide
            Frédéric Massart added a comment - - edited

            Thanks Dan,

            actually, you reviewed the blocker as well as the data generator comes from it. I have so updated the linked issue to match your comments. Now I am still only enabling the types which are not enabled during installation, but after that I check that all the types are enabled. This issue was rebased on the blocked, it's not the case any more (it made it easier to update both).

            2/ I moved the $cauchexception outside the try.
            3/ You are right, I removed that.
            4/ So was I

            (The tests failed because I apparently didn't update the tests properly, thanks for spotting that! I can't believe I didn't run them one last time...)

            Show
            Frédéric Massart added a comment - - edited Thanks Dan, actually, you reviewed the blocker as well as the data generator comes from it. I have so updated the linked issue to match your comments. Now I am still only enabling the types which are not enabled during installation, but after that I check that all the types are enabled. This issue was rebased on the blocked, it's not the case any more (it made it easier to update both). 2/ I moved the $cauchexception outside the try. 3/ You are right, I removed that. 4/ So was I (The tests failed because I apparently didn't update the tests properly, thanks for spotting that! I can't believe I didn't run them one last time...)
            Hide
            Dan Poltawski added a comment -

            Hi Fred,

            You can send this for integration, but you've got a critical behat-breaking problem in MDL-39477, which i've commented on.

            Show
            Dan Poltawski added a comment - Hi Fred, You can send this for integration, but you've got a critical behat-breaking problem in MDL-39477 , which i've commented on.
            Hide
            Frédéric Massart added a comment -

            Thanks, I will fix that!

            Show
            Frédéric Massart added a comment - Thanks, I will fix that!
            Hide
            Damyon Wiese added a comment -

            Fred,

            This issue is a bug and has backports but you have set an improvement issue as a blocker. I can't integrate this during the on-sync period (the improvement will get integration_held). Can you change the master branch for this issue to match the 25 branch and then add these changes to MDL-39477 ?

            Thanks, Damyon

            Show
            Damyon Wiese added a comment - Fred, This issue is a bug and has backports but you have set an improvement issue as a blocker. I can't integrate this during the on-sync period (the improvement will get integration_held). Can you change the master branch for this issue to match the 25 branch and then add these changes to MDL-39477 ? Thanks, Damyon
            Hide
            Frédéric Massart added a comment -

            Thanks Damyon, I've moved the specific commit to the blocker (which I'm setting as related). Cheers!

            Show
            Frédéric Massart added a comment - Thanks Damyon, I've moved the specific commit to the blocker (which I'm setting as related). Cheers!
            Hide
            Damyon Wiese added a comment -

            Also Fred,

            What is the purpose of removing the line:

            set_role_contextlevels($roleid, array($syscontext->contextlevel, $coursecontext->contextlevel));

            in the 25 branch?

            The 25 branch and master cannot have any differences.

            Show
            Damyon Wiese added a comment - Also Fred, What is the purpose of removing the line: set_role_contextlevels($roleid, array($syscontext->contextlevel, $coursecontext->contextlevel)); in the 25 branch? The 25 branch and master cannot have any differences.
            Hide
            Frédéric Massart added a comment -

            Sorry Damyon, I missed that. I've updated the patches.

            Show
            Frédéric Massart added a comment - Sorry Damyon, I missed that. I've updated the patches.
            Hide
            Damyon Wiese added a comment -

            Thanks Fred,

            This has been integrated to 24, 25 and master.

            Show
            Damyon Wiese added a comment - Thanks Fred, This has been integrated to 24, 25 and master.
            Hide
            Damyon Wiese added a comment -

            Test passed on 24, 25 and master branches. Thanks Fred!

            Show
            Damyon Wiese added a comment - Test passed on 24, 25 and master branches. Thanks Fred!
            Hide
            Damyon Wiese added a comment -

            Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

            Closing as Fixed!

            Show
            Damyon Wiese added a comment - Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone. Closing as Fixed!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: