Moodle
  1. Moodle
  2. MDL-42331

Automate MDLQA-5258 - A teacher can add and allocate a course badge

    Details

    • Testing Instructions:
      Hide

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

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

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

      Description

      As described in MDLQA-5258, a teacher can add and allocate a course badge

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            David Monllaó added a comment -

            Hi,

            Also looks good Jerome, but please go through https://tracker.moodle.org/browse/MDL-42330?focusedCommentId=251202&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-251202 comments that are also applicable here (the wait, naming including criteria, I should see...)

            Adding to this, would be good if rather than awarding both students with the badge you only award one, but you login as both of them to check that one sees the badge and not the other one, so we can detect possible regressions.

            Show
            David Monllaó added a comment - Hi, Also looks good Jerome, but please go through https://tracker.moodle.org/browse/MDL-42330?focusedCommentId=251202&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-251202 comments that are also applicable here (the wait, naming including criteria, I should see...) Adding to this, would be good if rather than awarding both students with the badge you only award one, but you login as both of them to check that one sees the badge and not the other one, so we can detect possible regressions.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            In my opinion if a behat test tests the main feature then it's enough. Otherwise we'll end to have endless peer-review/integration-review with: "you didn't test that", "I would have tested that", "As you are here also test that", etc...

            That said, we still should mention and fix things like "Don't use wait X seconds" or "These two tests are the same, please merge them".

            Also I think it's not necessary to do:

            Then I should see "Profile Badge"
            And I should not see "There are no badges available."

            That's all I wanted to mention about behat writing policy

            Show
            Jérôme Mouneyrac added a comment - - edited In my opinion if a behat test tests the main feature then it's enough. Otherwise we'll end to have endless peer-review/integration-review with: "you didn't test that", "I would have tested that", "As you are here also test that", etc... That said, we still should mention and fix things like "Don't use wait X seconds" or "These two tests are the same, please merge them". Also I think it's not necessary to do: Then I should see "Profile Badge" And I should not see "There are no badges available." That's all I wanted to mention about behat writing policy
            Show
            Jérôme Mouneyrac added a comment - - edited Integrators, you can take the last commit (this issue is blocked by MDL-42330 ). master: https://github.com/mouneyrac/moodle/commit/f061e95ef1b8d1c0872df989296ad9fd37332fd1 2.5: https://github.com/mouneyrac/moodle/commit/458a6f7bb25ffecac78f5d2c0d109554c129e77d
            Hide
            David Monllaó added a comment -

            I completely disagree with that, having a 1 minute test to check that a user can see a badge is not worth at all although I agree that there should be boundaries; we can include that single checking in another scenario and that's all, there are lots of regressions that we can catch and solve before a user reports to the tracker and this is this testing framework purpose.

            Show
            David Monllaó added a comment - I completely disagree with that, having a 1 minute test to check that a user can see a badge is not worth at all although I agree that there should be boundaries; we can include that single checking in another scenario and that's all, there are lots of regressions that we can catch and solve before a user reports to the tracker and this is this testing framework purpose.
            Hide
            Jérôme Mouneyrac added a comment - - edited

            David, it's true, it seems like a 1 minute job but it's not 1 minute job. For each remarks you have to switch your mind back to the issue, make the change to restest, repush... Additionally we are adding time to the issue being resolved.

            Let's take as example with this issue :
            you suggested "would be good if rather than awarding both students with the badge you only award one, but you login as both of them to check that one sees the badge and not the other one, so we can detect possible regressions."
            a) I was testing that no issues happen when you attribut the badge to two people at the same time and you suggest to remove it for...
            b) you want to test that one user is receving the badges
            I can not say like that that a) is less likely to fail than b), actually I would think that an error in a) could be more probable even thought they both look imporbable to me.
            So you could now reformulate that I don't need to remove the user but add another third one... but I think we already already slipping over the behat test goal.

            So for something that could be considered optional and could have been integrated we are now spending more time. I just want to bring people attention to this potential review difficulty. It's just my opinion on a review difficulty problem that I think it's going to happen quite often.

            Finally I understand your point of view. More tests done, more regressions found. I agree with you on this.
            But as we have to balance thing I was suggesting that the reviewer should just check if the user "red route" is tested. And in a behat test the "red route" is the scenario name.

            Show
            Jérôme Mouneyrac added a comment - - edited David, it's true, it seems like a 1 minute job but it's not 1 minute job. For each remarks you have to switch your mind back to the issue, make the change to restest, repush... Additionally we are adding time to the issue being resolved. Let's take as example with this issue : you suggested "would be good if rather than awarding both students with the badge you only award one, but you login as both of them to check that one sees the badge and not the other one, so we can detect possible regressions." a) I was testing that no issues happen when you attribut the badge to two people at the same time and you suggest to remove it for... b) you want to test that one user is receving the badges I can not say like that that a) is less likely to fail than b), actually I would think that an error in a) could be more probable even thought they both look imporbable to me. So you could now reformulate that I don't need to remove the user but add another third one... but I think we already already slipping over the behat test goal. So for something that could be considered optional and could have been integrated we are now spending more time. I just want to bring people attention to this potential review difficulty. It's just my opinion on a review difficulty problem that I think it's going to happen quite often. Finally I understand your point of view. More tests done, more regressions found. I agree with you on this. But as we have to balance thing I was suggesting that the reviewer should just check if the user "red route" is tested. And in a behat test the "red route" is the scenario name.
            Hide
            David Monllaó added a comment -

            I mean that is a 1 minute test for a CI job with one test like this is not a problem, with 300 as we have it will be a problem. Most of that minute is spend setting up the environment while the assertions have been optimized and the time they require is near 0, so adding lots of assertions will not change the elapsed time will require you (the developer) to code them which is not a lot if you begin writing the from that point of view.

            I didn't think that you needed to add more assertions to send the issue to integration, a peer review leaves that to your decision (I think that with a bit of common sense everyone knows when things must be changed and when could be changed) but, as a general advice, and always IMO, having scenarios that test just one thing is not enough, and is not the best way to invest the time we have, this is what I think is important to remark, the time invested in writing the test + review + integration + discussions like this against one single assertion... if we would do the same with all the tests we have I would say that is not worth to have automated functional testing.

            Show
            David Monllaó added a comment - I mean that is a 1 minute test for a CI job with one test like this is not a problem, with 300 as we have it will be a problem. Most of that minute is spend setting up the environment while the assertions have been optimized and the time they require is near 0, so adding lots of assertions will not change the elapsed time will require you (the developer) to code them which is not a lot if you begin writing the from that point of view. I didn't think that you needed to add more assertions to send the issue to integration, a peer review leaves that to your decision (I think that with a bit of common sense everyone knows when things must be changed and when could be changed) but, as a general advice, and always IMO, having scenarios that test just one thing is not enough, and is not the best way to invest the time we have, this is what I think is important to remark, the time invested in writing the test + review + integration + discussions like this against one single assertion... if we would do the same with all the tests we have I would say that is not worth to have automated functional testing.
            Hide
            Jérôme Mouneyrac added a comment -

            No problem I didn't understand you were speaking about CI. All what you said make sense to me.

            Sent to integration... actually it was already done but I like to finish with this line

            Show
            Jérôme Mouneyrac added a comment - No problem I didn't understand you were speaking about CI. All what you said make sense to me. Sent to integration... actually it was already done but I like to finish with this line
            Hide
            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
            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
            Dan Poltawski added a comment -

            Integrated to master and 25

            Show
            Dan Poltawski added a comment - Integrated to master and 25
            Hide
            Dan Poltawski added a comment -

            Thanks Jerome

            Show
            Dan Poltawski added a comment - Thanks Jerome
            Hide
            Ankit Agarwal added a comment -

            works as described
            1 scenario (1 passed)
            32 steps (32 passed)

            Show
            Ankit Agarwal added a comment - works as described 1 scenario (1 passed) 32 steps (32 passed)
            Hide
            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 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
            Eloy Lafuente (stronk7) added a comment -

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

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

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile