Merge branch '2102-fix-dry-run' into main

Given a scenario with several steps:

```feature
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

Author: mpkorstanje

CHANGELOG.md

@@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
  * [Core] CucumberOptions default snippet type should not override properties ([2107](https://github.com/cucumber/cucumber-jvm/pull/2107) M.P. Korstanje)
  * [Core] Replace parentFile.makeDirs with Files.createDirectories(parentFile) ([2104](https://github.com/cucumber/cucumber-jvm/pull/2104) M.P. Korstanje)
+ * [Core] Separate run, dry-run and skip execution modes ([2102](https://github.com/cucumber/cucumber-jvm/pull/2109), [2102](https://github.com/cucumber/cucumber-jvm/pull/2109) M.P. Korstanje)
+   * Fixes `--dry-run` not failing on undefined steps
 
 ### Security
  * [Core] Update `create-meta` to 2.0.2 to avoid sharing credentials ([2110](https://github.com/cucumber/cucumber-jvm/pull/2110) vincent-psarga)

core/src/main/java/io/cucumber/core/runner/AmbiguousPickleStepDefinitionsMatch.java

@@ -22,7 +22,7 @@ public void runStep(TestCaseState state) throws AmbiguousStepDefinitionsExceptio
 
     @Override
     public void dryRunStep(TestCaseState state) throws AmbiguousStepDefinitionsException {
-        runStep(state);
+        throw exception;
     }
 
 }

core/src/main/java/io/cucumber/core/runner/ExecutionMode.java

@@ -0,0 +1,34 @@
+package io.cucumber.core.runner;
+
+import io.cucumber.plugin.event.Status;
+
+enum ExecutionMode {
+
+    RUN {
+        @Override
+        Status execute(StepDefinitionMatch stepDefinitionMatch, TestCaseState state) throws Throwable {
+            stepDefinitionMatch.runStep(state);
+            return Status.PASSED;
+        }
+
+    },
+    DRY_RUN {
+        @Override
+        Status execute(StepDefinitionMatch stepDefinitionMatch, TestCaseState state) throws Throwable {
+            stepDefinitionMatch.dryRunStep(state);
+            return Status.PASSED;
+        }
+    },
+    SKIP {
+        @Override
+        Status execute(StepDefinitionMatch stepDefinitionMatch, TestCaseState state) {
+            return Status.SKIPPED;
+        }
+    };
+
+    abstract Status execute(StepDefinitionMatch stepDefinitionMatch, TestCaseState state) throws Throwable;
+
+    ExecutionMode next(ExecutionMode current) {
+        return current == SKIP ? current : this;
+    }
+}

core/src/main/java/io/cucumber/core/runner/PickleStepTestStep.java

@@ -39,20 +39,25 @@ final class PickleStepTestStep extends TestStep implements io.cucumber.plugin.ev
     }
 
     @Override
-    boolean run(TestCase testCase, EventBus bus, TestCaseState state, boolean skipSteps) {
-        boolean skipNextStep = skipSteps;
+    ExecutionMode run(TestCase testCase, EventBus bus, TestCaseState state, ExecutionMode executionMode) {
+        ExecutionMode nextExecutionMode = executionMode;
 
         for (HookTestStep before : beforeStepHookSteps) {
-            skipNextStep |= before.run(testCase, bus, state, skipSteps);
+            nextExecutionMode = before
+                    .run(testCase, bus, state, executionMode)
+                    .next(nextExecutionMode);
         }
 
-        skipNextStep |= super.run(testCase, bus, state, skipNextStep);
+        nextExecutionMode = super.run(testCase, bus, state, nextExecutionMode)
+                .next(nextExecutionMode);
 
         for (HookTestStep after : afterStepHookSteps) {
-            skipNextStep |= after.run(testCase, bus, state, skipSteps);
+            nextExecutionMode = after
+                    .run(testCase, bus, state, executionMode)
+                    .next(nextExecutionMode);
         }
 
-        return skipNextStep;
+        return nextExecutionMode;
     }
 
     List<HookTestStep> getBeforeStepHookSteps() {

core/src/main/java/io/cucumber/core/runner/TestCase.java

@@ -22,6 +22,8 @@
 import java.util.List;
 import java.util.UUID;
 
+import static io.cucumber.core.runner.ExecutionMode.DRY_RUN;
+import static io.cucumber.core.runner.ExecutionMode.RUN;
 import static io.cucumber.core.runner.TestStepResultStatus.from;
 import static io.cucumber.messages.TimeConversion.javaDurationToDuration;
 import static io.cucumber.messages.TimeConversion.javaInstantToTimestamp;
@@ -32,7 +34,7 @@ final class TestCase implements io.cucumber.plugin.event.TestCase {
 
     private final Pickle pickle;
     private final List<PickleStepTestStep> testSteps;
-    private final boolean dryRun;
+    private final ExecutionMode executionMode;
     private final List<HookTestStep> beforeHooks;
     private final List<HookTestStep> afterHooks;
     private final UUID id;
@@ -49,7 +51,7 @@ final class TestCase implements io.cucumber.plugin.event.TestCase {
         this.beforeHooks = beforeHooks;
         this.afterHooks = afterHooks;
         this.pickle = pickle;
-        this.dryRun = dryRun;
+        this.executionMode = dryRun ? DRY_RUN : RUN;
     }
 
     private static StepMatchArgument.Group makeMessageGroup(
@@ -82,7 +84,7 @@ private static String toString(Throwable error) {
     }
 
     void run(EventBus bus) {
-        boolean skipNextStep = this.dryRun;
+        ExecutionMode nextExecutionMode = this.executionMode;
         emitTestCaseMessage(bus);
 
         Instant start = bus.getInstant();
@@ -92,15 +94,21 @@ void run(EventBus bus) {
         TestCaseState state = new TestCaseState(bus, executionId, this);
 
         for (HookTestStep before : beforeHooks) {
-            skipNextStep |= before.run(this, bus, state, dryRun);
+            nextExecutionMode = before
+                    .run(this, bus, state, executionMode)
+                    .next(nextExecutionMode);
         }
 
         for (PickleStepTestStep step : testSteps) {
-            skipNextStep |= step.run(this, bus, state, skipNextStep);
+            nextExecutionMode = step
+                    .run(this, bus, state, nextExecutionMode)
+                    .next(nextExecutionMode);
         }
 
         for (HookTestStep after : afterHooks) {
-            after.run(this, bus, state, dryRun);
+            nextExecutionMode = after
+                    .run(this, bus, state, executionMode)
+                    .next(nextExecutionMode);
         }
 
         Instant stop = bus.getInstant();

core/src/main/java/io/cucumber/core/runner/TestStep.java

@@ -17,6 +17,7 @@
 import java.util.Arrays;
 import java.util.UUID;
 
+import static io.cucumber.core.runner.ExecutionMode.SKIP;
 import static io.cucumber.core.runner.TestStepResultStatus.from;
 import static io.cucumber.messages.TimeConversion.javaDurationToDuration;
 import static io.cucumber.messages.TimeConversion.javaInstantToTimestamp;
@@ -53,14 +54,14 @@ public UUID getId() {
         return id;
     }
 
-    boolean run(TestCase testCase, EventBus bus, TestCaseState state, boolean skipSteps) {
+    ExecutionMode run(TestCase testCase, EventBus bus, TestCaseState state, ExecutionMode executionMode) {
         Instant startTime = bus.getInstant();
         emitTestStepStarted(testCase, bus, state.getTestExecutionId(), startTime);
 
         Status status;
         Throwable error = null;
         try {
-            status = executeStep(state, skipSteps);
+            status = executeStep(state, executionMode);
         } catch (Throwable t) {
             error = t;
             status = mapThrowableToStatus(t);
@@ -72,7 +73,7 @@ boolean run(TestCase testCase, EventBus bus, TestCaseState state, boolean skipSt
 
         emitTestStepFinished(testCase, bus, state.getTestExecutionId(), stopTime, duration, result);
 
-        return !result.getStatus().is(Status.PASSED);
+        return result.getStatus().is(Status.PASSED) ? executionMode : SKIP;
     }
 
     private void emitTestStepStarted(TestCase testCase, EventBus bus, UUID textExecutionId, Instant startTime) {
@@ -85,16 +86,10 @@ private void emitTestStepStarted(TestCase testCase, EventBus bus, UUID textExecu
                 .build());
     }
 
-    private Status executeStep(TestCaseState state, boolean skipSteps) throws Throwable {
+    private Status executeStep(TestCaseState state, ExecutionMode executionMode) throws Throwable {
         state.setCurrentTestStepId(id);
         try {
-            if (!skipSteps) {
-                stepDefinitionMatch.runStep(state);
-                return Status.PASSED;
-            } else {
-                stepDefinitionMatch.dryRunStep(state);
-                return Status.SKIPPED;
-            }
+            return executionMode.execute(stepDefinitionMatch, state);
         } finally {
             state.clearCurrentTestStepId();
         }

core/src/main/java/io/cucumber/core/runner/UndefinedPickleStepDefinitionMatch.java

@@ -19,7 +19,7 @@ public void runStep(TestCaseState state) {
 
     @Override
     public void dryRunStep(TestCaseState state) {
-
+        throw new UndefinedStepDefinitionException();
     }
 
 }

core/src/test/java/io/cucumber/core/backend/StubHookDefinition.java

@@ -0,0 +1,68 @@
+package io.cucumber.core.backend;
+
+import java.util.function.Consumer;
+
+public class StubHookDefinition implements HookDefinition {
+
+    private static final String STUBBED_LOCATION_WITH_DETAILS = "{stubbed location with details}";
+    private final String location;
+    private final RuntimeException exception;
+    private final Consumer<TestCaseState> action;
+
+    public StubHookDefinition(String location, RuntimeException exception, Consumer<TestCaseState> action) {
+        this.location = location;
+        this.exception = exception;
+        this.action = action;
+    }
+
+    public StubHookDefinition(String location, Consumer<TestCaseState> action) {
+        this(location, null, action);
+    }
+
+    public StubHookDefinition() {
+        this(STUBBED_LOCATION_WITH_DETAILS, null, null);
+    }
+
+    public StubHookDefinition(Consumer<TestCaseState> action) {
+        this(STUBBED_LOCATION_WITH_DETAILS, null, action);
+    }
+
+    public StubHookDefinition(RuntimeException exception) {
+        this(STUBBED_LOCATION_WITH_DETAILS, exception, null);
+    }
+
+    public StubHookDefinition(String location) {
+        this(location, null, null);
+    }
+
+    @Override
+    public void execute(TestCaseState state) {
+        if (action != null) {
+            action.accept(state);
+        }
+        if (exception != null) {
+            throw exception;
+        }
+    }
+
+    @Override
+    public String getTagExpression() {
+        return "";
+    }
+
+    @Override
+    public int getOrder() {
+        return 0;
+    }
+
+    @Override
+    public boolean isDefinedAt(StackTraceElement stackTraceElement) {
+        return false;
+    }
+
+    @Override
+    public String getLocation() {
+        return location;
+    }
+
+}

core/src/test/java/io/cucumber/core/backend/StubPendingException.java

@@ -0,0 +1,16 @@
+package io.cucumber.core.backend;
+
+import io.cucumber.core.backend.Pending;
+
+@Pending
+public final class StubPendingException extends RuntimeException {
+
+    public StubPendingException() {
+        this("TODO: implement me");
+    }
+
+    public StubPendingException(String message) {
+        super(message);
+    }
+
+}

core/src/test/java/io/cucumber/core/runtime/StubStepDefinition.java renamed to core/src/test/java/io/cucumber/core/backend/StubStepDefinition.java

@@ -1,8 +1,4 @@
-package io.cucumber.core.runtime;
-
-import io.cucumber.core.backend.ParameterInfo;
-import io.cucumber.core.backend.StepDefinition;
-import io.cucumber.core.backend.TypeResolver;
+package io.cucumber.core.backend;
 
 import java.lang.reflect.Type;
 import java.util.List;
@@ -13,17 +9,28 @@
 
 public class StubStepDefinition implements StepDefinition {
 
+    private static final String STUBBED_LOCATION_WITH_DETAILS = "{stubbed location with details}";
     private final List<ParameterInfo> parameterInfos;
     private final String expression;
     private final RuntimeException exception;
+    private final String location;
+
+    public StubStepDefinition(String pattern, String location, Type... types) {
+        this(pattern, location, null, types);
+    }
 
     public StubStepDefinition(String pattern, Type... types) {
-        this(pattern, null, types);
+        this(pattern, STUBBED_LOCATION_WITH_DETAILS, null, types);
     }
 
     public StubStepDefinition(String pattern, RuntimeException exception, Type... types) {
+        this(pattern, STUBBED_LOCATION_WITH_DETAILS, exception, types);
+    }
+
+    public StubStepDefinition(String pattern, String location, RuntimeException exception, Type... types) {
         this.parameterInfos = Stream.of(types).map(StubParameterInfo::new).collect(Collectors.toList());
         this.expression = pattern;
+        this.location = location;
         this.exception = exception;
     }
 
@@ -34,7 +41,7 @@ public boolean isDefinedAt(StackTraceElement stackTraceElement) {
 
     @Override
     public String getLocation() {
-        return "{stubbed location with details}";
+        return location;
     }
 
     @Override

core/src/test/java/io/cucumber/core/feature/TestFeatureParser.java

@@ -6,9 +6,10 @@
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.net.URI;
-import java.nio.charset.StandardCharsets;
 import java.util.UUID;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+
 public class TestFeatureParser {
 
     public static Feature parse(final String source) {
@@ -20,6 +21,14 @@ public static Feature parse(final String uri, final String source) {
     }
 
     public static Feature parse(final URI uri, final String source) {
+        return parse(uri, new ByteArrayInputStream(source.getBytes(UTF_8)));
+    }
+
+    public static Feature parse(final String uri, final InputStream source) {
+        return parse(FeatureIdentifier.parse(uri), source);
+    }
+
+    public static Feature parse(final URI uri, final InputStream source) {
         return new FeatureParser(UUID::randomUUID).parseResource(new Resource() {
             @Override
             public URI getUri() {
@@ -28,7 +37,7 @@ public URI getUri() {
 
             @Override
             public InputStream getInputStream() {
-                return new ByteArrayInputStream(source.getBytes(StandardCharsets.UTF_8));
+                return source;
             }
 
         }).orElse(null);