Activity

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

David Monllaó
added a comment - 08/Mar/13 8:35 AM 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

Frédéric Massart
added a comment - 11/Mar/13 6:31 AM 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

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.

David Monllaó
added a comment - 12/Mar/13 2:42 AM 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.

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.

Eloy Lafuente (stronk7)
added a comment - 14/Mar/13 3:40 PM 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

Eloy Lafuente (stronk7)
added a comment - 19/Mar/13 11:28 PM - 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?>"
?