Stop further suite discovery when encountering a cycle (#3116)

It is relatively easy to create a test suite that contains itself by
accident. E.g:

```java
package org.example.tag;

@tag("fast")
public class FastTest {
    @test
    public void test() {
        System.out.println("I am fast test");
    }
}
```
```java
package org.example.tag;

@tag("slow")
public class SlowTest {
    @test
    public void test() {
        System.out.println("I am slow test");
    }
}
```
```java
package org.example.tag;

@SelectPackages("org.example.tag")
@IncludeTags("fast")
@suite
public class TestSuite {
}
```

This configuration will result in a a long stack trace with a message
explaining that the suite contained a cycle.

```
Configuration error: The suite configuration may not contain a cycle [[engine:junit-platform-suite]/[suite:org.example.tag.TestSuite]/[engine:junit-platform-suite]/[suite:org.example.tag.TestSuite]]
```

And it is is not immediately apparent how a user should resolve this
problem. This example could be resolved by explicitly excluding the
suite engine by adding `@ExcludeEngine("junit-platform-suite")`. This
prevents further discovery.  But when multiple suites are involved this
may not be right action.

Fortunately the suite itself is aware of the cycle and as stopping
further discovery is the only reasonable action. This means that this
can be done without any sort of explicit configuration.

Fixes #3115.

Author: mpkorstanje

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

@@ -27,6 +27,7 @@ JUnit repository on GitHub.
   avoid parsing unique IDs unnecessarily during test execution.
 * Support for limiting the `max-pool-size` for parallel execution via a configuration
   parameter
+* Quietly stop further suite discovery when encountering a cycle in a test suite
 
 
 [[release-notes-5.9.2-junit-jupiter]]

documentation/src/docs/asciidoc/user-guide/advanced-topics/junit-platform-suite-engine.adoc

@@ -20,7 +20,7 @@ you need _at least one_ other test engine and its dependencies on the classpath.
   `TestEngine` API for declarative test suites
 
 NOTE: Both of the required dependencies are aggregated in the `junit-platform-suite`
-artifact which can be declard in _test_ scope instead of declaring explicit dependencies
+artifact which can be declared in _test_ scope instead of declaring explicit dependencies
 on `junit-platform-suite-api` and `junit-platform-suite-engine`.
 
 [[junit-platform-suite-engine-setup-transitive-dependencies]]

junit-platform-suite-engine/src/main/java/org/junit/platform/suite/engine/ClassSelectorResolver.java

@@ -16,6 +16,8 @@
 import java.util.Optional;
 import java.util.function.Predicate;
 
+import org.junit.platform.commons.logging.Logger;
+import org.junit.platform.commons.logging.LoggerFactory;
 import org.junit.platform.commons.util.ReflectionUtils;
 import org.junit.platform.engine.ConfigurationParameters;
 import org.junit.platform.engine.TestDescriptor;
@@ -30,6 +32,8 @@
  */
 final class ClassSelectorResolver implements SelectorResolver {
 
+	private static final Logger log = LoggerFactory.getLogger(ClassSelectorResolver.class);
+
 	private static final IsSuiteClass isSuiteClass = new IsSuiteClass();
 
 	private final Predicate<String> classNameFilter;
@@ -94,7 +98,31 @@ private static Resolution toResolution(Optional<SuiteTestDescriptor> suite) {
 
 	private Optional<SuiteTestDescriptor> newSuiteDescriptor(Class<?> suiteClass, TestDescriptor parent) {
 		UniqueId id = parent.getUniqueId().append(SuiteTestDescriptor.SEGMENT_TYPE, suiteClass.getName());
+		if (containsCycle(id)) {
+			log.config(() -> createConfigContainsCycleMessage(suiteClass, id));
+			return Optional.empty();
+		}
+
 		return Optional.of(new SuiteTestDescriptor(id, suiteClass, configurationParameters));
 	}
 
+	private static boolean containsCycle(UniqueId id) {
+		List<Segment> segments = id.getSegments();
+		List<Segment> engineAndSuiteSegment = segments.subList(segments.size() - 2, segments.size());
+		List<Segment> ancestorSegments = segments.subList(0, segments.size() - 2);
+		for (int i = 0; i < ancestorSegments.size() - 1; i++) {
+			List<Segment> candidate = ancestorSegments.subList(i, i + 2);
+			if (engineAndSuiteSegment.equals(candidate)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	private static String createConfigContainsCycleMessage(Class<?> suiteClass, UniqueId suiteId) {
+		return String.format(
+			"The suite configuration of [%s] resulted in a cycle [%s] and will not be discovered a second time.",
+			suiteClass.getName(), suiteId);
+	}
+
 }

junit-platform-suite-engine/src/main/java/org/junit/platform/suite/engine/SuiteTestDescriptor.java

@@ -10,14 +10,9 @@
 
 package org.junit.platform.suite.engine;
 
-import static java.util.function.Function.identity;
-import static java.util.stream.Collectors.counting;
-import static java.util.stream.Collectors.groupingBy;
 import static org.junit.platform.commons.util.AnnotationUtils.findAnnotation;
 import static org.junit.platform.suite.commons.SuiteLauncherDiscoveryRequestBuilder.request;
 
-import java.util.function.Supplier;
-
 import org.junit.platform.commons.JUnitException;
 import org.junit.platform.commons.util.Preconditions;
 import org.junit.platform.commons.util.StringUtils;
@@ -26,7 +21,6 @@
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.TestExecutionResult;
 import org.junit.platform.engine.UniqueId;
-import org.junit.platform.engine.UniqueId.Segment;
 import org.junit.platform.engine.discovery.DiscoverySelectors;
 import org.junit.platform.engine.support.descriptor.AbstractTestDescriptor;
 import org.junit.platform.engine.support.descriptor.ClassSource;
@@ -60,28 +54,12 @@ final class SuiteTestDescriptor extends AbstractTestDescriptor {
 	private SuiteLauncher launcher;
 
 	SuiteTestDescriptor(UniqueId id, Class<?> suiteClass, ConfigurationParameters configurationParameters) {
-		super(requireNoCycles(id), getSuiteDisplayName(suiteClass), ClassSource.from(suiteClass));
+		super(id, getSuiteDisplayName(suiteClass), ClassSource.from(suiteClass));
 		this.configurationParameters = configurationParameters;
 		this.failIfNoTests = getFailIfNoTests(suiteClass);
 		this.suiteClass = suiteClass;
 	}
 
-	private static UniqueId requireNoCycles(UniqueId id) {
-		// @formatter:off
-		boolean containsCycle = id.getSegments().stream()
-				.filter(segment -> SuiteTestDescriptor.SEGMENT_TYPE.equals(segment.getType()))
-				.map(Segment::getValue)
-				.collect(groupingBy(identity(), counting()))
-				.values()
-				.stream()
-				.anyMatch(count -> count > 1);
-		// @formatter:on
-		Supplier<String> message = () -> String.format(
-			"Configuration error: The suite configuration may not contain a cycle [%s]", id);
-		Preconditions.condition(!containsCycle, message);
-		return id;
-	}
-
 	private static Boolean getFailIfNoTests(Class<?> suiteClass) {
 		// @formatter:off
 		return findAnnotation(suiteClass, Suite.class)

platform-tests/src/test/java/org/junit/platform/suite/engine/SuiteEngineTests.java

@@ -22,6 +22,7 @@
 import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure;
 import static org.junit.platform.testkit.engine.EventConditions.test;
 import static org.junit.platform.testkit.engine.TestExecutionResultConditions.instanceOf;
+import static org.junit.platform.testkit.engine.TestExecutionResultConditions.message;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.engine.descriptor.ClassTestDescriptor;
@@ -39,7 +40,9 @@
 import org.junit.platform.suite.engine.testcases.SingleTestTestCase;
 import org.junit.platform.suite.engine.testcases.TaggedTestTestCase;
 import org.junit.platform.suite.engine.testsuites.AbstractSuite;
+import org.junit.platform.suite.engine.testsuites.CyclicSuite;
 import org.junit.platform.suite.engine.testsuites.DynamicSuite;
+import org.junit.platform.suite.engine.testsuites.EmptyCyclicSuite;
 import org.junit.platform.suite.engine.testsuites.EmptyDynamicTestSuite;
 import org.junit.platform.suite.engine.testsuites.EmptyDynamicTestWithFailIfNoTestFalseSuite;
 import org.junit.platform.suite.engine.testsuites.EmptyTestCaseSuite;
@@ -50,6 +53,7 @@
 import org.junit.platform.suite.engine.testsuites.SelectClassesSuite;
 import org.junit.platform.suite.engine.testsuites.SuiteDisplayNameSuite;
 import org.junit.platform.suite.engine.testsuites.SuiteSuite;
+import org.junit.platform.suite.engine.testsuites.ThreePartCyclicSuite;
 import org.junit.platform.testkit.engine.EngineTestKit;
 
 /**
@@ -350,4 +354,42 @@ void pruneAfterPostDiscoveryFilters() {
 		// @formatter:on
 	}
 
+	@Test
+	void cyclicSuite() {
+		// @formatter:off
+		EngineTestKit.engine(ENGINE_ID)
+				.selectors(selectClass(CyclicSuite.class))
+				.execute()
+				.allEvents()
+				.assertThatEvents()
+				.haveExactly(1, event(test(SingleTestTestCase.class.getName()), finishedSuccessfully()));
+		// @formatter:on
+	}
+
+	@Test
+	void emptyCyclicSuite() {
+		// @formatter:off
+		EngineTestKit.engine(ENGINE_ID)
+				.selectors(selectClass(EmptyCyclicSuite.class))
+				.execute()
+				.allEvents()
+				.assertThatEvents()
+				.haveExactly(1, event(container(EmptyCyclicSuite.class), finishedWithFailure(message(
+						"Suite [org.junit.platform.suite.engine.testsuites.EmptyCyclicSuite] did not discover any tests"
+				))));
+		// @formatter:on
+	}
+
+	@Test
+	void threePartCyclicSuite() {
+		// @formatter:off
+		EngineTestKit.engine(ENGINE_ID)
+				.selectors(selectClass(ThreePartCyclicSuite.PartA.class))
+				.execute()
+				.allEvents()
+				.assertThatEvents()
+				.haveExactly(1, event(test(SingleTestTestCase.class.getName()), finishedSuccessfully()));
+		// @formatter:on
+	}
+
 }

platform-tests/src/test/java/org/junit/platform/suite/engine/SuiteTestDescriptorTests.java

@@ -25,14 +25,12 @@
 import org.junit.jupiter.engine.descriptor.ClassTestDescriptor;
 import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor;
 import org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor;
-import org.junit.platform.commons.JUnitException;
 import org.junit.platform.commons.PreconditionViolationException;
 import org.junit.platform.engine.ConfigurationParameters;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.UniqueId;
 import org.junit.platform.suite.api.Suite;
 import org.junit.platform.suite.engine.testcases.SingleTestTestCase;
-import org.junit.platform.suite.engine.testsuites.CyclicSuite;
 import org.junit.platform.suite.engine.testsuites.SelectClassesSuite;
 
 /**
@@ -90,21 +88,6 @@ void discoveryPlanCanNotBeModifiedAfterDiscovery() {
 		});
 	}
 
-	@Test
-	void suitesMayNotContainACycle() {
-		// @formatter:off
-		UniqueId expectedCycle = suiteId
-				.append("engine", SuiteEngineDescriptor.ENGINE_ID)
-				.append(SuiteTestDescriptor.SEGMENT_TYPE, CyclicSuite.class.getName())
-				.append("engine", SuiteEngineDescriptor.ENGINE_ID)
-				.append(SuiteTestDescriptor.SEGMENT_TYPE, CyclicSuite.class.getName());
-		// @formatter:on
-		suite.addDiscoveryRequestFrom(CyclicSuite.class);
-		JUnitException exception = assertThrows(JUnitException.class, suite::discover);
-		assertEquals("Configuration error: The suite configuration may not contain a cycle [" + expectedCycle + "]",
-			exception.getCause().getCause().getCause().getMessage());
-	}
-
 	@Test
 	void suiteMayRegisterTests() {
 		assertTrue(suite.mayRegisterTests());

platform-tests/src/test/java/org/junit/platform/suite/engine/testsuites/CyclicSuite.java

@@ -13,12 +13,13 @@
 import org.junit.platform.suite.api.IncludeClassNamePatterns;
 import org.junit.platform.suite.api.SelectClasses;
 import org.junit.platform.suite.api.Suite;
+import org.junit.platform.suite.engine.testcases.SingleTestTestCase;
 
 /**
  * @since 1.8
  */
 @Suite
 @IncludeClassNamePatterns(".*")
-@SelectClasses(CyclicSuite.class)
+@SelectClasses({ CyclicSuite.class, SingleTestTestCase.class })
 public class CyclicSuite {
 }

platform-tests/src/test/java/org/junit/platform/suite/engine/testsuites/EmptyCyclicSuite.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2015-2022 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.suite.engine.testsuites;
+
+import org.junit.platform.suite.api.IncludeClassNamePatterns;
+import org.junit.platform.suite.api.SelectClasses;
+import org.junit.platform.suite.api.Suite;
+
+/**
+ * @since 1.9.2
+ */
+@Suite
+@IncludeClassNamePatterns(".*")
+@SelectClasses(EmptyCyclicSuite.class)
+public class EmptyCyclicSuite {
+}

platform-tests/src/test/java/org/junit/platform/suite/engine/testsuites/ThreePartCyclicSuite.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright 2015-2022 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.suite.engine.testsuites;
+
+import org.junit.platform.suite.api.IncludeClassNamePatterns;
+import org.junit.platform.suite.api.SelectClasses;
+import org.junit.platform.suite.api.Suite;
+import org.junit.platform.suite.engine.testcases.SingleTestTestCase;
+
+/**
+ * @since 1.9.2
+ */
+public class ThreePartCyclicSuite {
+
+	@Suite
+	@IncludeClassNamePatterns(".*")
+	@SelectClasses({ PartB.class })
+	public static class PartA {
+
+	}
+
+	@Suite
+	@IncludeClassNamePatterns(".*")
+	@SelectClasses({ PartC.class })
+	public static class PartB {
+
+	}
+
+	@Suite
+	@IncludeClassNamePatterns(".*")
+	@SelectClasses({ PartB.class, SingleTestTestCase.class })
+	public static class PartC {
+
+	}
+}