-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
gherkin: Compiling empty scenarios #11
Comments
Consider there two feature files: Feature:
Background:
Given foo
Scenario: empty Feature:
Scenario: not empty
Given foo The first one should be |
Hi @mattwynne The compiler is pretty simple, compiler.rb is less than 200 LOC. It doesn't have unit tests, though - only approval tests (the I would start with these files:
Rather than changing the expected output by hand, I'd make the change you think is correct. Start with the Ruby implementation - you can do the other languages later. The fix is maybe to remove these lines:
Now, cd into Make deletes the generated file if the test fails (its presence indicates "test passed" and prevents make from running it next time). Disable that temporarily by commenting out Run
Does it look correct? Do you need to make more changes to the code? When you're happy, save it to the golden master:
After copying the new golden master over here, sync it to all the other gherkin implementations:
When you run This will of course cause the other Gherkin implementations' tests to fail, so now it's time to go and fix those. It's easier than you think - the structure is the same so you can hack on languages you don't know! I recommend fixing all implementations in the same branch/PR, one commit at a time. If you're stuck in a language, someone else will jump in and do it for you. Do try though. |
By producing an empty Pickle when the Scenario or Scenario Outline AST object has not steps (regardless of the existence of a Background with steps)? |
Ah yes of course! |
The alternative would be for the compiler to just diligently compile the test case as usual, and leave it up to the higher-level bits (e.g. the runner) to figure out that this step is from a background and still give the scenario an undefined status. Seems a little bit icky to me to have the compiler deciding about things like that. I'm happy to let this edge-case come out in the wash, TBH. |
I agree with @brasmusson - if there are no steps in the scenario, no steps in the pickle regardless of how many steps there are in the background. I don't see why you want to leave that decision to individual cucumber implementations when it can be done consistently in the Gherkin lib @mattwynne. Also seems like it would be hard for cucumber to determine whether pickle steps came from background or not. |
Currently the compilers treat the edge-case for a scenario with no steps (by not creating a Pickle) the same way regardless of any background content. So changing the outcome of that edge-case to creating an empty Pickle (regardless of any background content), should fit in nicely in the current compiler structure. |
I might not get to that until next week, so have a go at it if you want @brasmusson! |
I'm late to the party on this one but, for what it's worth, I see outlines test generators and backgrounds as test adjusters. So not pickling an outline without example rows makes sense because there are no concrete tests to wrap up. Similarly, a feature with only a background shouldn't pickle anything because it would be completely arbitrary to say what number of pickles should be made for anything besides 0 because there are still no actual tests to attach to. Once a scenario is added (whether it be direct with If I understand the point of pickles, they are the 'resolved' form of a test. We don't produce one pickle for an outline, we 'resolve' it and produce one pickle for each example row. We don't pickle up a scenario and then have an extra object for the background steps, we 'resolve' it and produce one pickle with all steps included. This being the case, I would expect one pickle per |
That!s a nice explanation @enkessler and I believe this is how it currently behaves. |
@aslakhellesoy According to our test data, no. Previous behavior: no pickles created for a scenario without steps. |
I disagree. Imagine you have this: Feature: Go shopping
Background: we have some money
Given I have £10
Scenario: spend some of it
When I buy a paper for £1
Then I should have £9 left
Scenario: spend some of it
When I buy a jumper for £11
Then I should be denied
But I should have £10 left Now we want to add some more scenarios, without fleshing them out, because we need to do some more analysis. We're adding them more as a note to remind ourselves that we have some more work to do. Feature: Go shopping
Background: we have some money
Given I have £10
Scenario: spend some of it
When I buy a paper for £1
Then I should have £9 left
Scenario: can't afford what we want
When I buy a jumper for £11
Then I should be denied
But I should have £10 left
# New scenarios
Scenario: get a discount
Scenario: got a loyalty card with a free gift We don't want those new scenarios to run at all, not even with the background. It potentially slows down the build, and it's pointless - there are no steps in the scenarios themselves. There is nothing new to learn from executing these scenarios. We do however want the report to contain information that we had 4 scenarios, 2 of which are undefined. We can represent that with empty pickles. If we did what you're suggesting, Cucumber would have to execute the 2 empty scenarios. It can't tell that its sole step came from a background and none from the scenario and decide not to execute it. |
Cucumber executing a test that got written down is a feature, not a bug. If you don't want a test to to run because it isn't finished yet, that is what filters are for. Slap a
If nothing else, having different behavior for this edge case is inconsistent. As soon as you add a single step in the Admittedly, the lack of any steps in a test aside from background ones is highly suspicious but if you want a tool warn you about suspicious things, that's what linters are for*. A compiler's job is to take in your source code (Gherkin) and spit out something actionable (Pickles), silly though that action may be. TLDR: preventing those test from running is someone else's job *We should really get back around to writing that thing. |
@aslakhellesoy Fun fact: I ran into this use case just the other day. I had a couple complete scenarios that had common enough setups that a If I upgrade to Gherkin 5 I will no longer have this nicety. :( |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
So I ran into this with Cucumber JVM. The general rule is that the state of a scenario is the worst state of its executed steps and passing (the best state) if no steps have been executed yet. By making this undefined two rather annoying edge cases are introduced:
Because we're deprecating non-strict mode undefined steps will always result in a failure. So I would suggest making empty scenarios fail. Perhaps with a pending exception/state. |
At the moment, it looks like the gherkin parser is parsing empty scenarios as requested in the initial message of that issue. cucumber-ruby is reporting the scenario as undefined. So, what needs to be done now? Should we try to aligning cucumber-jvm and cucumber-js on cucumber-ruby? Should that be part of that same issue? Maybe we could close it and open new one in cucumber-jvm and cucumber-js? |
Yes, this is a valid and passing junit test. But the equivalent is not a valid and passing So we should not rely only on how junit behaves to decide how cucumber should behave I guess. |
I don't think your fiddle worked. It's not showing me a pending test. |
Fixed :D |
PHP Unit does this by checking after the test was executed if any assertions were invoked. This is essentially a build in after hook. PHP Unit can do this because they provide their own assertion library. In Java there are many to chose from (most linters just check for the word assert).
If you pass in an empty function, it does pass. 😆
And mocha will execute hooks, even if the test is pending:
Though marking the test as |
@mpkorstanje there's something you said that I don't understand:
I don't see how we'd have an undefined step here since there are no steps. What did I miss? Let's use a concrete example, we like those 😁
Feature: Login
Background:
Given a user Dave
Scenario: Successful login
When Dave logs in with his password
Then he can access the dashboard
Scenario: Failed login The Let's imagine there's also a hook in this suite to reset the state of the database between tests.
const { Before } = require('@cucumber/cucumber')
Before(() => Database.reset) What I'd like to have happen is that the team can see in their results that there are two scenarios, and one of them is "not finished".
I can see that considering the case of the background steps adds complexity - I don't know if we have information in the pickle steps about whether a step came from a background or not - so it might be nice to ignore that one for now. I do think we should deal with hooks though. I don't think this would be very useful otherwise. |
I can imagine how we'd implement this in cucumber-ruby, once the pickle compiler is passing these empty scenarios through. Perhaps @aurelien-reeves and I should pair on this and we can show you what we come up with, for discussion. |
The proposal is the following rule:
This comes on top of the existing rule:
I'm making a two pronged argument as to why this is a bad idea. Prong 1: For example you write a plugin that prints a summary of a scenario executions. To make this possible, whenever an undefined step is encountered a suggested snippet event is emitted. So there is a third rule:
So you could collect all these suggested snippets for each scenario, and when a scenario finishes with the status "undefined" you print the list of suggested snippets. BUT if there are no snippets, the scenario had no steps and a different message must be printed. And that's a really weird edge case to deal with when writing a plugin. The reason you have this edge case is because there are two ways for a scenario to get the "undefined" state:
You could work around this by instead proposing:
But this will run into problems the second prong. Prong 2: Before- and after-hooks have access to the current scenario state. This for example allows them to take screenshots when a scenario did not pass. Following the the proposed rule the first before hook will see the scenario state as "undefined" because no steps have yet been executed. You could work around this by defining the default scenario state from the perspective of the consumer:
Implementation wise this is also kinda annoying because now depending on the consumer you have to calculate the state of the scenario differently. But more importantly this results in an inconsistency between hooks and event consumers when dealing with the empty scenario. The last after hook would see the scenario as passed while the event consumer would see the scenario as undefined. And of course there are ways to work around this and add more and more nuance. However by doing this we keep adding more and more complexity. Complexity that we absolutely shouldn't have to deal with to finesse what amounts to an edge case. In nearly every other test framework in existence the empty test passes by definition. Hence my proposal:
This allows us to treat the empty scenario as any other scenario, but with an empty list of steps. Before- and after-hooks are executed as the would for any other scenario with steps. Hooks and and event consumers can be build on the same assumptions. There is no weird interaction with suggested snippet events and the whole thing can be expressed elegantly as: private final List<Result> stepResults = new ArrayList<>();
public Status getStatus() {
if (stepResults.isEmpty()) {
return Status.
}
return max(stepResults, comparing(Result::getStatus)).getStatus();
} |
So far I've been talking about compiled pickles with no steps. You've gone down a different rabit hole with your example: For everything I wrote the following example can be used: Feature: Login
Scenario: Successful login
Given a user Dave
When Dave logs in with his password
Then he can access the dashboard
Scenario: Failed login Here the failed login scenario has an empty list of steps. However for this example: Feature: Login
Background:
Given a user Dave
Scenario: Successful login
When Dave logs in with his password
Then he can access the dashboard
Scenario: Failed login The question should be: "Does the Failed login scenario compile into a pickle with 0 or 1 steps". I'm somewhat ambivalent about either. With what I've been proposing so far regardless if the scenario has 1 or 0 steps, the outcome would probably be passed. However with the other proposal the outcome might also be undefined/pending. And then this distinction matters because it would flag a potential problem in the workflow. |
Anyway, I get the impression that all arguments for pending/undefined stem from the need to facilitate a test driven workflows where scenarios are intentionally incomplete. However we are discussing Cucumbers execution model and implicitly its event model. We are also trying to keep it consistent across multiple implementations and integrate it with various test runners and IDEs. What ever complexity we add here is expensive, time consuming and very hard to change. This means we need simple and well defined behaviors. Yet workflows are fuzzy and mostly involve people. This means we need configuration, flexibility and nuance. The execution model is the wrong abstraction layer to address these concerns. By the time we get this deep into Cucumbers guts workflow issues should have been dealt with. This is why I've been proposing a linter. Linters are the ideal tool to handle workflows. They don't require Cucumber to be executed. This makes them light weight in terms of dependencies and allows them to run while writing features. This removes a lot of complexity from the linter and potentially makes them language agnostic. Linters tend to come with configurable rules and a set of of defaults. This makes them ideal for dealing with individual workflows. Linters usually also tend to have machine consumable output for easy integration with IDE's and other tools. All things we can't really cram into Cucumbers execution model and shouldn't try. |
I'd say about 2-3 years of working on Cucumber. 😛 Like Cucumber Ruby, Cucumber JVM used to default to "Undefined" for empty scenarios. Along with strict/lenient-mode it has been an endless source of bugs and confusion. So I removed both from Cucumber JVM. I did this by defaulting to strict mode, removing lenient mode and treating empty scenarios as passed by definition. Now empty scenarios never fail the build while undefined scenarios always fail the build. In practice it hasn't made a lick of a difference for the end user. In the JVM ecosystem tests frameworks only support three test outcomes; passed, skipped, or failed. And in CI this is reduced to either passed or failed. So Cucumbers very nuanced test outcome with six possible states is ultimately reduced to a binary or trinary outcome. Simply not bothering with some of the nuance removed a bucket of complexity. This has yet to be a source of complaints. The bugs and inconsistencies on the other hand were. So I think it is important to look at this holistically. |
Good point here: maybe we should not try harder to report empty scenarios consistently. As you said, it may depend on the ecosystem. As it would make perfect sense to have empty scenarios reported as "pending" in ruby and JS, if it make absolutely no sense in Java (even the "skipped" one?), maybe we should stop trying do have the exact same behavior for it. That should not prevent the common packages to be consistent as the definitive status of a test remain hardly tied to the implementation itself |
Speaking of concrete examples, this is a reminder that, whatever is eventually decided, the test data will need to be updated (and, unless I am misinterpreting that expected output, the test has been inaccurate for years). https://github.com/cucumber/common/blob/master/gherkin/testdata/good/incomplete_scenario.feature Edit: To clarify, it looks to me like a pickle is getting created despite the feature file verbiage indicating that no pickle should get created. |
You've made a lot of points Rien! I appreciate the history you've been through with Cucumber-JVM since I raised this ticket back in 2017, and the clarity of thinking you can bring to this problem. I see two sides to this problem:
The original intent of this ticket was to focus on (1) - that's why I raised it in this repository, where the compiler is - because at the time empty scenarios were effectively "censored" from Cucumber by the compiler. Any debate about how to handle their execution and results would be moot because they didn't even appear in the message stream. My feeling is that although a linter is probably a good idea to have at some point, linting is a long way away at the moment, and it still makes sense for us to think about what is the right way for the compiler to express these edge cases. I'd like to clarify what the current behaviour of the compiler is here, as it may have changed since I raised the ticket. As @enkessler has pointed out, it also seems to be doing some stuff that's inconsistent with what we'd intended. |
For compilation, my take on the rules would be (and I don't know what the current behaviour is yet):
[1] i.e. the background steps are only slurped up into scenarios that are non-empty) At a common-sense/POLS level, those rules seem to make sense to me. The one I'm not sure about is a scenario outline. If we have a scenario outline with steps but no examples, should that also be represented as a single empty pickle? |
I would expect an outline with no examples to generate no pickles because an outline is merely a template from which actual scenarios are generated. An empty scenario, on the other hand, is at least a definite thing. We could debate on whether it should be a pickle with no steps or a pickle with some steps (inherited from a background) but we can say that it at least exists as a discrete test case on which a consumer might decide to take some action. Lacking example rows, however, an outline cannot be said to be any definite thing. We could justify making a thousand pickles from it as easily as justifying just one pickle because it's all being made up from nothing anyway. Zero pickles is a reasonably safe interpretation of no examples rows. |
Might be good to do your homework first. 😛 |
If we take the idea of "Write the code you wish you had" up a level and make it "Write the requirements you wish you had", then not knowing the current behavior might actually be a good thing. It could free one from the temptation of altering the requirements to reduce the effort needed to implement a change rather than sticking to the desired goal and worrying about how to get there until after it is settled. |
The PR cucumber/common#1498 is ready for review. It does not change any behavior in runtime code. It just add an acceptance tests of what is already happening. Fake cucumber has been updated a little bit to match the behavior of cucumber-ruby because cucumber-ruby already has an explicit support for empty scenarios, cucumber-js has not - and also it was far more easy to do so due to how acceptance test-data are managed in the monorepo at the moment. Also, I suggest to merge that PR not with the idea to impose the actual behavior neither to enforce the same behavior on all implementation, but to have some explicit spec for what is already happening now within the mono-repo. |
The CCK is a canonical representation of how cucumber should work. Currently there is no agreement between implementations, currently there is also no consensus on what the correct behavior should be. And I don't think a decision like this should be made without. However adding a scenario to the cck, even without changing any implementation suggests that there is a correct behavior. Especially considering that in cucumber/common#1498 you say:
So I don't really understand what is meant by:
This seems to contravene the intended purpose of the CCK. I am guessing there may be an underlying motivation that is rooted in the dual use of the CCK as a test set for the html formatter which may need an empty case for good measure. If this is the case it doesn't appear to have been stated. And while I appreciate that you are being very diplomatic about the whole thing, logically either the CCK is no longer a CCK or we made a decision about the correct behavior of Cucumber without consensus. |
Yes, that makes sense, actually 👍
I mean that no runtime code has been changed. Messages, Gherkin, no module in the monorepo that is then embedded into an implementation as runtime code is impacted by the PR
When I started working on that issue to move-on with it as it has been opened years ago, I did not realize there would be such a debate 😓 The CCK seemed to be a good starting point. If the CCK intent is to specify the behavior of cucumber across implementation with no exception, given the discussion around empty scenarios, indeed it is actually not a good starting point 😅 Past week I asked for the review because empty scenarios are responsible for an issue in cucumber-js (cucumber/cucumber-js#1668). Users are badly impacted by the non-support for that behavior. So I wanted to move-on in order to be able to fix the issue. As I already mention: I think we may consider the possibility to have non-standard behavior depending implementations - based on the implementation ecosystem. I am not sure we may have a consensus on all and every topics like this one. As far as it does not impact the mono-repo, I think we should let us the possibility to have some explicit divergences over our implementations |
@aurelien-reeves FWIW I'm not convinced it's causing that serious a cucumber-js issue (see my latest comment there). Still it would be great to somehow find a way forward with this! |
Would it be possible to add "individual" test cases to the formatters? That way we don't have to abuse the CCK because we want to test a few edge cases? |
That sounds interesting 👍 On the other hand, "abusing" the CCK - but properly - would keep all our spec - even the one that diverge based on the implementation - at the same place. I am still confused with the CCK. Do we consider it as
I know you said that it is a "canonical representation of how cucumber should work". But I am not sure to understand what do you mean here - and it would be the same with the french translation of that sentence: I am not sure what "canonical" means 😅 |
Just ran into this problem again while working on cucumber/common#871. The JUnit XML format works with |
Summary
Currently, the Gherkin pickles compiler removes Gherkin scenarios that don't have any steps, so they don't appear as test cases.
I understand why this was done - what's the point in running a test case that isn't going to test anything? - yet I think we're missing a thing here.
When I watch teams do BDD, they very often start out by creating just the headline or title of a scenario, and defer filling out the Given/When/Then steps until later. I'd like it if Cucumber were reporting that scenario's status as
undefined
. In order to do this, we'd need to pass a pickle out of the compiler.Expected Behavior
Pickles compiler outputs pickles (with no steps) for scenarios with no steps.
Current Behavior
Pickles compiler does not output a pickle for scenarios with no steps.
Possible Solution
I'd love some pointers here. I've never worked on the pickles compiler, but if someone can show me where to start (and we're agreed on the idea), I'd have a go at this.
Context & Motivation
I'm thinking about how people like product owners engage with Cucumber, and that I'd guess they think of a scenario as having taken life as soon as they name it. It seems wrong to hide those scenarios from Cucumber.
The text was updated successfully, but these errors were encountered: