Make SummaryGeneratingListener thread-safe (#2549)

Prior to this commit, concurrently reported failures were stored in an
unguarded `ArrayList` which caused a race condition that sometimes
caused failures to get lost and hence not reported. Now the list of
failures is wrapped in `synchronizedList` to make it thread-safe.

Fixes #2546.

Co-authored-by: Christian Stein <[email protected]>

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-5.8.0-M1.adoc

@@ -15,7 +15,8 @@ on GitHub.
 
 ==== Bug Fixes
 
-* ❓
+* `SummaryGeneratingListener` is now thread-safe and can handle concurrently failing
+  tests.
 
 ==== Deprecations and Breaking Changes
 

junit-jupiter-engine/junit-jupiter-engine.gradle.kts

@@ -17,6 +17,7 @@ dependencies {
 	testImplementation(project(":junit-platform-launcher"))
 	testImplementation(project(":junit-platform-runner"))
 	testImplementation(project(":junit-platform-testkit"))
+	testImplementation(testFixtures(project(":junit-platform-commons")))
 	testImplementation("org.jetbrains.kotlin:kotlin-stdlib")
 	testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-core")
 	testImplementation("org.codehaus.groovy:groovy")

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

@@ -10,22 +10,16 @@
 
 package org.junit.jupiter.engine.execution;
 
-import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.platform.commons.test.ConcurrencyTestingUtils.executeConcurrently;
 
-import java.util.ArrayList;
 import java.util.List;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
-import java.util.function.Supplier;
 
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
@@ -300,7 +294,7 @@ void removeNullValueWithTypeSafety() {
 		}
 
 		@Test
-		void simulateRaceConditionInGetOrComputeIfAbsent() {
+		void simulateRaceConditionInGetOrComputeIfAbsent() throws Exception {
 			int threads = 10;
 			AtomicInteger counter = new AtomicInteger();
 			ExtensionValuesStore localStore = new ExtensionValuesStore(null);
@@ -313,30 +307,6 @@ void simulateRaceConditionInGetOrComputeIfAbsent() {
 		}
 	}
 
-	private <T> List<T> executeConcurrently(int threads, Supplier<T> supplier) {
-		ExecutorService executorService = Executors.newFixedThreadPool(threads);
-		try {
-			CountDownLatch latch = new CountDownLatch(threads);
-			List<CompletableFuture<T>> futures = new ArrayList<>();
-			for (int i = 0; i < threads; i++) {
-				futures.add(CompletableFuture.supplyAsync(() -> {
-					latch.countDown();
-					try {
-						latch.await();
-					}
-					catch (InterruptedException e) {
-						Thread.currentThread().interrupt();
-					}
-					return supplier.get();
-				}, executorService));
-			}
-			return futures.stream().map(CompletableFuture::join).collect(toList());
-		}
-		finally {
-			executorService.shutdown();
-		}
-	}
-
 	@Nested
 	class InheritedValuesTests {
 

junit-platform-commons/junit-platform-commons.gradle.kts

@@ -4,6 +4,7 @@ plugins {
 	`java-library-conventions`
 	`java-multi-release-sources`
 	`java-repackage-jars`
+	`java-test-fixtures`
 }
 
 description = "JUnit Platform Commons"

junit-platform-commons/src/testFixtures/java/org/junit/platform/commons/test/ConcurrencyTestingUtils.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2015-2021 the original author or authors.
+ *
+ * All rights reserved. This program and the accompanying materials are
+ * made available under the terms of the Eclipse Public License v2.0 which
+ * accompanies this distribution and is available at
+ *
+ * https://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.platform.commons.test;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class ConcurrencyTestingUtils {
+
+	public static void executeConcurrently(int threads, Runnable action) throws Exception {
+		executeConcurrently(threads, () -> {
+			action.run();
+			return null;
+		});
+	}
+
+	public static <T> List<T> executeConcurrently(int threads, Callable<T> action) throws Exception {
+		ExecutorService executorService = Executors.newFixedThreadPool(threads);
+		try {
+			CountDownLatch latch = new CountDownLatch(threads);
+			List<CompletableFuture<T>> futures = new ArrayList<>();
+			for (int i = 0; i < threads; i++) {
+				futures.add(CompletableFuture.supplyAsync(() -> {
+					try {
+						latch.countDown();
+						latch.await();
+						return action.call();
+					}
+					catch (InterruptedException e) {
+						Thread.currentThread().interrupt();
+						throw new CompletionException(e);
+					}
+					catch (Exception e) {
+						throw new CompletionException("Action failed", e);
+					}
+				}, executorService));
+			}
+			List<T> list = new ArrayList<>();
+			for (CompletableFuture<T> future : futures) {
+				list.add(future.get(5, SECONDS));
+			}
+			return list;
+		}
+		finally {
+			executorService.shutdownNow();
+			var terminated = executorService.awaitTermination(5, SECONDS);
+			if (!terminated) {
+				//noinspection ThrowFromFinallyBlock
+				throw new AssertionError("ExecutorService did not cleanly shut down");
+			}
+		}
+	}
+}

junit-platform-launcher/src/main/java/org/junit/platform/launcher/listeners/MutableTestExecutionSummary.java

@@ -11,6 +11,7 @@
 package org.junit.platform.launcher.listeners;
 
 import static java.lang.String.join;
+import static java.util.Collections.synchronizedList;
 
 import java.io.PrintWriter;
 import java.util.ArrayList;
@@ -54,7 +55,7 @@ class MutableTestExecutionSummary implements TestExecutionSummary {
 	final AtomicLong testsFailed = new AtomicLong();
 
 	private final TestPlan testPlan;
-	private final List<Failure> failures = new ArrayList<>();
+	private final List<Failure> failures = synchronizedList(new ArrayList<>());
 	private final long timeStarted;
 	long timeFinished;
 

platform-tests/platform-tests.gradle.kts

@@ -18,6 +18,7 @@ dependencies {
 	// --- Things we are testing with ---------------------------------------------
 	testImplementation(project(":junit-platform-runner"))
 	testImplementation(project(":junit-platform-testkit"))
+	testImplementation(testFixtures(project(":junit-platform-commons")))
 	testImplementation(testFixtures(project(":junit-platform-engine")))
 	testImplementation(testFixtures(project(":junit-platform-launcher")))
 	testImplementation(project(":junit-jupiter-engine"))

platform-tests/src/test/java/org/junit/platform/launcher/listeners/SummaryGenerationTests.java

@@ -15,12 +15,14 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.platform.commons.test.ConcurrencyTestingUtils.executeConcurrently;
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.util.List;
 import java.util.Optional;
 
+import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
 import org.junit.platform.engine.TestExecutionResult;
 import org.junit.platform.engine.TestSource;
@@ -220,6 +222,27 @@ public Optional<TestSource> getSource() {
 		);
 	}
 
+	@RepeatedTest(10)
+	void reportingConcurrentlyFinishedTests() throws Exception {
+		var numThreads = 250;
+		var testIdentifier = TestIdentifier.from(new TestDescriptorStub(UniqueId.root("root", "2"), "failingTest") {
+			@Override
+			public Optional<TestSource> getSource() {
+				return Optional.of(ClassSource.from(Object.class));
+			}
+		});
+		var result = TestExecutionResult.failed(new RuntimeException());
+
+		listener.testPlanExecutionStarted(testPlan);
+		executeConcurrently(numThreads, () -> {
+			listener.executionStarted(testIdentifier);
+			listener.executionFinished(testIdentifier, result);
+		});
+		listener.testPlanExecutionFinished(testPlan);
+
+		assertThat(listener.getSummary().getFailures()).hasSize(numThreads);
+	}
+
 	@SuppressWarnings("deprecation")
 	private TestIdentifier createTestIdentifier(String uniqueId) {
 		var identifier = TestIdentifier.from(new TestDescriptorStub(UniqueId.root("test", uniqueId), uniqueId));