From 62c433c14ce9d70fa5b82ef385a6e15c22cc0003 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sat, 25 Jun 2016 21:29:20 +0200 Subject: [PATCH] Ensure AfterAlls are invoked if exception is thrown by a BeforeAll - BeforeAll: @BeforeAll method or BeforeAllCallback - AfterAll: @AfterAll method or AfterAllCallback Prior to this commit, if an exception was thrown by a BeforeAll, then AfterAlls were never invoked, which is in strict contrast to the semantics of JUnit 4 and other frameworks. This commit fixes this problem by ensuring that AfterAlls (for the appropriate nesting level) are invoked even if a BeforeAll throws an exception. Fixes: #359 --- .../descriptor/ClassTestDescriptor.java | 39 ++++++--- .../JupiterEngineExecutionContext.java | 18 +++- .../extension/BeforeAndAfterAllTests.java | 87 ++++++++++++++++++- .../extension/BeforeAndAfterEachTests.java | 4 +- .../HierarchicalTestExecutor.java | 26 +++--- .../HierarchicalTestExecutorTests.java | 4 +- 6 files changed, 149 insertions(+), 29 deletions(-) diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java index 755fa6cc64b..b0affe44ef6 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java @@ -156,9 +156,14 @@ public SkipResult shouldBeSkipped(JupiterEngineExecutionContext context) throws public JupiterEngineExecutionContext before(JupiterEngineExecutionContext context) throws Exception { ExtensionRegistry registry = context.getExtensionRegistry(); ContainerExtensionContext extensionContext = (ContainerExtensionContext) context.getExtensionContext(); + ThrowableCollector throwableCollector = context.getThrowableCollector(); - invokeBeforeAllCallbacks(registry, extensionContext); - invokeBeforeAllMethods(registry, extensionContext); + invokeBeforeAllCallbacks(registry, extensionContext, throwableCollector); + if (throwableCollector.isEmpty()) { + context.beforeAllMethodsExecuted(true); + invokeBeforeAllMethods(registry, extensionContext, throwableCollector); + } + throwableCollector.assertEmpty(); return context; } @@ -167,9 +172,11 @@ public JupiterEngineExecutionContext before(JupiterEngineExecutionContext contex public void after(JupiterEngineExecutionContext context) throws Exception { ExtensionRegistry registry = context.getExtensionRegistry(); ContainerExtensionContext extensionContext = (ContainerExtensionContext) context.getExtensionContext(); - ThrowableCollector throwableCollector = new ThrowableCollector(); + ThrowableCollector throwableCollector = context.getThrowableCollector(); - invokeAfterAllMethods(registry, extensionContext, throwableCollector); + if (context.beforeAllMethodsExecuted()) { + invokeAfterAllMethods(registry, extensionContext, throwableCollector); + } invokeAfterAllCallbacks(registry, extensionContext, throwableCollector); throwableCollector.assertEmpty(); } @@ -193,14 +200,26 @@ protected void invokeTestInstancePostProcessors(Object instance, ExtensionRegist // @formatter:on } - private void invokeBeforeAllCallbacks(ExtensionRegistry registry, ContainerExtensionContext context) { - registry.stream(BeforeAllCallback.class)// - .forEach(extension -> executeAndMaskThrowable(() -> extension.beforeAll(context))); + private void invokeBeforeAllCallbacks(ExtensionRegistry registry, ContainerExtensionContext context, + ThrowableCollector throwableCollector) { + + for (BeforeAllCallback callback : registry.toList(BeforeAllCallback.class)) { + throwableCollector.execute(() -> callback.beforeAll(context)); + if (throwableCollector.isNotEmpty()) { + break; + } + } } - private void invokeBeforeAllMethods(ExtensionRegistry registry, ContainerExtensionContext context) { - this.beforeAllMethods.forEach( - method -> executeAndMaskThrowable(() -> executableInvoker.invoke(method, context, registry))); + private void invokeBeforeAllMethods(ExtensionRegistry registry, ContainerExtensionContext context, + ThrowableCollector throwableCollector) { + + for (Method method : this.beforeAllMethods) { + throwableCollector.execute(() -> executableInvoker.invoke(method, context, registry)); + if (throwableCollector.isNotEmpty()) { + break; + } + } } private void invokeAfterAllMethods(ExtensionRegistry registry, ContainerExtensionContext context, diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/JupiterEngineExecutionContext.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/JupiterEngineExecutionContext.java index 5fe7a5578ca..5d7e332e64e 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/JupiterEngineExecutionContext.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/JupiterEngineExecutionContext.java @@ -57,11 +57,23 @@ public ExtensionContext getExtensionContext() { return this.state.extensionContext; } + public ThrowableCollector getThrowableCollector() { + return this.state.throwableCollector; + } + + public void beforeAllMethodsExecuted(boolean beforeAllMethodsExecuted) { + this.state.beforeAllMethodsExecuted = beforeAllMethodsExecuted; + } + + public boolean beforeAllMethodsExecuted() { + return this.state.beforeAllMethodsExecuted; + } + public Builder extend() { return builder(this); } - public static Builder builder(JupiterEngineExecutionContext context) { + private static Builder builder(JupiterEngineExecutionContext context) { return new Builder(context.state, null); } @@ -69,11 +81,13 @@ private static final class State implements Cloneable { final EngineExecutionListener executionListener; final ConfigurationParameters configurationParameters; + final ThrowableCollector throwableCollector = new ThrowableCollector(); TestInstanceProvider testInstanceProvider; ExtensionRegistry extensionRegistry; ExtensionContext extensionContext; + boolean beforeAllMethodsExecuted = false; - public State(EngineExecutionListener executionListener, ConfigurationParameters configurationParameters) { + State(EngineExecutionListener executionListener, ConfigurationParameters configurationParameters) { this.executionListener = executionListener; this.configurationParameters = configurationParameters; } diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterAllTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterAllTests.java index 9aeb44c3b39..0238826ba81 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterAllTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterAllTests.java @@ -97,13 +97,46 @@ void beforeAllAndAfterAllCallbacksInSubSubclass() { // @formatter:on } + @Test + void beforeAllMethodThrowsAnException() { + // @formatter:off + assertBeforeAllAndAfterAllCallbacks(ExceptionInBeforeAllMethodTestCase.class, 0, 0, + "fooBeforeAllCallback", + "beforeAllMethod", // throws an exception. + // test should not get invoked. + "afterAllMethod", + "fooAfterAllCallback" + ); + // @formatter:on + } + + @Test + void beforeAllCallbackThrowsAnException() { + // @formatter:off + assertBeforeAllAndAfterAllCallbacks(ExceptionInBeforeAllCallbackTestCase.class, 0, 0, + "fooBeforeAllCallback", + "exceptionThrowingBeforeAllCallback", // throws an exception. + // beforeAllMethod should not get invoked. + // test should not get invoked. + // afterAllMethod should not get invoked. + "fooAfterAllCallback" + ); + // @formatter:on + } + private void assertBeforeAllAndAfterAllCallbacks(Class testClass, String... expectedCalls) { + assertBeforeAllAndAfterAllCallbacks(testClass, 1, 1, expectedCalls); + } + + private void assertBeforeAllAndAfterAllCallbacks(Class testClass, int testsStarted, int testsSuccessful, + String... expectedCalls) { + callSequence.clear(); TestDiscoveryRequest request = request().selectors(selectClass(testClass)).build(); ExecutionEventRecorder eventRecorder = executeTests(request); - assertEquals(1, eventRecorder.getTestStartedCount(), "# tests started"); - assertEquals(1, eventRecorder.getTestSuccessfulCount(), "# tests succeeded"); + assertEquals(testsStarted, eventRecorder.getTestStartedCount(), "# tests started"); + assertEquals(testsSuccessful, eventRecorder.getTestSuccessfulCount(), "# tests succeeded"); assertEquals(asList(expectedCalls), callSequence, () -> "wrong call sequence for " + testClass.getName()); } @@ -171,6 +204,47 @@ void test() { } } + @ExtendWith(FooClassLevelCallbacks.class) + private static class ExceptionInBeforeAllMethodTestCase { + + @BeforeAll + static void beforeAll() { + callSequence.add("beforeAllMethod"); + throw new RuntimeException("@BeforeAll"); + } + + @Test + void test() { + callSequence.add("test"); + } + + @AfterAll + static void afterAll() { + callSequence.add("afterAllMethod"); + } + } + + @ExtendWith({ FooClassLevelCallbacks.class, ExceptionThrowingBeforeAllCallback.class }) + private static class ExceptionInBeforeAllCallbackTestCase { + + @BeforeAll + static void beforeAll() { + callSequence.add("beforeAllMethod"); + } + + @Test + void test() { + callSequence.add("test"); + } + + @AfterAll + static void afterAll() { + callSequence.add("afterAllMethod"); + } + } + + // ------------------------------------------------------------------------- + private static class FooClassLevelCallbacks implements BeforeAllCallback, AfterAllCallback { @Override @@ -223,4 +297,13 @@ public void afterAll(ContainerExtensionContext testExecutionContext) { } } + private static class ExceptionThrowingBeforeAllCallback implements BeforeAllCallback { + + @Override + public void beforeAll(ContainerExtensionContext context) { + callSequence.add("exceptionThrowingBeforeAllCallback"); + throw new RuntimeException("BeforeAllCallback"); + } + } + } diff --git a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterEachTests.java b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterEachTests.java index 6ff1536301c..a1814543332 100644 --- a/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterEachTests.java +++ b/junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterEachTests.java @@ -162,7 +162,7 @@ public void beforeEachCallbackThrowsAnException() { "exceptionThrowingBeforeEachCallback", // throws an exception. // barBeforeCallback should not get invoked. // beforeMethod should not get invoked. - // test() should not get invoked. + // test should not get invoked. // afterMethod should not get invoked. "barAfterCallback", "fooAfterCallback" @@ -187,7 +187,7 @@ public void beforeEachMethodThrowsAnException() { assertEquals(asList( "fooBeforeCallback", "beforeMethod", // throws an exception. - // test() should not get invoked. + // test should not get invoked. "afterMethod", "fooAfterCallback" ), callSequence, "wrong call sequence"); diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java index ca53700ddcf..d6daebef9fb 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutor.java @@ -75,19 +75,23 @@ private void execute(TestDescriptor testDescriptor, C parentContext) { this.listener.executionStarted(testDescriptor); TestExecutionResult result = singleTestExecutor.executeSafely(() -> { - C context = node.before(preparedContext); - context = node.execute(context); - - // If a node is not a leaf, execute its children recursively. - // Note: executing children for a leaf would result in accidental - // execution of dynamically added children. - if (!node.isLeaf()) { - for (TestDescriptor child : testDescriptor.getChildren()) { - execute(child, context); + C context = preparedContext; + try { + context = node.before(context); + context = node.execute(context); + + // If a node is NOT a leaf, execute its children recursively. + // Note: executing children for a leaf could result in accidental + // execution of dynamically added children. + if (!node.isLeaf()) { + for (TestDescriptor child : testDescriptor.getChildren()) { + execute(child, context); + } } } - - node.after(context); + finally { + node.after(context); + } }); this.listener.executionFinished(testDescriptor, result); diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutorTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutorTests.java index 454ff4973af..f5e094a6221 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutorTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutorTests.java @@ -208,7 +208,7 @@ public void exceptionInContainerBeforeAll() throws Exception { inOrder.verify(root).shouldBeSkipped(rootContext); inOrder.verify(listener).executionStarted(root); inOrder.verify(root).before(rootContext); - inOrder.verify(root, never()).after(rootContext); + inOrder.verify(root).after(rootContext); inOrder.verify(listener).executionFinished(eq(root), rootExecutionResult.capture()); assertTrue(rootExecutionResult.getValue().getStatus() == TestExecutionResult.Status.FAILED, @@ -289,7 +289,7 @@ public void abortInContainerBeforeAll() throws Exception { inOrder.verify(root).shouldBeSkipped(rootContext); inOrder.verify(listener).executionStarted(root); inOrder.verify(root).before(rootContext); - inOrder.verify(root, never()).after(rootContext); + inOrder.verify(root).after(rootContext); inOrder.verify(listener).executionFinished(eq(root), rootExecutionResult.capture()); assertTrue(rootExecutionResult.getValue().getStatus() == TestExecutionResult.Status.ABORTED,