Accession spawn Accession #17
Conversation
blacksmith-welder
changed the title
blacksmith-welder
force-pushed
the
accession_accession_spawn_feature
branch
from
ef12429 to
20237f4
Compare
kdimopulu
requested review from
balmas,
brianzelip,
donaldjosephsmith and
thimios
balmas
left a comment
balmas
left a comment
There was a problem hiding this comment.
Looks good. A couple of minor changes suggested.
| expect(page).to have_field(label, checked: true) | ||
| end | ||
|
|
||
| Then 'the following error message is displayed' do |messages| |
There was a problem hiding this comment.
maybe consider renaming to 'only the following error message is displayed' since that's what distinguishes this method from the prior one which also tests for a message being displayed
There was a problem hiding this comment.
I agree that it is more distinguishable that way. I have prepended the word only to all the steps that refer to a single message, as well as a check that there is only one message wherever it was missing.
Some notes, which also apply to the other reviews (16)
@kdimopulu suggested the same thing before submitting for review, but I thought that since the distinction is already there (plural/singular), it's redundant to prepend the word only, to make it as simple and intuitive as possible for the Gherkin writer, without having to search other steps or referring to the documentation.
From the technical point of view, the error/info messages can always be an arbitrary amount of entries in Gherkin/Cucumber, so we could even use the plural version for everything, to make it even simpler.
eg.: Somebody could write the following for a single message, which is totally valid:
Given ...
When ...
Then the following error messages are displayed
| Expression - is required unless a begin or end date is given |
Then ...
| expect(find('.alert.alert-success.with-hide-alert').text).to match(/^#{string}.*updated$/i) | ||
| end | ||
|
|
||
| Then('the following info message is displayed') do |messages| |
There was a problem hiding this comment.
s/b 'the following info messages are displayed' because it accepts and tests multiple
There was a problem hiding this comment.
Fixed. There is a check for messages length now.
blacksmith-welder
force-pushed
the
accession_accession_spawn_feature
branch
4 times, most recently
from
faf13d9 to
1c04ca1
Compare
blacksmith-welder
force-pushed
the
accession_accession_spawn_feature
branch
from
a7ee499 to
70b6d9a
Compare
No description provided.