Report intermediate containers finished lazily

Since JUnit 4 does report events for intermediate containers, the
Vintage engine has to synthesize them. Until now this was done lazily
for start events, i.e. when the first child was started, but eagerly for
finish events, i.e. when the last static child of a container was
finished. However, the latter approach is problematic for containers
that contain both static and dynamic children, for example a Spock
specification with a regular and an unrolled feature method. Instead,
intermediate containers are now reported as finished as soon as a test
is started for which the container is not an ancestor.

Fixes #1819.

junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/EventType.java

@@ -0,0 +1,18 @@
+/*
+ * Copyright 2015-2019 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
+ *
+ * http://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.vintage.engine.execution;
+
+/**
+ * @since 5.4.1
+ */
+enum EventType {
+	REPORTED, SYNTHETIC
+}

junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/RunListenerAdapter.java

@@ -47,7 +47,7 @@ class RunListenerAdapter extends RunListener {
 	@Override
 	public void testRunStarted(Description description) {
 		if (description.isSuite() && description.getAnnotation(Ignore.class) == null) {
-			fireExecutionStarted(testRun.getRunnerTestDescriptor());
+			fireExecutionStarted(testRun.getRunnerTestDescriptor(), EventType.REPORTED);
 		}
 	}
 
@@ -58,7 +58,7 @@ public void testIgnored(Description description) {
 
 	@Override
 	public void testStarted(Description description) {
-		testStarted(lookupOrRegisterTestDescriptor(description));
+		testStarted(lookupOrRegisterTestDescriptor(description), EventType.REPORTED);
 	}
 
 	@Override
@@ -81,8 +81,11 @@ public void testRunFinished(Result result) {
 		RunnerTestDescriptor runnerTestDescriptor = testRun.getRunnerTestDescriptor();
 		if (testRun.isNotSkipped(runnerTestDescriptor)) {
 			if (testRun.isNotStarted(runnerTestDescriptor)) {
-				fireExecutionStarted(runnerTestDescriptor);
+				fireExecutionStarted(runnerTestDescriptor, EventType.SYNTHETIC);
 			}
+			testRun.getInProgressTestDescriptorsWithSyntheticStartEvents().stream() //
+					.filter(this::canFinish) //
+					.forEach(this::fireExecutionFinished);
 			if (testRun.isNotFinished(runnerTestDescriptor)) {
 				fireExecutionFinished(runnerTestDescriptor);
 			}
@@ -128,17 +131,17 @@ private void handleFailure(Failure failure, Function<Throwable, TestExecutionRes
 
 	private void fireMissingContainerEvents(TestDescriptor testDescriptor) {
 		if (testRun.isNotStarted(testDescriptor)) {
-			testStarted(testDescriptor);
+			testStarted(testDescriptor, EventType.SYNTHETIC);
 		}
 		if (testRun.isNotFinished(testDescriptor)) {
 			testFinished(testDescriptor);
 		}
 	}
 
 	private void testIgnored(TestDescriptor testDescriptor, String reason) {
+		fireExecutionFinishedForInProgressNonAncestorTestDescriptorsWithSyntheticStartEvents(testDescriptor);
 		fireExecutionStartedIncludingUnstartedAncestors(testDescriptor.getParent());
 		fireExecutionSkipped(testDescriptor, reason);
-		fireExecutionFinishedIncludingAncestorsWithoutPendingChildren(testDescriptor.getParent());
 	}
 
 	private String determineReasonForIgnoredTest(Description description) {
@@ -151,27 +154,38 @@ private void dynamicTestRegistered(TestDescriptor testDescriptor) {
 		listener.dynamicTestRegistered(testDescriptor);
 	}
 
-	private void testStarted(TestDescriptor testDescriptor) {
+	private void testStarted(TestDescriptor testDescriptor, EventType eventType) {
+		fireExecutionFinishedForInProgressNonAncestorTestDescriptorsWithSyntheticStartEvents(testDescriptor);
 		fireExecutionStartedIncludingUnstartedAncestors(testDescriptor.getParent());
-		fireExecutionStarted(testDescriptor);
+		fireExecutionStarted(testDescriptor, eventType);
+	}
+
+	private void fireExecutionFinishedForInProgressNonAncestorTestDescriptorsWithSyntheticStartEvents(
+			TestDescriptor testDescriptor) {
+		testRun.getInProgressTestDescriptorsWithSyntheticStartEvents().stream() //
+				.filter(it -> !isAncestor(it, testDescriptor) && canFinish(it)) //
+				.forEach(this::fireExecutionFinished);
+	}
+
+	private boolean isAncestor(TestDescriptor candidate, TestDescriptor testDescriptor) {
+		Optional<TestDescriptor> parent = testDescriptor.getParent();
+		if (!parent.isPresent()) {
+			return false;
+		}
+		if (parent.get().equals(candidate)) {
+			return true;
+		}
+		return isAncestor(candidate, parent.get());
 	}
 
 	private void testFinished(TestDescriptor descriptor) {
 		fireExecutionFinished(descriptor);
-		fireExecutionFinishedIncludingAncestorsWithoutPendingChildren(descriptor.getParent());
 	}
 
 	private void fireExecutionStartedIncludingUnstartedAncestors(Optional<TestDescriptor> parent) {
 		if (parent.isPresent() && canStart(parent.get())) {
 			fireExecutionStartedIncludingUnstartedAncestors(parent.get().getParent());
-			fireExecutionStarted(parent.get());
-		}
-	}
-
-	private void fireExecutionFinishedIncludingAncestorsWithoutPendingChildren(Optional<TestDescriptor> parent) {
-		if (parent.isPresent() && canFinish(parent.get())) {
-			fireExecutionFinished(parent.get());
-			fireExecutionFinishedIncludingAncestorsWithoutPendingChildren(parent.get().getParent());
+			fireExecutionStarted(parent.get(), EventType.SYNTHETIC);
 		}
 	}
 
@@ -192,8 +206,8 @@ private void fireExecutionSkipped(TestDescriptor testDescriptor, String reason)
 		listener.executionSkipped(testDescriptor, reason);
 	}
 
-	private void fireExecutionStarted(TestDescriptor testDescriptor) {
-		testRun.markStarted(testDescriptor);
+	private void fireExecutionStarted(TestDescriptor testDescriptor, EventType eventType) {
+		testRun.markStarted(testDescriptor, eventType);
 		listener.executionStarted(testDescriptor);
 	}
 

junit-vintage-engine/src/main/java/org/junit/vintage/engine/execution/TestRun.java

@@ -19,11 +19,14 @@
 import static org.junit.platform.engine.TestExecutionResult.successful;
 
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -46,7 +49,7 @@ class TestRun {
 	private final Map<Description, List<VintageTestDescriptor>> descriptionToDescriptors;
 	private final Map<TestDescriptor, List<TestExecutionResult>> executionResults = new LinkedHashMap<>();
 	private final Set<TestDescriptor> skippedDescriptors = new LinkedHashSet<>();
-	private final Set<TestDescriptor> startedDescriptors = new LinkedHashSet<>();
+	private final Map<TestDescriptor, EventType> startedDescriptors = new LinkedHashMap<>();
 	private final Set<TestDescriptor> finishedDescriptors = new LinkedHashSet<>();
 
 	TestRun(RunnerTestDescriptor runnerTestDescriptor) {
@@ -70,6 +73,16 @@ RunnerTestDescriptor getRunnerTestDescriptor() {
 		return runnerTestDescriptor;
 	}
 
+	Collection<TestDescriptor> getInProgressTestDescriptorsWithSyntheticStartEvents() {
+		List<TestDescriptor> result = startedDescriptors.entrySet().stream() //
+				.filter(entry -> entry.getValue().equals(EventType.SYNTHETIC)) //
+				.map(Entry::getKey) //
+				.filter(descriptor -> !isFinished(descriptor)) //
+				.collect(toCollection(ArrayList::new));
+		Collections.reverse(result);
+		return result;
+	}
+
 	boolean isDescendantOfRunnerTestDescriptor(TestDescriptor testDescriptor) {
 		return runnerDescendants.contains(testDescriptor);
 	}
@@ -113,12 +126,12 @@ boolean isSkipped(TestDescriptor testDescriptor) {
 		return skippedDescriptors.contains(testDescriptor);
 	}
 
-	void markStarted(TestDescriptor testDescriptor) {
-		startedDescriptors.add(testDescriptor);
+	void markStarted(TestDescriptor testDescriptor, EventType eventType) {
+		startedDescriptors.put(testDescriptor, eventType);
 	}
 
 	boolean isNotStarted(TestDescriptor testDescriptor) {
-		return !startedDescriptors.contains(testDescriptor);
+		return !startedDescriptors.containsKey(testDescriptor);
 	}
 
 	void markFinished(TestDescriptor testDescriptor) {

junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java

@@ -46,10 +46,13 @@
 import org.junit.runner.RunWith;
 import org.junit.runner.Runner;
 import org.junit.runner.notification.RunNotifier;
+import org.junit.runners.Suite;
+import org.junit.runners.Suite.SuiteClasses;
 import org.junit.vintage.engine.samples.junit3.PlainJUnit3TestCaseWithSingleTestWhichFails;
 import org.junit.vintage.engine.samples.junit4.EmptyIgnoredTestCase;
 import org.junit.vintage.engine.samples.junit4.EnclosedJUnit4TestCase;
 import org.junit.vintage.engine.samples.junit4.IgnoredJUnit4TestCase;
+import org.junit.vintage.engine.samples.junit4.IgnoredParameterizedTestCase;
 import org.junit.vintage.engine.samples.junit4.JUnit4SuiteOfSuiteWithIgnoredJUnit4TestCase;
 import org.junit.vintage.engine.samples.junit4.JUnit4SuiteOfSuiteWithJUnit4TestCaseWithAssumptionFailureInBeforeClass;
 import org.junit.vintage.engine.samples.junit4.JUnit4SuiteOfSuiteWithJUnit4TestCaseWithErrorInBeforeClass;
@@ -420,6 +423,23 @@ void executesParameterizedTestCase() {
 			event(engine(), finishedSuccessfully()));
 	}
 
+	@Test
+	void executesIgnoredParameterizedTestCase() {
+		Class<?> testClass = IgnoredParameterizedTestCase.class;
+
+		execute(testClass).assertEventsMatchExactly( //
+			event(engine(), started()), //
+			event(container(testClass), started()), //
+			event(container("[foo]"), started()), //
+			event(test("test[foo]"), skippedWithReason("")), //
+			event(container("[foo]"), finishedSuccessfully()), //
+			event(container("[bar]"), started()), //
+			event(test("test[bar]"), skippedWithReason("")), //
+			event(container("[bar]"), finishedSuccessfully()), //
+			event(container(testClass), finishedSuccessfully()), //
+			event(engine(), finishedSuccessfully()));
+	}
+
 	@Test
 	void executesJUnit4TestCaseWithExceptionThrowingRunner() {
 		Class<?> testClass = JUnit4TestCaseWithExceptionThrowingRunner.class;
@@ -466,7 +486,6 @@ public void run(RunNotifier notifier) {
 
 	@RunWith(DynamicSuiteRunner.class)
 	public static class DynamicTestClass {
-
 	}
 
 	@Test
@@ -483,6 +502,61 @@ void reportsDynamicTestsForUnknownDescriptions() {
 			event(engine(), finishedSuccessfully()));
 	}
 
+	public static class DynamicAndStaticChildrenRunner extends Runner {
+
+		private final Class<?> testClass;
+
+		public DynamicAndStaticChildrenRunner(Class<?> testClass) {
+			this.testClass = testClass;
+		}
+
+		@Override
+		public Description getDescription() {
+			Description suiteDescription = createSuiteDescription(testClass);
+			suiteDescription.addChild(createTestDescription(testClass, "staticTest"));
+			return suiteDescription;
+		}
+
+		@Override
+		public void run(RunNotifier notifier) {
+			Description staticDescription = getDescription().getChildren().get(0);
+			notifier.fireTestStarted(staticDescription);
+			notifier.fireTestFinished(staticDescription);
+			Description dynamicDescription = createTestDescription(testClass, "dynamicTest");
+			notifier.fireTestStarted(dynamicDescription);
+			notifier.fireTestFinished(dynamicDescription);
+		}
+
+	}
+
+	@RunWith(DynamicAndStaticChildrenRunner.class)
+	public static class DynamicAndStaticTestClass {
+	}
+
+	@RunWith(Suite.class)
+	@SuiteClasses(DynamicAndStaticTestClass.class)
+	public static class SuiteWithDynamicAndStaticTestClass {
+	}
+
+	@Test
+	void reportsIntermediateContainersFinishedAfterTheirDynamicChildren() {
+		Class<?> suiteClass = SuiteWithDynamicAndStaticTestClass.class;
+		Class<?> testClass = DynamicAndStaticTestClass.class;
+
+		execute(suiteClass).assertEventsMatchExactly( //
+			event(engine(), started()), //
+			event(container(suiteClass.getName()), started()), //
+			event(container(testClass.getName()), started()), //
+			event(test("staticTest"), started()), //
+			event(test("staticTest"), finishedSuccessfully()), //
+			event(dynamicTestRegistered("dynamicTest")), //
+			event(test("dynamicTest"), started()), //
+			event(test("dynamicTest"), finishedSuccessfully()), //
+			event(container(testClass.getName()), finishedSuccessfully()), //
+			event(container(suiteClass.getName()), finishedSuccessfully()), //
+			event(engine(), finishedSuccessfully()));
+	}
+
 	public static class MisbehavingChildlessRunner extends Runner {
 
 		private final Class<?> testClass;

junit-vintage-engine/src/test/java/org/junit/vintage/engine/samples/junit4/IgnoredParameterizedTestCase.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2015-2019 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
+ *
+ * http://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.vintage.engine.samples.junit4;
+
+import static java.util.Arrays.asList;
+
+import org.junit.Ignore;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * @since 5.4.1
+ */
+@RunWith(Parameterized.class)
+public class IgnoredParameterizedTestCase {
+
+	@Parameters(name = "{0}")
+	public static Iterable<String> parameters() {
+		return asList("foo", "bar");
+	}
+
+	@Parameter
+	public String value;
+
+	@Test
+	@Ignore
+	public void test() {
+		// never called
+	}
+
+}