Fix container failure XML reporting (#2542)

Prior to this commit, failing containers were only reported in case they
contained at least one test. However, for example for a parameterized
Jupiter tests and an exception in a `@BeforeAll` method, that led to
failures being silently swallowed. Now, in addition to tests, leaf
nodes of the test tree are always included in the XML report, even if
they are containers, not tests.

Moreover, failures on the container level that occurred after their
children had been executed were not reported, e.g. when an exception
was thrown from a Jupiter `@AfterAll` method. Now such failures cause
the contained tests to be reported as failed.

Fixes #2537.

Author: marcphilipp

documentation/src/docs/asciidoc/release-notes/release-notes-5.7.1.adoc

@@ -27,6 +27,8 @@ GitHub.
   of this change may be witnessed by end users and test engine or extension authors.
 * Method `scanForClassesInPackage(String)` in `ClasspathScanner` now returns a valid list
   of class names when the package name is equal to the name of a module on the module path.
+* The legacy XML report now always includes container-level failures (e.g. from
+  `@BeforeAll` Jupiter lifecycle methods).
 
 ==== Deprecations and Breaking Changes
 

junit-platform-engine/src/testFixtures/java/org/junit/platform/engine/support/hierarchical/DemoEngineExecutionContext.java

@@ -13,5 +13,5 @@
 /**
  * @since 1.0
  */
-class DemoEngineExecutionContext implements EngineExecutionContext {
+public class DemoEngineExecutionContext implements EngineExecutionContext {
 }

junit-platform-engine/src/testFixtures/java/org/junit/platform/engine/support/hierarchical/DemoHierarchicalTestDescriptor.java

@@ -26,7 +26,7 @@ public class DemoHierarchicalTestDescriptor extends AbstractTestDescriptor imple
 	private String skippedReason;
 	private boolean skipped;
 
-	DemoHierarchicalTestDescriptor(UniqueId uniqueId, String displayName, Runnable executeBlock) {
+	public DemoHierarchicalTestDescriptor(UniqueId uniqueId, String displayName, Runnable executeBlock) {
 		this(uniqueId, displayName, null, executeBlock);
 	}
 

junit-platform-engine/src/testFixtures/java/org/junit/platform/engine/support/hierarchical/DemoHierarchicalTestEngine.java

@@ -11,6 +11,7 @@
 package org.junit.platform.engine.support.hierarchical;
 
 import java.lang.reflect.Method;
+import java.util.function.Function;
 
 import org.junit.platform.engine.EngineDiscoveryRequest;
 import org.junit.platform.engine.ExecutionRequest;
@@ -59,10 +60,8 @@ public DemoHierarchicalTestDescriptor addTest(Method testMethod, Runnable execut
 	}
 
 	public DemoHierarchicalTestDescriptor addTest(String uniqueName, String displayName, Runnable executeBlock) {
-		UniqueId uniqueId = engineDescriptor.getUniqueId().append("test", uniqueName);
-		DemoHierarchicalTestDescriptor child = new DemoHierarchicalTestDescriptor(uniqueId, displayName, executeBlock);
-		engineDescriptor.addChild(child);
-		return child;
+		return addChild(uniqueName, uniqueId -> new DemoHierarchicalTestDescriptor(uniqueId, displayName, executeBlock),
+			"test");
 	}
 
 	public DemoHierarchicalContainerDescriptor addContainer(String uniqueName, String displayName, TestSource source) {
@@ -76,11 +75,17 @@ public DemoHierarchicalContainerDescriptor addContainer(String uniqueName, Runna
 	public DemoHierarchicalContainerDescriptor addContainer(String uniqueName, String displayName, TestSource source,
 			Runnable beforeBlock) {
 
-		UniqueId uniqueId = engineDescriptor.getUniqueId().append("container", uniqueName);
-		DemoHierarchicalContainerDescriptor container = new DemoHierarchicalContainerDescriptor(uniqueId, displayName,
-			source, beforeBlock);
-		engineDescriptor.addChild(container);
-		return container;
+		return addChild(uniqueName,
+			uniqueId -> new DemoHierarchicalContainerDescriptor(uniqueId, displayName, source, beforeBlock),
+			"container");
+	}
+
+	public <T extends TestDescriptor & Node<DemoEngineExecutionContext>> T addChild(String uniqueName,
+			Function<UniqueId, T> creator, String segmentType) {
+		var uniqueId = engineDescriptor.getUniqueId().append(segmentType, uniqueName);
+		var child = creator.apply(uniqueId);
+		engineDescriptor.addChild(child);
+		return child;
 	}
 
 	@Override

junit-platform-reporting/src/main/java/org/junit/platform/reporting/legacy/xml/XmlReportData.java

@@ -11,15 +11,16 @@
 package org.junit.platform.reporting.legacy.xml;
 
 import static java.util.Collections.emptyList;
+import static java.util.stream.Collectors.toList;
 import static org.junit.platform.engine.TestExecutionResult.Status.ABORTED;
-import static org.junit.platform.engine.TestExecutionResult.Status.SUCCESSFUL;
 
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Predicate;
@@ -103,32 +104,23 @@ String getSkipReason(TestIdentifier testIdentifier) {
 		}).orElse(null);
 	}
 
-	Optional<TestExecutionResult> getResult(TestIdentifier testIdentifier) {
-		if (this.finishedTests.containsKey(testIdentifier)) {
-			return Optional.of(this.finishedTests.get(testIdentifier));
-		}
-		Optional<TestIdentifier> parent = this.testPlan.getParent(testIdentifier);
-		Optional<TestIdentifier> ancestor = findAncestor(parent, this.finishedTests::containsKey);
-		if (ancestor.isPresent()) {
-			TestExecutionResult result = this.finishedTests.get(ancestor.get());
-			if (result.getStatus() != SUCCESSFUL) {
-				return Optional.of(result);
-			}
-		}
-		return Optional.empty();
+	List<TestExecutionResult> getResults(TestIdentifier testIdentifier) {
+		return getAncestors(testIdentifier).stream() //
+				.map(this.finishedTests::get) //
+				.filter(Objects::nonNull) //
+				.collect(toList());
 	}
 
 	List<ReportEntry> getReportEntries(TestIdentifier testIdentifier) {
 		return this.reportEntries.getOrDefault(testIdentifier, emptyList());
 	}
 
 	private Optional<TestIdentifier> findSkippedAncestor(TestIdentifier testIdentifier) {
-		return findAncestor(Optional.of(testIdentifier), this.skippedTests::containsKey);
+		return findAncestor(testIdentifier, this.skippedTests::containsKey);
 	}
 
-	private Optional<TestIdentifier> findAncestor(Optional<TestIdentifier> testIdentifier,
-			Predicate<TestIdentifier> predicate) {
-		Optional<TestIdentifier> current = testIdentifier;
+	private Optional<TestIdentifier> findAncestor(TestIdentifier testIdentifier, Predicate<TestIdentifier> predicate) {
+		Optional<TestIdentifier> current = Optional.of(testIdentifier);
 		while (current.isPresent()) {
 			if (predicate.test(current.get())) {
 				return current;
@@ -138,4 +130,14 @@ private Optional<TestIdentifier> findAncestor(Optional<TestIdentifier> testIdent
 		return Optional.empty();
 	}
 
+	private List<TestIdentifier> getAncestors(TestIdentifier testIdentifier) {
+		TestIdentifier current = testIdentifier;
+		List<TestIdentifier> ancestors = new ArrayList<>();
+		while (current != null) {
+			ancestors.add(current);
+			current = this.testPlan.getParent(current).orElse(null);
+		}
+		return ancestors;
+	}
+
 }