Details

      Description

      See summary.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dmonllao David Monllaó added a comment -

            Hi Fred,

            It look very good, just a few things.

            • The name of the feature can be add_comments.feature to be a bit more descriptive
            • The In order to is supposed to give info about why do we need to "Comment on a blog entry", something like "In order to give my opinion or provide feedback", is just an example
            • FYI after logging in you are in homepage, you can skip the "I am on homepage" step of the background section if you want, but is also ok if it's there as it provides info
            • This is a MDLQA test, when is possible is better to use nasty strings instead of strings without nasty caracthers ($NASTYSTRING1) The way you used the background section is perfect, but note that there is an issue (MDL-38346) using nasty strings in the background section, is currently waiting for peer review so until it's patch is integrated this would fail in a 90% change if you switch to nasty strings.
            • After finishing a scenario you don't need to logout as the state of the site and the session is reset before each scenario, what you have done and is really important is to save changes or cancel after finishing the tests, otherwise there will appear that popup stating if you are sure to leave the current page without saving changes.
            • When I click on ".comment-link" "css_element" I guess you already tried with Comments (0), when possible is better to point with human-readable strings
            • Most of the time when interacting with elements you can use I follow "Save comment", the I click.... is more suitable for cases where you can simply use a I follow...
            • Is better to be conservative with the wait times to avoid false failures, when running tests in remote servers the response time is worst, I usually use around 4 seconds
            • As you did the background section should be only used to set up the initial status to begin writting scenarios, so only 'Given' as prefix for the steps; but inside the scenarios there should only be a Given -> When -> Then transition, I mean this would be correct:
              Given
              And
              And
              And
              When
              And
              And
              Then
              And
              You are not restricted to use the prefix Then with the I should see "I've got nothing to say!" step, if what you are testing requires this Then steps to be used during the scenario and not only at the end means that probably you can split the scenario and you are testing multiple scenarios at the same time. In the Commenting on my own blog entry case you could split it in different scenarios:
            • Commenting on my own blog entry
            • Deleting my own comments

            This point is somehow controversial because we have to balance when are we repeating the same steps for more than a test against having descriptive and readable scenarios.

            Don't hesitate to comment if there are parts of the documentation or the steps definitions descriptions that can be improved to make a better use of them

            Show
            dmonllao David Monllaó added a comment - Hi Fred, It look very good, just a few things. The name of the feature can be add_comments.feature to be a bit more descriptive The In order to is supposed to give info about why do we need to "Comment on a blog entry", something like "In order to give my opinion or provide feedback", is just an example FYI after logging in you are in homepage, you can skip the "I am on homepage" step of the background section if you want, but is also ok if it's there as it provides info This is a MDLQA test, when is possible is better to use nasty strings instead of strings without nasty caracthers ($NASTYSTRING1) The way you used the background section is perfect, but note that there is an issue ( MDL-38346 ) using nasty strings in the background section, is currently waiting for peer review so until it's patch is integrated this would fail in a 90% change if you switch to nasty strings. After finishing a scenario you don't need to logout as the state of the site and the session is reset before each scenario, what you have done and is really important is to save changes or cancel after finishing the tests, otherwise there will appear that popup stating if you are sure to leave the current page without saving changes. When I click on ".comment-link" "css_element" I guess you already tried with Comments (0) , when possible is better to point with human-readable strings Most of the time when interacting with elements you can use I follow "Save comment" , the I click.... is more suitable for cases where you can simply use a I follow... Is better to be conservative with the wait times to avoid false failures, when running tests in remote servers the response time is worst, I usually use around 4 seconds As you did the background section should be only used to set up the initial status to begin writting scenarios, so only 'Given' as prefix for the steps; but inside the scenarios there should only be a Given -> When -> Then transition, I mean this would be correct: Given And And And When And And Then And You are not restricted to use the prefix Then with the I should see "I've got nothing to say!" step, if what you are testing requires this Then steps to be used during the scenario and not only at the end means that probably you can split the scenario and you are testing multiple scenarios at the same time. In the Commenting on my own blog entry case you could split it in different scenarios: Commenting on my own blog entry Deleting my own comments This point is somehow controversial because we have to balance when are we repeating the same steps for more than a test against having descriptive and readable scenarios. Don't hesitate to comment if there are parts of the documentation or the steps definitions descriptions that can be improved to make a better use of them
            Hide
            fred Frédéric Massart added a comment -

            Many thanks David, here it is for a second review.

            • I kept the name comment.feature as it is not only about adding them, and also this is a sub feature of the main component blog.
            • I have adapted a bit the In order to, are you ok with that?
            • As the background goes here and there, I preferred keeping the I am on homepage here, to be sure I start back from where I am expecting.
            • I tried using nastystrings but it'd fail. The string could not be located afterwards. Not sure if it's a bug from the comment API, or Behat so I left it without but added my own few characters.
            • I am using I follow more often, it's more readable as you said.
            • I have properly split into 3 distinctive steps Given, When and Then.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Many thanks David, here it is for a second review. I kept the name comment.feature as it is not only about adding them, and also this is a sub feature of the main component blog . I have adapted a bit the In order to , are you ok with that? As the background goes here and there, I preferred keeping the I am on homepage here, to be sure I start back from where I am expecting. I tried using nastystrings but it'd fail. The string could not be located afterwards. Not sure if it's a bug from the comment API, or Behat so I left it without but added my own few characters. I am using I follow more often, it's more readable as you said. I have properly split into 3 distinctive steps Given, When and Then. Cheers, Fred
            Hide
            dmonllao David Monllaó added a comment -

            Hi Fred,

            It looks very good and is ready for integration from my point of view, only a couple of points you might (or might not as is nothing really important) want to consider:

            • You don't need to specify the password in Given the following "users" exists: as is not a required field and the same username will be used as pwd
            • And I follow "Logout" can be replaced by And I log out

            About the nasty strings I'll comment on MDL-37858, the former issue, I've found the same problems, it would not be that easy to use them to assert against them, as moodle applies different filters (PARAM_TEXT, PARAM_CLEAN, RAW...) to different fields and the test writer is not supposed to know it, also it would require to look at the source code most of the time, so probably developers specially interested in testing with this particular strings and using PARAM_RAW would be the ones interested on it.

            Show
            dmonllao David Monllaó added a comment - Hi Fred, It looks very good and is ready for integration from my point of view, only a couple of points you might (or might not as is nothing really important) want to consider: You don't need to specify the password in Given the following "users" exists: as is not a required field and the same username will be used as pwd And I follow "Logout" can be replaced by And I log out About the nasty strings I'll comment on MDL-37858 , the former issue, I've found the same problems, it would not be that easy to use them to assert against them, as moodle applies different filters (PARAM_TEXT, PARAM_CLEAN, RAW...) to different fields and the test writer is not supposed to know it, also it would require to look at the source code most of the time, so probably developers specially interested in testing with this particular strings and using PARAM_RAW would be the ones interested on it.
            Hide
            fred Frédéric Massart added a comment -

            Thanks David, the patch is updated and pushed for integration. Cheers!

            Show
            fred Frédéric Massart added a comment - Thanks David, the patch is updated and pushed for integration. Cheers!
            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
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            uhm, when deleting the 2nd comment... should it be:

            And I should not see "$My own >nasty< \"string\"!"

            or

            And I should not see "Another $Nasty <string?>"

            ?

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited uhm, when deleting the 2nd comment... should it be: And I should not see "$My own >nasty< \"string\"!" or And I should not see "Another $Nasty <string?>" ?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            aha, forget, by bad. I was mixing scenarios and miss-interpretting how many comments were being deleted.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - aha, forget, by bad. I was mixing scenarios and miss-interpretting how many comments were being deleted.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            dmonllao David Monllaó added a comment -

            Passing as running in the CI server

            Show
            dmonllao David Monllaó added a comment - Passing as running in the CI server
            Hide
            damyon Damyon Wiese added a comment -

            This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            Thanks for your contributions!

            Show
            damyon Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            FYI: related MDLQA-1157 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-1157 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:
                  Fix Release Date:
                  14/May/13