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

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,

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/JupiterEngineExecutionContext.java

@@ -57,23 +57,37 @@ 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);
 	}
 
 	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;
 		}

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");
+		}
+	}
+
 }

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");

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);

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,