Proposal: Unambiguous Keywords

[badeball]

Proposal Summary / Motivation

I've briefly presented this idea on Slack and in the spirit of gathering more feedback I'm posting it here as well.

The Gherkin reference specifically stipulates that keywords should not be taken into account when looking for a step definition [1].

I've personally witnessed in several projects that this can lead to deterioration in test language unless every change is strictly reviewed.

Below is an example of a hastily written patch that breaks the flow of language, but doesn't yield an error (actually taken from my Git history, but modified slightly to anonymize the domain).

       When I press the foobar button
-      Then I should see 2 events in total
       And I should see these new fields with values
         | My Field      | foo |
         | Another Field | bar |

[1] https://cucumber.io/docs/gherkin/reference/#steps

Current Behavior

Currently, the following steps would yield ambiguity error.

• Given(“I press button”)
• When(“I press button”)

I don't propose changing this.

However, it doesn't matter which keyword you use in your feature files and the example below runs just fine.

# features/step_definitions/cucumber_steps.js
import {When} from 'cucumber'

When(/^a step$/, function() {});

# features/a.feature
Feature: a feature name
  Scenario: a scenario name
    Given a step

Proposed Behavior

Given the following step definitions.

When("step a", () => {});

... and the following feature file.

Feature: a feature name
  Scenario: a scenario name
    Given a step

I propose for Cucumber to fail with a wrong-keyword error. Furthermore, and just as importantly, for this to properly work, any use of And and But keyword must be interpreted as if they were of the same type as the previous step.

I've tried to outline this behavior with some test cases here.

[aslakhellesoy]

I like this proposal because it encourages even less ambiguity. You can no longer mix and match Given/When/Then as you see fit.

[luke-hill]

The only thing I propose to this is we do a major update to all our APIs with this and only this, because I can see this either being reported 20 times or causing some grievances.

That being said, I do like it :)

[mpkorstanje]

I feel like this should be in the realm of a linters rather then in the realm of the interpreters.

[aslakhellesoy]

@mpkorstanje that linter would require access to more than the gherkin document - it would also need the step definitions. I think that’s too heavy for a linter. Plus, people don’t use linters.

If we add this we could always add an option to disable it, so people with years of legacy scenarios can go on with their lives.

Interested in your perspective @mattwynne @sebrose

[badeball]

I guess this would necessarily require a change in the messages, which only contains the difficult-to-interpret synonym used in tests.

[ehuelsmann]

I very much like this proposal. It also mirrors the behaviour that has been implemented in Perl's implementation (pherkin/test-bdd-cucumber-perl) since before 2013.

[mattwynne]

I like this proposal too.

From my point of view, it's not just about language clashes, it's also about wanting to be able to do different technical stuff during context, action and outcome phases of a scenario. For example, if I'm using some kind of driver adapter to connect my steps to my app, I would like to be able to use the domain-driver during Given steps, then switch to a UI driver during the When and Then steps.

It would be super-awesome if there were messages (that front-ends could them build hooks for) that told us when we were moving from context into action, from action into outcome phases of the scenario. But keep that out of scope for now!

[davidjgoss]

I'm a fan of this. The example in the original summary is exactly the kind of thing I have seen happening in practise.

The only thing I propose to this is we do a major update to all our APIs with this and only this, because I can see this either being reported 20 times or causing some grievances.

It could work like a deprecation - ship the change whenever, have it raise a prominent warning at runtime initially but not fail (unless you opt in early), then move to hard failure in a major release after X months?

Another minor facet: cucumber-js currently exposes the generic defineStep function (that the keywords delegate to) in its API so removing that would be a prerequisite there I guess.

[aslakhellesoy]

I love your thinking here @mattwynne - about separating the context/action/outcome scopes, and giving the user control over how they are created. It's something @joshski and I have been discussing regularly since we paired on the cucumber electron subscond tdd code a couple of years ago. We ended up implementing this pattern ourselves, but it would be much cleaner if it were part of Cucumber itself.

[aslakhellesoy]

@davidjgoss - regarding removing defineStep we have an open ticket for that - which has caused some grievances. cucumber/cucumber-ruby#1362 - I'd be interested in your thoughts in that ticket.

[davidjgoss]

@aslakhellesoy I've looked through that thread but struggling to see the overlap - defineStep isn't a subroutines thing, it's just that it exposes a way to do a step definition without specifying which of "Given", "When" or "Then" it is - which would be at odds with this change.

[laeubi]

I also think a warning would be suitable as a first step, with an option to turn warning into error.
Maybe a commandline option like -strict could be added, or we adopt something similar how gcc and other compilers work by adding +/-W switches.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[stale]

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

[charlierudolph]

Really like this idea. Want to recap to check my understand and see if we can determining next steps

• With step definition: Given("x", ...)
  • Use in a feature file
    • Given x -> matches step definition
    • When x -> no match, wrong keyword
    • Then x -> no match, wrong keyword
    • And x / But x -> keyword need to be transformed to last usage of Given / When / Then
• Multiple step definitions Given("x", ...) and Given("x", ...) lead to ambiguous result

Questions:

• Are multiple step definitions Given("x", ...) and When("x", ...) allowed?
  • These stop leading to ambiguous results and could technically be allowed after keywords are always unambiguous

Implementation Plan

• Core changes
  • gherkin - pickle steps include a keyword type that is computed to be Given / When / Then (or context / action / outcome)
  • add example to compatibility kit
• Language Libraries
  • Initial release
    • Warning generic step definitions will be removed (those without keyword type)
    • Warning on wrong keyword type
    • Opt-in flag to unambiguous keywords
  • Major release for unambiguous keywords only

Thoughts?

[davidjgoss]

@charlierudolph sounds like a plan to me.

Are multiple step definitions Given("x", ...) and When("x", ...) allowed?

Still no IMO, because fundamentally Given (context), When (action) and Then (verification) should be different stuff.

There is also the * keyword which is potentially tricky with this.

[enkessler]

• Are multiple step definitions Given("x", ...) and When("x", ...) allowed?

Presumably, yes. If the fundamental complaint is that Given("x", ...) and When("x", ...) are being treated the same and the goal of this change is to treat them as different, then I would expect that both of those definitions would be allowed to co-exist because, you know, they are different.

[luke-hill]

Dependent on work involved, I would say maybe to start with do the smallest change (That would in theory permit both Given("x") and When("x"), and then maybe later set it up to prevent them. But I'm only spitballing at the moment.

Also shared this link with slack as we were discussing it recently.

[marnen]

I would prefer that this be a configurable option. I do occasionally reuse the same steps between Given and When, and I don't really want to have to write the same step definitions twice if I can help it. At the same time, I understand that some people see value in matching the keyword, and I don't want to deny them that either.

[ehuelsmann]

I'll be joining the stand-up coming Thursday. I'm keen to understand what will be required to bring this forward, because it's one of the last obstacles for Test::BDD::Cucumber to move to using the Gherkin library.

[ehuelsmann]

In response to @ciaranmcnulty :

If this goes ahead we'll need to consider / test the impact on i18n

Will it add to my experience to have to remember to use Given() whenever I write Gegewe in my steps?

That depends: in the Perl Cucumber variant that I maintain, translations for Given/When/Then are registered for the step files to be available (documentation: https://metacpan.org/pod/Test::BDD::Cucumber::Manual::Tutorial#Non-English-features-and-step-files). No idea how other implementations would (be able to) deal with this though.

[sebrose]

I really don't like this suggestion:

• IMO it encourages ambiguous business domain language, with automation code modified by preceding keyword
• Gherkin currently supports steps being preceded by '*'
• when conjunctions are used (And, But) the reader will have to identify the preceding G/W/T keyword

However, a number of Gherkin implementations do work in this way (SpecFlow, Perl, Behat?). SpecFlow introduced the @StepDefinition annotation to emulate core Cucumber implementations.

I support the passing of the keyword to the pickle.

[mpkorstanje]

I also support passing of the keyword to the pickle.

But I do not support making the keyword part of the step definition. Major breaking change with no value.

[ehuelsmann]

Discussed on the weekly Cucumber team call; consensus was that since some implementations have had the step-definition-per-keyword behavior (mentioned during the meeting: SpecFlow and Test::BDD::Cucumber (Perl)), it's necessary to add the keywords to the pickles. Additionally, since Cucumber never exhibited this behavior, it's expected to be a major breaking change for all those who have Cucumber-based test suites. In other words: Let's do this in the pickle so more projects can move to the generic parser infrastructure, but lets not incorporate this any further than that.

[ehuelsmann]

It's my action to implement this in the Gherkin (and Messages) libraries up to the Pickle (for as many languages as I can get).

[ehuelsmann]

@aslakhellesoy , @mattwynne , @aurelien-reeves , @mpkorstanje Now that #1741 has been merged, what further work is left in this issue, in your opinions?

[aslakhellesoy]

I can't think of anything left to do other than starting to integrate this in to Cucumbers.

They should all have an option to turn on the strict validation (matching against expression and keyword). I suggest we call this done for now and deal with any problems as we start integrating.

[mattwynne]

I agree Aslak.

So that we can keep track of the ongoing work to implement this and get it in front of users, I've used the new Github Projects beta to create a cross-org project:

https://github.com/orgs/cucumber/projects/11/views/1

I've created an issue in each of the trackers for the JavaScript, Ruby, Java and Go implementations of Cucumber. If @jenisys @gasparnagy @ciaranmcnulty want to add their own issues to that project too, feel free 😄

[ciaranmcnulty]

PHP will take a while (it requires Behat to actually use the cucumber parser, first)

[ehuelsmann]

Perl already behaves like this, however, in order to use this work, we need to start using the cucumber parser.

[gasparnagy]

SpecFlow already behaves like that, so we are done.