Replace Node.isLeaf() with explicit tracking of executed descriptors

Fixes #812.

documentation/src/docs/asciidoc/release-notes-5.0.0-M5.adoc

@@ -29,6 +29,8 @@ on GitHub.
   - `junit-platform-commons`: `ReflectionUtils.findAllClassesInClasspathRoot(Path, Predicate, Predicate)`
 * The `junit-platform-surefire-provider` now requires `maven-surefire-plugin` version
   2.20 or higher.
+* The `isLeaf()` method of the `org.junit.platform.engine.support.hierarchical.Node`
+  interface has been removed.
 
 ===== New Features and Improvements
 

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

@@ -94,11 +94,6 @@ protected static <E extends AnnotatedElement> String determineDisplayName(E elem
 
 	// --- Node ----------------------------------------------------------------
 
-	@Override
-	public boolean isLeaf() {
-		return !isContainer();
-	}
-
 	protected SkipResult shouldContainerBeSkipped(JupiterEngineExecutionContext context) {
 		ConditionEvaluationResult evaluationResult = conditionEvaluator.evaluateForContainer(
 			context.getExtensionRegistry(), context.getConfigurationParameters(),

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

@@ -58,11 +58,6 @@ public boolean hasTests() {
 
 	// --- Node ----------------------------------------------------------------
 
-	@Override
-	public boolean isLeaf() {
-		return true;
-	}
-
 	@Override
 	protected void invokeTestMethod(JupiterEngineExecutionContext context, DynamicTestExecutor dynamicTestExecutor) {
 		TestExtensionContext testExtensionContext = (TestExtensionContext) context.getExtensionContext();

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

@@ -53,11 +53,6 @@ public boolean hasTests() {
 
 	// --- Node ----------------------------------------------------------------
 
-	@Override
-	public boolean isLeaf() {
-		return true;
-	}
-
 	@Override
 	public JupiterEngineExecutionContext prepare(JupiterEngineExecutionContext context) throws Exception {
 		ExtensionRegistry registry = populateNewExtensionRegistryFromExtendWith(getTestMethod(),

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

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2015-2017 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 v1.0 which
+ * accompanies this distribution and is available at
+ *
+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+
+package org.junit.platform.engine.support.hierarchical;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.platform.engine.TestDescriptor;
+import org.junit.platform.engine.UniqueId;
+
+/**
+ * @since 1.0
+ */
+class ExecutionTracker {
+
+	private final Set<UniqueId> executedUniqueIds = new HashSet<>();
+
+	void markExecuted(TestDescriptor testDescriptor) {
+		executedUniqueIds.add(testDescriptor.getUniqueId());
+	}
+
+	boolean wasAlreadyExecuted(TestDescriptor testDescriptor) {
+		return executedUniqueIds.contains(testDescriptor.getUniqueId());
+	}
+}

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

@@ -48,11 +48,12 @@ class HierarchicalTestExecutor<C extends EngineExecutionContext> {
 	}
 
 	void execute() {
-		execute(this.rootTestDescriptor, this.rootContext);
+		execute(this.rootTestDescriptor, this.rootContext, new ExecutionTracker());
 	}
 
-	private void execute(TestDescriptor testDescriptor, C parentContext) {
+	private void execute(TestDescriptor testDescriptor, C parentContext, ExecutionTracker tracker) {
 		Node<C> node = asNode(testDescriptor);
+		tracker.markExecuted(testDescriptor);
 
 		C preparedContext;
 		try {
@@ -77,20 +78,19 @@ private void execute(TestDescriptor testDescriptor, C parentContext) {
 			C context = preparedContext;
 			try {
 				context = node.before(context);
-				C dynamicTestContext = context;
+
+				C contextForDynamicChildren = context;
 				context = node.execute(context, dynamicTestDescriptor -> {
 					this.listener.dynamicTestRegistered(dynamicTestDescriptor);
-					execute(dynamicTestDescriptor, dynamicTestContext);
+					execute(dynamicTestDescriptor, contextForDynamicChildren, tracker);
 				});
 
-				// 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);
-					}
-				}
+				C contextForStaticChildren = context;
+				// @formatter:off
+				testDescriptor.getChildren().stream()
+						.filter(child -> !tracker.wasAlreadyExecuted(child))
+						.forEach(child -> execute(child, contextForStaticChildren, tracker));
+				// @formatter:on
 			}
 			finally {
 				node.after(context);

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

@@ -29,15 +29,6 @@
 @API(Experimental)
 public interface Node<C extends EngineExecutionContext> {
 
-	/**
-	 * Determine if this {@code Node} is a leaf in the hierarchy.
-	 *
-	 * <p>The default implementation returns {@code false}.
-	 */
-	default boolean isLeaf() {
-		return false;
-	}
-
 	/**
 	 * Prepare the supplied {@code context} prior to execution.
 	 *

platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/DemoHierarchicalContainerDescriptor.java

@@ -46,11 +46,6 @@ public Type getType() {
 		return Type.CONTAINER;
 	}
 
-	@Override
-	public boolean isLeaf() {
-		return false;
-	}
-
 	@Override
 	public boolean hasTests() {
 		return true;

platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/DemoHierarchicalEngineDescriptor.java

@@ -50,9 +50,4 @@ public DemoEngineExecutionContext before(DemoEngineExecutionContext context) thr
 		return context;
 	}
 
-	@Override
-	public boolean isLeaf() {
-		return false;
-	}
-
 }

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

@@ -44,11 +44,6 @@ public Type getType() {
 		return this.executeBlock != null ? Type.TEST : Type.CONTAINER;
 	}
 
-	@Override
-	public boolean isLeaf() {
-		return isTest();
-	}
-
 	public void markSkipped(String reason) {
 		this.skipped = true;
 		this.skippedReason = reason;

platform-tests/src/test/java/org/junit/platform/engine/support/hierarchical/HierarchicalTestExecutorTests.java

@@ -407,12 +407,6 @@ protected MyContainer(UniqueId uniqueId) {
 		public Type getType() {
 			return Type.CONTAINER;
 		}
-
-		@Override
-		public boolean isLeaf() {
-			return isTest();
-		}
-
 	}
 
 	private static class MyLeaf extends AbstractTestDescriptor implements Node<MyEngineExecutionContext> {
@@ -431,11 +425,6 @@ public MyEngineExecutionContext execute(MyEngineExecutionContext context,
 		public Type getType() {
 			return Type.TEST;
 		}
-
-		@Override
-		public boolean isLeaf() {
-			return isTest();
-		}
 	}
 
 	private static class MyExecutor extends HierarchicalTestExecutor<MyEngineExecutionContext> {