Exceptions in @AfterAll methods fail aborted containers

Issue: #1313

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassExtensionContext.java

@@ -19,9 +19,9 @@
 import org.apiguardian.api.API;
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.engine.execution.ThrowableCollector;
 import org.junit.platform.engine.ConfigurationParameters;
 import org.junit.platform.engine.EngineExecutionListener;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * @since 5.0

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java

@@ -45,7 +45,6 @@
 import org.junit.jupiter.engine.execution.ExecutableInvoker;
 import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
 import org.junit.jupiter.engine.execution.TestInstanceProvider;
-import org.junit.jupiter.engine.execution.ThrowableCollector;
 import org.junit.jupiter.engine.extension.ExtensionRegistry;
 import org.junit.platform.commons.JUnitException;
 import org.junit.platform.commons.util.BlacklistedExceptions;
@@ -57,6 +56,7 @@
 import org.junit.platform.engine.UniqueId;
 import org.junit.platform.engine.support.descriptor.ClassSource;
 import org.junit.platform.engine.support.hierarchical.ExclusiveResource;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * {@link TestDescriptor} for tests based on Java classes.
@@ -201,7 +201,7 @@ public JupiterEngineExecutionContext before(JupiterEngineExecutionContext contex
 	@Override
 	public void after(JupiterEngineExecutionContext context) {
 
-		boolean throwableWasNotAlreadyThrown = context.getThrowableCollector().isEmpty();
+		Throwable previousThrowable = context.getThrowableCollector().getThrowable();
 
 		if (context.beforeAllMethodsExecuted()) {
 			invokeAfterAllMethods(context);
@@ -211,19 +211,12 @@ public void after(JupiterEngineExecutionContext context) {
 			invokeAfterAllCallbacks(context);
 		}
 
-		// If the ThrowableCollector was not empty when this method was called,
+		// If the previous Throwable was not null when this method was called,
 		// that means an exception was already thrown either before or during
 		// the execution of this Node. If an exception was already thrown, any
 		// later exceptions were added as suppressed exceptions to that original
-		// exception, and we don't need to rethrow the original exception here
-		// since it will be thrown by HierarchicalTestExecutor.NodeExecutor.
-		//
-		// However, if no exception was previously thrown, we need to ensure
-		// that any exceptions thrown by @AfterAll methods or AfterAllCallbacks
-		// are thrown here.
-		if (throwableWasNotAlreadyThrown) {
-			context.getThrowableCollector().assertEmpty();
-		}
+		// exception unless a more severe exception occurred in the meantime.
+		context.getThrowableCollector().assertNotSame(previousThrowable);
 	}
 
 	private TestInstanceFactory resolveTestInstanceFactory(ExtensionRegistry registry) {

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/MethodExtensionContext.java

@@ -19,9 +19,9 @@
 import org.apiguardian.api.API;
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.engine.execution.ThrowableCollector;
 import org.junit.platform.engine.ConfigurationParameters;
 import org.junit.platform.engine.EngineExecutionListener;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * @since 5.0

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestMethodTestDescriptor.java

@@ -26,16 +26,16 @@
 import org.junit.jupiter.api.extension.Extension;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.TestExecutionExceptionHandler;
-import org.junit.jupiter.api.function.Executable;
 import org.junit.jupiter.engine.execution.AfterEachMethodAdapter;
 import org.junit.jupiter.engine.execution.BeforeEachMethodAdapter;
 import org.junit.jupiter.engine.execution.ExecutableInvoker;
 import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
-import org.junit.jupiter.engine.execution.ThrowableCollector;
 import org.junit.jupiter.engine.extension.ExtensionRegistry;
 import org.junit.platform.commons.util.ExceptionUtils;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.UniqueId;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector.Executable;
 
 /**
  * {@link TestDescriptor} for tests based on Java methods.

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

@@ -26,6 +26,7 @@
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
 import org.junit.jupiter.api.extension.ExtensionContextException;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * {@code ExtensionValuesStore} is used inside implementations of

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

@@ -21,6 +21,7 @@
 import org.junit.platform.engine.ConfigurationParameters;
 import org.junit.platform.engine.EngineExecutionListener;
 import org.junit.platform.engine.support.hierarchical.EngineExecutionContext;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * @since 5.0

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/ExceptionHandlingTests.java

@@ -27,6 +27,7 @@
 import static org.junit.platform.engine.test.event.TestExecutionResultConditions.isA;
 import static org.junit.platform.engine.test.event.TestExecutionResultConditions.message;
 import static org.junit.platform.engine.test.event.TestExecutionResultConditions.suppressed;
+import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;
 
 import java.io.IOException;
 import java.lang.reflect.Method;
@@ -44,6 +45,7 @@
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.platform.engine.test.event.ExecutionEventRecorder;
+import org.junit.platform.launcher.LauncherDiscoveryRequest;
 import org.opentest4j.AssertionFailedError;
 import org.opentest4j.TestAbortedException;
 
@@ -247,6 +249,25 @@ void exceptionInConstructorPreventsExecutionOfAfterAllCallbacksWhenTestInstanceP
 			event(engine(), finishedSuccessfully()));
 	}
 
+	@Test
+	void failureInAfterAllTakesPrecedenceOverTestAbortedExceptionInBeforeAll() throws NoSuchMethodException {
+		Method method = FailureTestCase.class.getDeclaredMethod("succeedingTest");
+		LauncherDiscoveryRequest request = request().selectors(selectMethod(FailureTestCase.class, method)).build();
+
+		FailureTestCase.exceptionToThrowInBeforeAll = Optional.of(new TestAbortedException("aborted"));
+		FailureTestCase.exceptionToThrowInAfterAll = Optional.of(new IOException("checked"));
+
+		ExecutionEventRecorder eventRecorder = executeTests(request);
+
+		assertRecordedExecutionEventsContainsExactly(eventRecorder.getExecutionEvents(), //
+			event(engine(), started()), //
+			event(container(FailureTestCase.class), started()), //
+			event(container(FailureTestCase.class),
+				finishedWithFailure(allOf(isA(IOException.class), message("checked"),
+					suppressed(0, allOf(isA(TestAbortedException.class), message("aborted")))))), //
+			event(engine(), finishedSuccessfully()));
+	}
+
 	@AfterEach
 	void cleanUpExceptions() {
 		FailureTestCase.exceptionToThrowInBeforeAll = Optional.empty();

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/descriptor/TestFactoryTestDescriptorTests.java

@@ -28,7 +28,6 @@
 import org.junit.jupiter.api.TestFactory;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.engine.execution.JupiterEngineExecutionContext;
-import org.junit.jupiter.engine.execution.ThrowableCollector;
 import org.junit.platform.engine.TestSource;
 import org.junit.platform.engine.UniqueId;
 import org.junit.platform.engine.support.descriptor.ClasspathResourceSource;
@@ -37,6 +36,7 @@
 import org.junit.platform.engine.support.descriptor.FileSource;
 import org.junit.platform.engine.support.descriptor.UriSource;
 import org.junit.platform.engine.support.hierarchical.Node;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 
 /**
  * Unit tests for {@link TestFactoryTestDescriptor}.

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/execution/ExtensionContextTests.java

@@ -43,6 +43,7 @@
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.UniqueId;
 import org.junit.platform.engine.reporting.ReportEntry;
+import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 

junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/NodeTestTask.java

@@ -11,8 +11,6 @@
 package org.junit.platform.engine.support.hierarchical;
 
 import static java.util.stream.Collectors.toCollection;
-import static org.junit.platform.commons.util.BlacklistedExceptions.rethrowIfBlacklisted;
-import static org.junit.platform.engine.TestExecutionResult.Status.FAILED;
 import static org.junit.platform.engine.TestExecutionResult.failed;
 
 import java.util.ArrayList;
@@ -24,7 +22,6 @@
 import org.junit.platform.commons.JUnitException;
 import org.junit.platform.engine.EngineExecutionListener;
 import org.junit.platform.engine.TestDescriptor;
-import org.junit.platform.engine.TestExecutionResult;
 import org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutorService.TestTask;
 import org.junit.platform.engine.support.hierarchical.Node.ExecutionMode;
 import org.junit.platform.engine.support.hierarchical.Node.SkipResult;
@@ -34,9 +31,7 @@
  */
 class NodeTestTask<C extends EngineExecutionContext> implements TestTask {
 
-	private static final SingleTestExecutor singleTestExecutor = new SingleTestExecutor();
-
-	private final List<Throwable> executionErrors = new ArrayList<>();
+	private final ThrowableCollector throwableCollector = new ThrowableCollector();
 	private final TestDescriptor testDescriptor;
 	private final EngineExecutionListener listener;
 	private final HierarchicalTestExecutorService executorService;
@@ -52,7 +47,7 @@ class NodeTestTask<C extends EngineExecutionContext> implements TestTask {
 	private C context;
 
 	private SkipResult skipResult;
-	private TestExecutionResult executionResult;
+	private boolean started;
 
 	NodeTestTask(TestDescriptor testDescriptor, EngineExecutionListener listener,
 			HierarchicalTestExecutorService executorService) {
@@ -102,10 +97,10 @@ public void setParentContext(C parentContext) {
 	@Override
 	public void execute() {
 		prepare();
-		if (executionErrors.isEmpty()) {
+		if (throwableCollector.isEmpty()) {
 			checkWhetherSkipped();
 		}
-		if (executionErrors.isEmpty() && !skipResult.isSkipped()) {
+		if (throwableCollector.isEmpty() && !skipResult.isSkipped()) {
 			executeRecursively();
 		}
 		if (context != null) {
@@ -115,40 +110,34 @@ public void execute() {
 	}
 
 	private void prepare() {
-		executeSafely(() -> context = node.prepare(parentContext));
+		throwableCollector.execute(() -> context = node.prepare(parentContext));
 	}
 
 	private void checkWhetherSkipped() {
-		executeSafely(() -> skipResult = node.shouldBeSkipped(context));
+		throwableCollector.execute(() -> skipResult = node.shouldBeSkipped(context));
 	}
 
 	private void executeRecursively() {
 		listener.executionStarted(testDescriptor);
+		started = true;
 
-		executionResult = singleTestExecutor.executeSafely(() -> {
-			Throwable failure = null;
-			try {
-				context = node.before(context);
+		throwableCollector.execute(() -> {
+			context = node.before(context);
 
-				List<Future<?>> futures = new ArrayList<>();
-				context = node.execute(context,
-					dynamicTestDescriptor -> executeDynamicTest(dynamicTestDescriptor, futures));
+			List<Future<?>> futures = new ArrayList<>();
+			context = node.execute(context,
+				dynamicTestDescriptor -> executeDynamicTest(dynamicTestDescriptor, futures));
 
-				children.forEach(child -> child.setParentContext(context));
-				executorService.invokeAll(children);
+			children.forEach(child -> child.setParentContext(context));
+			executorService.invokeAll(children);
 
-				// using a for loop for the sake for ForkJoinPool's work stealing
-				for (Future<?> future : futures) {
-					future.get();
-				}
-			}
-			catch (Throwable t) {
-				failure = t;
-			}
-			finally {
-				executeAfter(failure);
+			// using a for loop for the sake for ForkJoinPool's work stealing
+			for (Future<?> future : futures) {
+				future.get();
 			}
 		});
+
+		throwableCollector.execute(() -> node.after(context));
 	}
 
 	private void executeDynamicTest(TestDescriptor dynamicTestDescriptor, List<Future<?>> futures) {
@@ -166,74 +155,20 @@ private void executeDynamicTest(TestDescriptor dynamicTestDescriptor, List<Futur
 		}
 	}
 
-	private void executeAfter(Throwable failure) throws Throwable {
-		try {
-			node.after(context);
-		}
-		catch (Throwable t) {
-			if (failure != null && failure != t) {
-				failure.addSuppressed(t);
-			}
-			else {
-				throw t;
-			}
-		}
-		if (failure != null) {
-			throw failure;
-		}
-	}
-
 	private void cleanUp() {
-		executeSafely(() -> node.cleanUp(context));
+		throwableCollector.execute(() -> node.cleanUp(context));
 	}
 
 	private void reportCompletion() {
-		if (executionResult != null) {
-			addExecutionErrorsToTestExecutionResult();
-			listener.executionFinished(testDescriptor, executionResult);
-		}
-		else if (executionErrors.isEmpty() && skipResult.isSkipped()) {
+		if (throwableCollector.isEmpty() && skipResult.isSkipped()) {
 			listener.executionSkipped(testDescriptor, skipResult.getReason().orElse("<unknown>"));
+			return;
 		}
-		else {
+		if (!started) {
 			// Call executionStarted first to comply with the contract of EngineExecutionListener.
 			listener.executionStarted(testDescriptor);
-			listener.executionFinished(testDescriptor, createTestExecutionResultFromExecutionErrors());
 		}
-	}
-
-	private void addExecutionErrorsToTestExecutionResult() {
-		if (executionErrors.isEmpty()) {
-			return;
-		}
-		if (executionResult.getStatus() == FAILED && executionResult.getThrowable().isPresent()) {
-			Throwable throwable = executionResult.getThrowable().get();
-			executionErrors.forEach(throwable::addSuppressed);
-		}
-		else {
-			executionResult = createTestExecutionResultFromExecutionErrors();
-		}
-	}
-
-	private TestExecutionResult createTestExecutionResultFromExecutionErrors() {
-		Throwable throwable = executionErrors.get(0);
-		executionErrors.stream().skip(1).forEach(throwable::addSuppressed);
-		return failed(throwable);
-	}
-
-	private void executeSafely(Action action) {
-		try {
-			action.execute();
-		}
-		catch (Throwable t) {
-			rethrowIfBlacklisted(t);
-			executionErrors.add(t);
-		}
-	}
-
-	@FunctionalInterface
-	private interface Action {
-		void execute() throws Exception;
+		listener.executionFinished(testDescriptor, throwableCollector.toTestExecutionResult());
 	}
 
 	@SuppressWarnings("unchecked")

junit-platform-engine/src/main/java/org/junit/platform/engine/support/hierarchical/SingleTestExecutor.java

@@ -10,7 +10,7 @@
 
 package org.junit.platform.engine.support.hierarchical;
 
-import static org.apiguardian.api.API.Status.MAINTAINED;
+import static org.apiguardian.api.API.Status.DEPRECATED;
 import static org.junit.platform.commons.util.BlacklistedExceptions.rethrowIfBlacklisted;
 import static org.junit.platform.engine.TestExecutionResult.aborted;
 import static org.junit.platform.engine.TestExecutionResult.failed;
@@ -26,8 +26,11 @@
  *
  * @since 1.0
  * @see #executeSafely(Executable)
+ * @deprecated Please use {@link ThrowableCollector#execute} and
+ * {@link ThrowableCollector#toTestExecutionResult} instead.
  */
-@API(status = MAINTAINED, since = "1.0")
+@Deprecated
+@API(status = DEPRECATED, since = "1.2")
 public class SingleTestExecutor {
 
 	/**