Avoid inner class cycle detection for non-matching predicate

JUnit 5.6 introduced inner class cycle detection in
ReflectionSupport.findNestedClasses() (see #2039). Unfortunately, the
cycle detection introduced a regression for test classes that contain
classes with inner class cycles when those inner classes do not match
the predicate supplied to findNestedClasses() (e.g., "is annotated with
@nested"). Consequently, an exception is thrown for any inner class
cycled detected, even if the cycle would not adversely affect test
discovery or execution.

This commit fixes this regression by avoiding inner class cycle
detection for candidate classes that do match the predicate. For
example, within JUnit Jupiter inner class cycle detection is now only
performed for inner classes annotated with @nested.

Fixes #2249

Author: sbrannen

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

@@ -5,8 +5,8 @@
 
 *Scope:* Bug fixes since 5.6.1
 
-For a complete list of all _closed_ issues and pull requests for this release, consult
-the link:{junit5-repo}+/milestone/48?closed=1+[5.6.2] milestone page in the JUnit repository
+For a complete list of all _closed_ issues and pull requests for this release, consult the
+link:{junit5-repo}+/milestone/48?closed=1+[5.6.2] milestone page in the JUnit repository
 on GitHub.
 
 
@@ -15,15 +15,19 @@ on GitHub.
 
 ==== Bug Fixes
 
-* ❓
+* `ReflectionSupport.findNestedClasses()` no longer detects inner class cycles for classes
+  that do not match the supplied `Predicate`. For example, JUnit Jupiter no longer throws
+  an exception if an inner class cycle is detected in a nested class hierarchy whose inner
+  classes are not annotated with `@Nested`.
 
 
 [[release-notes-5.6.2-junit-jupiter]]
 === JUnit Jupiter
 
 ==== Bug Fixes
 
-* ❓
+* Test discovery no longer halts with an exception for inner class hierarchies that form a
+  cycle if such inner classes are not annotated with `@Nested`.
 
 
 [[release-notes-5.6.2-junit-vintage]]

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/discovery/predicates/IsTestClassWithTestsTests.java

@@ -84,6 +84,15 @@ void privateStaticTestClassEvaluatesToFalse() {
 		assertFalse(isTestClassWithTests.test(PrivateStaticTestCase.class));
 	}
 
+	/**
+	 * @see https://github.com/junit-team/junit5/issues/2249
+	 */
+	@Test
+	void recursiveHierarchies() {
+		assertTrue(isTestClassWithTests.test(OuterClass.class));
+		assertFalse(isTestClassWithTests.test(OuterClass.RecursiveInnerClass.class));
+	}
+
 	// -------------------------------------------------------------------------
 
 	private class PrivateClassWithTestMethod {
@@ -143,6 +152,22 @@ void test() {
 		}
 	}
 
+	static class OuterClass {
+
+		@Nested
+		class InnerClass {
+
+			@Test
+			void test() {
+			}
+		}
+
+		// Intentionally commented out so that RecursiveInnerClass is NOT a candidate test class
+		// @Nested
+		class RecursiveInnerClass extends OuterClass {
+		}
+	}
+
 }
 
 // -----------------------------------------------------------------------------

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

@@ -1004,34 +1004,38 @@ public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?
 		Preconditions.notNull(predicate, "Predicate must not be null");
 
 		Set<Class<?>> candidates = new LinkedHashSet<>();
-		findNestedClasses(clazz, candidates);
-		return candidates.stream().filter(predicate).collect(toUnmodifiableList());
+		findNestedClasses(clazz, predicate, candidates);
+		return Collections.unmodifiableList(new ArrayList<>(candidates));
 	}
 
-	private static void findNestedClasses(Class<?> clazz, Set<Class<?>> candidates) {
+	private static void findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate, Set<Class<?>> candidates) {
 		if (!isSearchable(clazz)) {
 			return;
 		}
 
-		detectInnerClassCycle(clazz);
+		if (isInnerClass(clazz) && predicate.test(clazz)) {
+			detectInnerClassCycle(clazz);
+		}
 
 		try {
 			// Candidates in current class
 			for (Class<?> nestedClass : clazz.getDeclaredClasses()) {
-				detectInnerClassCycle(nestedClass);
-				candidates.add(nestedClass);
+				if (predicate.test(nestedClass)) {
+					detectInnerClassCycle(nestedClass);
+					candidates.add(nestedClass);
+				}
 			}
 		}
 		catch (NoClassDefFoundError error) {
 			logger.debug(error, () -> "Failed to retrieve declared classes for " + clazz.getName());
 		}
 
 		// Search class hierarchy
-		findNestedClasses(clazz.getSuperclass(), candidates);
+		findNestedClasses(clazz.getSuperclass(), predicate, candidates);
 
 		// Search interface hierarchy
 		for (Class<?> ifc : clazz.getInterfaces()) {
-			findNestedClasses(ifc, candidates);
+			findNestedClasses(ifc, predicate, candidates);
 		}
 	}
 

platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java

@@ -690,6 +690,14 @@ void findNestedClassesWithSeeminglyRecursiveHierarchies() {
 		assertThat(findNestedClasses(AbstractOuterClass.class))//
 				.containsExactly(AbstractOuterClass.InnerClass.class);
 
+		// OuterClass contains recursive hierarchies, but the non-matching
+		// predicate should prevent cycle detection.
+		// See https://github.com/junit-team/junit5/issues/2249
+		assertThat(ReflectionUtils.findNestedClasses(OuterClass.class, clazz -> false)).isEmpty();
+		// RecursiveInnerInnerClass is part of a recursive hierarchy, but the non-matching
+		// predicate should prevent cycle detection.
+		assertThat(ReflectionUtils.findNestedClasses(RecursiveInnerInnerClass.class, clazz -> false)).isEmpty();
+
 		// Sibling types don't actually result in cycles.
 		assertThat(findNestedClasses(StaticNestedSiblingClass.class))//
 				.containsExactly(AbstractOuterClass.InnerClass.class);