[Core] Separate run, dry-run and skip execution modes

[mpkorstanje]

Given a scenario with several steps:

Scenario: 
 Given a passed step
 And a skipped step
 And an pending step
 And an undefined step
 And an ambiguous step

When executing this the outcome of the result is:

a passed step -> PASSED
a skipped step -> SKIPPED
an pending step -> PENDING
an undefined step -> SKIPPED
an ambiguous step -> AMBIGUOUS

This is wrong, because after the first non-passed result all other steps should be skipped. So:

a passed step -> PASSED
a skipped step -> SKIPPED
an pending step -> SKIPPED
an undefined step -> SKIPPED
an ambiguous step -> SKIPPED

When using executing this scenario with --dry-run the result is:

a passed step -> PASSED
a skipped step -> PASSED
an pending step -> PASSED
an undefined step -> UNDEFINED
an ambiguous step -> AMBIGUOUS

And surprisingly enough this is also wrong. While the skipped and pending states can only be detected by executing an implemented step, the undefined step should be marked as undefined and the ambiguous step that follows it should be skipped.

a passed step -> PASSED
a skipped step -> PASSED
an pending step -> PASSED
an undefined step -> UNDEFINED
an ambiguous step -> SKIPPED

The cause for this confusion lies in the fact that --dry-run was implemented using the skip mechanism rather then its own execution mode that is distinct from both a regular run and skip mode. By implementing these as individual execution modes we can avoid this confusion.

Implementing this however revealed that our formatters were often being tested with completely undefined scenarios. This does not provide a representative test and allowed #2102 to come into existence. Fixing this was rather complicated, the formatters were being tested with an overly complicated mock implementation. Replacing this mock implementation with stubs made the tests more readable and removed a significant chunk of complexity.

Fixes: #2102

[coveralls]

Coverage increased (+0.3%) to 86.48% when pulling 16e7a33 on 2102-fix-dry-run into b4ed58f on main.

[aslakhellesoy]

This sounds like nice cleanup!

I still think there is room for improvement.

The UNDEFINED and AMBIGUOUS statuses can be identified without execution. They're just a special kind of SKIPPED with more information about why (0 matches or >=2 matches). Marking a step that is UNDEFINED or AMBIGUOUS as SKIPPED weakens the signal about why a step was skipped.

Here is an example. I've modified your example a little since a skipped step doesn't make sense to me - users can't skip steps.

# Execution
a matched step -> PASSED
a matched throwing step -> FAILED
a matched step -> SKIPPED
a matched pending step -> SKIPPED
an undefined step -> UNDEFINED
an ambiguous step -> AMBIGUOUS

# Dry Run
a matched step -> SKIPPED
a matched throwing step -> SKIPPED
a matched step -> SKIPPED
a matched pending step -> SKIPPED
an undefined step -> UNDEFINED
an ambiguous step -> AMBIGUOUS

WDYT @mpkorstanje?

[mpkorstanje]

It would be more accurate to say that a user aborts the scenario. This is a common feature in JUnit and TestNG and Cucumber supports several exceptions. Because Cucumber doesn't have an aborted state these are all converted to the skipped state.

https://github.com/cucumber/cucumber-jvm/blob/main/core/src/main/java/io/cucumber/core/runner/TestStep.java#L28

While it is nice that we can inform the user that there were multiple problems. The primary reason for skipping all steps after the first non-passing step is that it simplifies interpreting Cucumbers results.

• I would expect that the result of scenario is the most severe results of the steps in that scenario.
• I would also expect that the first non-passing step determines the result of the scenario.

By skipping all steps after the first non-passing step both expectations hold.

Additionally this also makes it much easier to integrate with JUnit and TestNG. Both only support one exception per test. Having additional states may make people wonder why JUnit marked the test as skipped but Cucumber reports undefined steps.

And in practice, Cucumber reports all undefined steps with the first exception because we collect the SnippetSuggested event.

One improvement I would consider though is also emitting ambiguous step events. That way we can report all ambiguous and undefined steps in a single exception and we don't have to throw the AmbiguousStepDefinitionsException from the AmbiguousPickleStepDefinitionsMatch.

[mpkorstanje]

If you think this makes sense it might be good to create a new feature request where we can hash out the details. Otherwise lets talk about it in the regular call.

[mpkorstanje]

Note to self:

We can avoid throwing UndefinedStepDefinitionException and AmbiguousStepDefinitionsException by making the runStep and dryRunStep return the state rather then ExecutionMode always returning pass. This avoids the use of exceptions as a control flow mechanism. Currently the exceptions are mapped back to state again in TestStep.mapThrowableToStatus which is redundant.