Moodle
  1. Moodle
  2. MDL-38498

Add units tests for repository check_capability

    Details

    • Rank:
      48488

      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.

        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: