-
-
Notifications
You must be signed in to change notification settings - Fork 616
test: Prove that Scenarios support multiple Examples tables #1696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
93c9ae5 to
6149dee
Compare
| | number1 | number2 | result | | ||
| | 1 | 6 | 6 | | ||
| | 5 | 4 | 10 | | ||
| | 2 | 3 | 6 | | ||
| Examples: Big numbers | ||
| | number1 | number2 | result | | ||
| | 139 | 201 | 99 | | ||
| | 200 | 300 | 60000 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not wild about the structure of this feature (IMO it would be better if the examples specified correct values and the implementation in the FeatureContext produced incorrect answers, rather than the other way round). However I didn't want to go down the rabbit hole of restructuring all the other scenarios / features in this file. We can maybe address that when/if we restructure this feature to use a fixtures directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that would be a lot harder to implement though. Among all the examples, we want some of them to be incorrect while the others would be correct. The FeatureContext would look weird, by having to hardcode some specific failures (the one actually expected by the scenario) instead of doing multiplication.
As we explicitly want the scenario to fail in some cases, I find things a lot more understandable when the scenario itself defines expected values that are obviously failing for anyone able to do basic math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point, and I agree it'd be tricky to do with a feature that is implicitly passing / failing due to maths or some other "real-world" type concern.
I think for things like this I'd actually lean towards just being explicit about whether we want the step to pass or fail e.g. variations of:
Given something
When I run a step that fails
That feels like a clearer representation of what we actually want to prove, without the distraction of reading (even basic) maths to figure that out.
And a better example from a BDD perspective than the idea you might have started development where the specification itself is obviously wrong & needs to be corrected, rather than the failure being due to the implementation.
It's not a big deal though, and certainly not a priority for right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed better for cases where you want the step to always fail. But this approach does not work (at least not easily) for this case where we want some examples to fail a step but not all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a simple case, that could just be done with something like:
Given something
When I run a step that <result>
Examples:
| result |
| passes |
| fails |
Obviously more params / context could be added if required to make the scenario more readable.
6149dee to
a574ed3
Compare
| Examples: | ||
| | name | result | | ||
| | Bob | Hi Bob | | ||
| | Jenny | Hi Jenny | | ||
| Failed step: Then I should see "Hi Jenny" | ||
| Failed - got: Hi Bob (InvalidArgumentException) | ||
| | 123456 | '123456' doesn't look like a name? | | ||
| | Brian | Sorry Brian, you're banned | | ||
| Failed step: Then I should see "Sorry Brian, you're banned" | ||
| Failed - got: Hi Bob (InvalidArgumentException) | ||
| --- Failed scenarios: | ||
| features/test.feature:13 (on line 8) | ||
| features/test.feature:18 (on line 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As can be seen, the pretty printer correctly shows each example (and correctly shows original line numbers for any failures), but it squashes them together into a single "table". It also breaks the padding of the columns - because the rows are coming from different tables.
Fixing this requires a bit of work and likely a BC break - I've opened #1697 to track that.
Feature writers should be able to structure examples into multiple tables, so that they can be clearly grouped into different kinds of values. Historically, this was not supported - see Behat#1082 - and was believed to be a BC break in Behat. However, it was later implemented via a workaround in Behat/Gherkin#161 which was released in 2021. I have added test coverage to prove that features like this are executed as expected. As can be seen, the current pretty printer output is not perfect (we lose the detail of the separate tables in the original file) however it looks like changing this would be a BC break due to the way the pretty printer is implemented. Fixes Behat#1082 - I will open a separate issue to improve the pretty printer output with example tables (which may also relate to the upcoming Gherkin improvements to support descriptions on these nodes).
a574ed3 to
1f2741e
Compare
Feature writers should be able to structure examples into multiple tables, so that they can be clearly grouped into different kinds of values.
Historically, this was not supported - see #1082 - and was believed to be a BC break in Behat.
However, it was later implemented via a workaround in Behat/Gherkin#161 which was released in 2021.
I have added test coverage to prove that features like this are executed as expected. As can be seen, the current pretty printer output is not perfect (we lose the detail of the separate tables in the original file) however it looks like changing this would be a BC break due to the way the pretty printer is implemented.
Fixes #1082 - I will open a separate issue to improve the pretty printer output with example tables (which may also relate to the upcoming Gherkin improvements to support descriptions on these nodes).