[JUnit] Fix swallowed exception (#2714)

Author: mpkorstanje

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - [Pico] Improve performance ([#2724](https://github.com/cucumber/cucumber-jvm/issues/2724) Julien Kronegg)
+- [JUnit 4] Fix swallowed exception ([#2714](https://github.com/cucumber/cucumber-jvm/issues/2714) M.P. Korstanje)
 
 ## [7.11.2] - 2023-03-23
 ### Fixed

cucumber-core/src/main/java/io/cucumber/core/runtime/CucumberExecutionContext.java

@@ -25,6 +25,8 @@
 
 import static io.cucumber.cienvironment.DetectCiEnvironment.detectCiEnvironment;
 import static io.cucumber.core.exception.ExceptionUtils.printStackTrace;
+import static io.cucumber.core.exception.ExceptionUtils.throwAsUncheckedException;
+import static io.cucumber.core.exception.UnrecoverableExceptions.rethrowIfUnrecoverable;
 import static io.cucumber.messages.Convertor.toMessage;
 import static java.util.Collections.singletonList;
 
@@ -46,6 +48,11 @@ public CucumberExecutionContext(EventBus bus, ExitStatus exitStatus, RunnerSuppl
         this.runnerSupplier = runnerSupplier;
     }
 
+    @FunctionalInterface
+    public interface ThrowingRunnable {
+        void run() throws Throwable;
+    }
+
     public void startTestRun() {
         emitMeta();
         emitTestRunStarted();
@@ -134,4 +141,30 @@ private Runner getRunner() {
         return collector.executeAndThrow(runnerSupplier::get);
     }
 
+    public void runFeatures(ThrowingRunnable executeFeatures) {
+        startTestRun();
+        execute(() -> {
+            runBeforeAllHooks();
+            executeFeatures.run();
+        });
+        try {
+            execute(this::runAfterAllHooks);
+        } finally {
+            finishTestRun();
+        }
+        Throwable throwable = getThrowable();
+        if (throwable != null) {
+            throwAsUncheckedException(throwable);
+        }
+    }
+
+    private void execute(ThrowingRunnable runnable) {
+        try {
+            runnable.run();
+        } catch (Throwable t) {
+            // Collected in CucumberExecutionContext
+            rethrowIfUnrecoverable(t);
+        }
+    }
+
 }

cucumber-core/src/main/java/io/cucumber/core/runtime/Runtime.java

@@ -29,8 +29,6 @@
 import java.util.function.Predicate;
 import java.util.function.Supplier;
 
-import static io.cucumber.core.exception.ExceptionUtils.throwAsUncheckedException;
-import static io.cucumber.core.exception.UnrecoverableExceptions.rethrowIfUnrecoverable;
 import static io.cucumber.core.runtime.SynchronizedEventBus.synchronize;
 import static java.util.Collections.emptyList;
 import static java.util.stream.Collectors.collectingAndThen;
@@ -77,29 +75,7 @@ public static Builder builder() {
     public void run() {
         // Parse the features early. Don't proceed when there are lexer errors
         List<Feature> features = featureSupplier.get();
-        context.startTestRun();
-        execute(() -> {
-            context.runBeforeAllHooks();
-            runFeatures(features);
-        });
-        try {
-            execute(context::runAfterAllHooks);
-        } finally {
-            context.finishTestRun();
-        }
-        Throwable exception = context.getThrowable();
-        if (exception != null) {
-            throwAsUncheckedException(exception);
-        }
-    }
-
-    private void execute(Runnable runnable) {
-        try {
-            runnable.run();
-        } catch (Throwable t) {
-            // Collected in CucumberExecutionContext
-            rethrowIfUnrecoverable(t);
-        }
+        context.runFeatures(() -> runFeatures(features));
     }
 
     private void runFeatures(List<Feature> features) {

cucumber-core/src/test/java/io/cucumber/core/runtime/RuntimeTest.java

@@ -1,13 +1,16 @@
 package io.cucumber.core.runtime;
 
+import io.cucumber.core.backend.CucumberBackendException;
 import io.cucumber.core.backend.Glue;
 import io.cucumber.core.backend.HookDefinition;
 import io.cucumber.core.backend.ParameterInfo;
 import io.cucumber.core.backend.ScenarioScoped;
+import io.cucumber.core.backend.StaticHookDefinition;
 import io.cucumber.core.backend.StubStepDefinition;
 import io.cucumber.core.backend.TestCaseState;
 import io.cucumber.core.eventbus.EventBus;
 import io.cucumber.core.exception.CompositeCucumberException;
+import io.cucumber.core.exception.CucumberException;
 import io.cucumber.core.feature.TestFeatureParser;
 import io.cucumber.core.gherkin.Feature;
 import io.cucumber.core.gherkin.FeatureParserException;
@@ -31,7 +34,6 @@
 import io.cucumber.plugin.event.TestRunStarted;
 import io.cucumber.plugin.event.TestStepFinished;
 import io.cucumber.plugin.event.TestStepStarted;
-import org.hamcrest.CoreMatchers;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
 import org.mockito.ArgumentCaptor;
@@ -401,6 +403,30 @@ void should_fail_on_event_listener_exception_at_test_run_finished() {
         assertThat(actualThrown, equalTo(expectedException));
     }
 
+    @Test
+    void should_fail_on_exception_invoking_after_all_hook() {
+        RuntimeException expectedException = new RuntimeException("This exception is expected");
+        CucumberBackendException backendException = new CucumberBackendException("failed", expectedException);
+        MockedStaticHookDefinition mockedStaticHookDefinition = new MockedStaticHookDefinition(() -> {
+            throw backendException;
+        });
+
+        BackendSupplier backendSupplier = new TestBackendSupplier() {
+            @Override
+            public void loadGlue(Glue glue, List<URI> gluePaths) {
+                glue.addAfterAllHook(mockedStaticHookDefinition);
+            }
+        };
+
+        Executable testMethod = () -> Runtime.builder()
+                .withFeatureSupplier(new StubFeatureSupplier())
+                .withBackendSupplier(backendSupplier)
+                .build()
+                .run();
+        CucumberException actualThrown = assertThrows(CucumberException.class, testMethod);
+        assertThat(actualThrown.getCause(), equalTo(backendException));
+    }
+
     @Test
     void should_interrupt_waiting_plugins() throws InterruptedException {
         final Feature feature1 = TestFeatureParser.parse("path/test.feature", "" +
@@ -574,6 +600,35 @@ public String getLocation() {
 
     }
 
+    private static class MockedStaticHookDefinition implements StaticHookDefinition {
+
+        private final Runnable runnable;
+
+        private MockedStaticHookDefinition(Runnable runnable) {
+            this.runnable = runnable;
+        }
+
+        @Override
+        public void execute() {
+            runnable.run();
+        }
+
+        @Override
+        public int getOrder() {
+            return 0;
+        }
+
+        @Override
+        public boolean isDefinedAt(StackTraceElement stackTraceElement) {
+            return false;
+        }
+
+        @Override
+        public String getLocation() {
+            return "mocked hook definition definition";
+        }
+    }
+
     private static class FormatterSpy implements EventListener {
 
         private final StringBuilder calls = new StringBuilder();

cucumber-junit-platform-engine/src/main/java/io/cucumber/junit/platform/engine/CucumberEngineDescriptor.java

@@ -6,6 +6,7 @@
 import org.junit.platform.engine.support.hierarchical.Node;
 
 import java.util.Optional;
+import java.util.function.Consumer;
 
 class CucumberEngineDescriptor extends EngineDescriptor implements Node<CucumberEngineExecutionContext> {
 
@@ -15,53 +16,55 @@ class CucumberEngineDescriptor extends EngineDescriptor implements Node<Cucumber
         super(uniqueId, "Cucumber");
     }
 
-    private static void recursivelyMerge(TestDescriptor descriptor, TestDescriptor parent) {
-        Optional<? extends TestDescriptor> byUniqueId = parent.findByUniqueId(descriptor.getUniqueId());
-        if (!byUniqueId.isPresent()) {
-            parent.addChild(descriptor);
-        } else {
-            byUniqueId.ifPresent(
-                existingParent -> descriptor.getChildren()
-                        .forEach(child -> recursivelyMerge(child, existingParent)));
-        }
-    }
-
     @Override
     public CucumberEngineExecutionContext prepare(CucumberEngineExecutionContext context) {
-        if (getChildren().isEmpty()) {
-            return context;
-        }
-        context.startTestRun();
-        return context;
+        return ifChildren(context, CucumberEngineExecutionContext::startTestRun);
     }
 
     @Override
     public CucumberEngineExecutionContext before(CucumberEngineExecutionContext context) {
-        if (getChildren().isEmpty()) {
-            return context;
-        }
-        context.runBeforeAllHooks();
-        return context;
+        return ifChildren(context, CucumberEngineExecutionContext::runBeforeAllHooks);
     }
 
     @Override
     public void after(CucumberEngineExecutionContext context) {
-        if (getChildren().isEmpty()) {
-            return;
-        }
-        context.runAfterAllHooks();
+        ifChildren(context, CucumberEngineExecutionContext::runAfterAllHooks);
     }
 
     @Override
     public void cleanUp(CucumberEngineExecutionContext context) {
-        if (getChildren().isEmpty()) {
-            return;
+        ifChildren(context, CucumberEngineExecutionContext::finishTestRun);
+    }
+
+    /*
+     * Problem: The JUnit Platform will always execute all engines that
+     * participated in discovery. In combination with the JUnit Platform Suite
+     * Engine this may result in CucumberEngine being executed multiple times.
+     * To ensure Cucumber only performs works if/when there are tests to run we
+     * don't do anything unless there are tests. I.e. only when this test
+     * descriptor has children.
+     */
+    private CucumberEngineExecutionContext ifChildren(
+            CucumberEngineExecutionContext context, Consumer<CucumberEngineExecutionContext> action
+    ) {
+        if (!getChildren().isEmpty()) {
+            action.accept(context);
         }
-        context.finishTestRun();
+        return context;
     }
 
     void mergeFeature(FeatureDescriptor descriptor) {
         recursivelyMerge(descriptor, this);
     }
 
+    private static void recursivelyMerge(TestDescriptor descriptor, TestDescriptor parent) {
+        Optional<? extends TestDescriptor> byUniqueId = parent.findByUniqueId(descriptor.getUniqueId());
+        if (!byUniqueId.isPresent()) {
+            parent.addChild(descriptor);
+        } else {
+            byUniqueId.ifPresent(
+                existingParent -> descriptor.getChildren()
+                        .forEach(child -> recursivelyMerge(child, existingParent)));
+        }
+    }
 }

cucumber-junit-platform-engine/src/main/java/io/cucumber/junit/platform/engine/CucumberEngineExecutionContext.java

@@ -76,7 +76,7 @@ void startTestRun() {
         // Problem: The JUnit Platform will always execute all engines that
         // participated in discovery. In combination with the JUnit Platform
         // Suite Engine this may result in CucumberEngine being executed
-        // twice.
+        // multiple times.
         //
         // One of these instances may not have discovered any tests and would
         // write empty reports. Therefor we do not invoke 'startTestRun' if

cucumber-junit/src/main/java/io/cucumber/junit/Cucumber.java

@@ -199,13 +199,7 @@ protected void runChild(ParentRunner<?> child, RunNotifier notifier) {
     @Override
     protected Statement childrenInvoker(RunNotifier notifier) {
         Statement statement = super.childrenInvoker(notifier);
-
-        statement = new RunBeforeAllHooks(statement);
-        statement = new RunAfterAllHooks(statement);
-
-        statement = new StartTestRun(statement);
-        statement = new FinishTestRun(statement);
-
+        statement = new StartAndFinishTestRun(statement);
         return statement;
     }
 
@@ -215,75 +209,22 @@ public void setScheduler(RunnerScheduler scheduler) {
         multiThreadingAssumed = true;
     }
 
-    private class StartTestRun extends Statement {
+    private class StartAndFinishTestRun extends Statement {
         private final Statement next;
 
-        public StartTestRun(Statement next) {
+        public StartAndFinishTestRun(Statement next) {
             this.next = next;
         }
 
         @Override
-        public void evaluate() throws Throwable {
+        public void evaluate() {
             if (multiThreadingAssumed) {
                 plugins.setSerialEventBusOnEventListenerPlugins(bus);
             } else {
                 plugins.setEventBusOnEventListenerPlugins(bus);
             }
-            context.startTestRun();
-            next.evaluate();
-        }
-
-    }
-
-    private class FinishTestRun extends Statement {
-        private final Statement next;
-
-        public FinishTestRun(Statement next) {
-            this.next = next;
-        }
-
-        @Override
-        public void evaluate() throws Throwable {
-            try {
-                next.evaluate();
-            } finally {
-                context.finishTestRun();
-            }
-        }
-
-    }
-
-    private class RunBeforeAllHooks extends Statement {
-        private final Statement next;
-
-        public RunBeforeAllHooks(Statement next) {
-            this.next = next;
-        }
-
-        @Override
-        public void evaluate() throws Throwable {
-            context.runBeforeAllHooks();
-            next.evaluate();
-        }
-
-    }
-
-    private class RunAfterAllHooks extends Statement {
-        private final Statement next;
-
-        public RunAfterAllHooks(Statement next) {
-            this.next = next;
-        }
-
-        @Override
-        public void evaluate() throws Throwable {
-            try {
-                next.evaluate();
-            } finally {
-                context.runAfterAllHooks();
-            }
+            context.runFeatures(next::evaluate);
         }
-
     }
 
 }