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

Automate MDLQA-5257 - An admin can award a site badge

    Details

    • Testing Instructions:
      Hide

      vendor/bin/behat --config=/home/jerome/moodles/stable_master/moodledata_behat/behat/behat.yml --name "Award badges"

      vendor/bin/behat --config=/home/jerome/moodles/stable_25/moodledata_behat/behat/behat.yml --name "Award badges"

      Show
      vendor/bin/behat --config=/home/jerome/moodles/stable_master/moodledata_behat/behat/behat.yml --name "Award badges" vendor/bin/behat --config=/home/jerome/moodles/stable_25/moodledata_behat/behat/behat.yml --name "Award badges"
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-42330-master-2
    • Sprint:
      FRONTEND Sprint 6
    • Story Points (Obsolete):
      3
    • Sprint:
      FRONTEND Sprint 6

      Description

      As MDLQA-5257 described, an admin can award a site badge

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dmonllao David Monllaó added a comment -

            Hi Jerome,

            Thanks for converting this MDLQA test to a behat test, looks good, just a few comments:

            • Please rebase one issue on top of the other one and add it as a blocker, there are integration conflicts right now
            • As talked we can rearrange the award_badge.feature scenarios to remove duplicates and give more info about what we are testing, this is also applicable to MDL-42331, you can remove Add criteria scenario as is included in Earn badge and rename all scenarios according to what they are testing (Earn badge by profile completion for example)
            • Will be worth to add more I should see and I should not see steps, is not just about converting the MDLQA to behat, the more regressions we detect the better. Rather than using I follow, which is not supposed to be used to assert we can use I should (not) see
            • The *I wait "5" seconds" is not necessary, the framework is supposed to manage the redirection waits. There are special cases, but I don't see it here
            • I know that is not strictly part of this issue, but as you worked on both badges tests, feel free to update Earn badge scenario according to the tests you are adding
            • As talked the test should be split in 3 sections, there is no When, probably would be around I create badge or I set the criteria

            Some of those points are also applicable to MDL-42441 review (commenting on it now)

            Show
            dmonllao David Monllaó added a comment - Hi Jerome, Thanks for converting this MDLQA test to a behat test, looks good, just a few comments: Please rebase one issue on top of the other one and add it as a blocker, there are integration conflicts right now As talked we can rearrange the award_badge.feature scenarios to remove duplicates and give more info about what we are testing, this is also applicable to MDL-42331 , you can remove Add criteria scenario as is included in Earn badge and rename all scenarios according to what they are testing (Earn badge by profile completion for example) Will be worth to add more I should see and I should not see steps, is not just about converting the MDLQA to behat, the more regressions we detect the better. Rather than using I follow, which is not supposed to be used to assert we can use I should (not) see The *I wait "5" seconds" is not necessary, the framework is supposed to manage the redirection waits. There are special cases, but I don't see it here I know that is not strictly part of this issue, but as you worked on both badges tests, feel free to update Earn badge scenario according to the tests you are adding As talked the test should be split in 3 sections, there is no When, probably would be around I create badge or I set the criteria Some of those points are also applicable to MDL-42441 review (commenting on it now)
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I kept it in two commits, I find it easier to review the first one, then the second one than the final diff.

            Show
            jerome Jérôme Mouneyrac added a comment - I kept it in two commits, I find it easier to review the first one, then the second one than the final diff.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Jerome,

            In http://docs.moodle.org/dev/Coding_style#Git_commits we say:

            Git commits should not:
            Include changes from bugs found and fixed before integration

            It looks like your two commits are doing this? Would you consider squashing the commits?

            Show
            poltawski Dan Poltawski added a comment - Hi Jerome, In http://docs.moodle.org/dev/Coding_style#Git_commits we say: Git commits should not: Include changes from bugs found and fixed before integration It looks like your two commits are doing this? Would you consider squashing the commits?
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I do multiple commits for peer-review/integrator so when they retest they don't need to re-read the all things. And for this one it considered it was easier to read both that all diff in once. Also I can remember seen many people commenting they prefer having multiple commit to re-review that re-read the all stuff all once again.

            This was just for the explanation, so it makes sense to you why I decided to do like that.
            Squashing both in one

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I do multiple commits for peer-review/integrator so when they retest they don't need to re-read the all things. And for this one it considered it was easier to read both that all diff in once. Also I can remember seen many people commenting they prefer having multiple commit to re-review that re-read the all stuff all once again. This was just for the explanation, so it makes sense to you why I decided to do like that. Squashing both in one
            Hide
            poltawski Dan Poltawski added a comment -

            Yep - makes total sense - much better for peer reviewer.

            Show
            poltawski Dan Poltawski added a comment - Yep - makes total sense - much better for peer reviewer.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated to master and 25 - thanks Jerome

            Show
            poltawski Dan Poltawski added a comment - Integrated to master and 25 - thanks Jerome
            Hide
            jerome Jérôme Mouneyrac added a comment -

            arh I was squashing... ah ... I just did master anyway...

            Show
            jerome Jérôme Mouneyrac added a comment - arh I was squashing... ah ... I just did master anyway...
            Hide
            jerome Jérôme Mouneyrac added a comment -

            integration deadline day issue

            Show
            jerome Jérôme Mouneyrac added a comment - integration deadline day issue
            Hide
            andyjdavis Andrew Davis added a comment -

            Tests passed. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Tests passed. Passing.
            Hide
            damyon Damyon Wiese added a comment -

            Here lies 52 bugs.
            All fixed or swept under a rug.
            If they come back one day,
            To our dismay,
            We all will feel quite un-smug.

            Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            Show
            damyon Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FYI: related MDLQA-5257 has been moved from MDLQA-1 to MDLQA-5249 (bag of behat-converted tests). Thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - FYI: related MDLQA-5257 has been moved from MDLQA-1 to MDLQA-5249 (bag of behat-converted tests). Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13

                  Agile