Optimize search for existence of @Nested test classes

Author: marcphilipp

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicates.java

@@ -15,14 +15,15 @@
 import static org.junit.platform.commons.support.ModifierSupport.isAbstract;
 import static org.junit.platform.commons.support.ModifierSupport.isNotAbstract;
 import static org.junit.platform.commons.support.ModifierSupport.isNotPrivate;
+import static org.junit.platform.commons.util.ReflectionUtils.isMethodPresent;
+import static org.junit.platform.commons.util.ReflectionUtils.isNestedClassPresent;
 
 import java.lang.reflect.Method;
 import java.util.function.Predicate;
 
 import org.apiguardian.api.API;
 import org.junit.jupiter.api.ClassTemplate;
 import org.junit.jupiter.api.Nested;
-import org.junit.platform.commons.support.ReflectionSupport;
 import org.junit.platform.commons.util.ReflectionUtils;
 import org.junit.platform.engine.DiscoveryIssue;
 import org.junit.platform.engine.support.descriptor.ClassSource;
@@ -79,12 +80,11 @@ public boolean isValidStandaloneTestClass(Class<?> candidate) {
 	}
 
 	private boolean hasTestOrTestFactoryOrTestTemplateMethods(Class<?> candidate) {
-		return ReflectionUtils.isMethodPresent(candidate, this.isTestOrTestFactoryOrTestTemplateMethod);
+		return isMethodPresent(candidate, this.isTestOrTestFactoryOrTestTemplateMethod);
 	}
 
 	private boolean hasNestedTests(Class<?> candidate) {
-		// TODO [#242] Add ReflectionSupport.isNestedClassPresent(candidate, TestClassPredicates::isValidNestedTestClass)
-		return !ReflectionSupport.findNestedClasses(candidate, this.isAnnotatedWithNestedAndValid).isEmpty();
+		return isNestedClassPresent(candidate, this.isAnnotatedWithNestedAndValid);
 	}
 
 	private static Condition<Class<?>> isNotPrivateUnlessAbstract(String prefix, DiscoveryIssueReporter issueReporter) {

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

@@ -1236,10 +1236,35 @@ public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?
 		Preconditions.notNull(predicate, "Predicate must not be null");
 
 		Set<Class<?>> candidates = new LinkedHashSet<>();
-		findNestedClasses(clazz, predicate, candidates);
+		visitNestedClasses(clazz, predicate, nestedClass -> {
+			candidates.add(nestedClass);
+			return true;
+		});
 		return Collections.unmodifiableList(new ArrayList<>(candidates));
 	}
 
+	/**
+	 * Determine if a nested class within the supplied class, or inherited by the
+	 * supplied class, that conforms to the supplied predicate is present.
+	 *
+	 * <p>This method does <strong>not</strong> search for nested classes
+	 * recursively.
+	 *
+	 * @param clazz the class to be searched; never {@code null}
+	 * @param predicate the predicate against which the list of nested classes is
+	 * checked; never {@code null}
+	 * @return {@code true} if such a nested class is present
+	 * @throws JUnitException if a cycle is detected within an inner class hierarchy
+	 */
+	@API(status = INTERNAL, since = "1.13")
+	public static boolean isNestedClassPresent(Class<?> clazz, Predicate<Class<?>> predicate) {
+		Preconditions.notNull(clazz, "Class must not be null");
+		Preconditions.notNull(predicate, "Predicate must not be null");
+
+		boolean visitorWasNotCalled = visitNestedClasses(clazz, predicate, __ -> false);
+		return !visitorWasNotCalled;
+	}
+
 	/**
 	 * since 1.10
 	 * @see org.junit.platform.commons.support.ReflectionSupport#streamNestedClasses(Class, Predicate)
@@ -1248,9 +1273,10 @@ public static Stream<Class<?>> streamNestedClasses(Class<?> clazz, Predicate<Cla
 		return findNestedClasses(clazz, predicate).stream();
 	}
 
-	private static void findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate, Set<Class<?>> candidates) {
+	private static boolean visitNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
+			Visitor<Class<?>> visitor) {
 		if (!isSearchable(clazz)) {
-			return;
+			return true;
 		}
 
 		if (isInnerClass(clazz) && predicate.test(clazz)) {
@@ -1262,7 +1288,10 @@ private static void findNestedClasses(Class<?> clazz, Predicate<Class<?>> predic
 			for (Class<?> nestedClass : clazz.getDeclaredClasses()) {
 				if (predicate.test(nestedClass)) {
 					detectInnerClassCycle(nestedClass);
-					candidates.add(nestedClass);
+					boolean shouldContinue = visitor.accept(nestedClass);
+					if (!shouldContinue) {
+						return false;
+					}
 				}
 			}
 		}
@@ -1271,12 +1300,20 @@ private static void findNestedClasses(Class<?> clazz, Predicate<Class<?>> predic
 		}
 
 		// Search class hierarchy
-		findNestedClasses(clazz.getSuperclass(), predicate, candidates);
+		boolean shouldContinue = visitNestedClasses(clazz.getSuperclass(), predicate, visitor);
+		if (!shouldContinue) {
+			return false;
+		}
 
 		// Search interface hierarchy
 		for (Class<?> ifc : clazz.getInterfaces()) {
-			findNestedClasses(ifc, predicate, candidates);
+			shouldContinue = visitNestedClasses(ifc, predicate, visitor);
+			if (!shouldContinue) {
+				return false;
+			}
 		}
+
+		return true;
 	}
 
 	/**
@@ -2073,4 +2110,14 @@ private static boolean getLegacySearchSemanticsFlag() {
 		return isTrue;
 	}
 
+	private interface Visitor<T> {
+
+		/**
+		 * @return {@code true} if the visitor should continue searching;
+		 * {@code false} if the visitor should stop
+		 */
+		boolean accept(T value);
+
+	}
+
 }

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

@@ -1036,24 +1036,43 @@ void findNestedClassesPreconditions() {
 			// @formatter:on
 		}
 
+		@Test
+		void isNestedClassPresentPreconditions() {
+			// @formatter:off
+			assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(null, null));
+			assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(null, clazz -> true));
+			assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(getClass(), null));
+			// @formatter:on
+		}
+
 		@Test
 		void findNestedClasses() {
 			// @formatter:off
 			assertThat(findNestedClasses(Object.class)).isEmpty();
+			assertThat(isNestedClassPresent(Object.class)).isFalse();
 
 			assertThat(findNestedClasses(ClassWithNestedClasses.class))
 					.containsOnly(Nested1.class, Nested2.class, Nested3.class);
+			assertThat(isNestedClassPresent(ClassWithNestedClasses.class))
+					.isTrue();
 
 			assertThat(ReflectionUtils.findNestedClasses(ClassWithNestedClasses.class, clazz -> clazz.getName().contains("1")))
 					.containsExactly(Nested1.class);
+			assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, clazz -> clazz.getName().contains("1")))
+					.isTrue();
 
 			assertThat(ReflectionUtils.findNestedClasses(ClassWithNestedClasses.class, ReflectionUtils::isStatic))
 					.containsExactly(Nested3.class);
+			assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, ReflectionUtils::isStatic))
+					.isTrue();
 
 			assertThat(findNestedClasses(ClassExtendingClassWithNestedClasses.class))
 					.containsOnly(Nested1.class, Nested2.class, Nested3.class, Nested4.class, Nested5.class);
+			assertThat(isNestedClassPresent(ClassExtendingClassWithNestedClasses.class))
+					.isTrue();
 
 			assertThat(findNestedClasses(ClassWithNestedClasses.Nested1.class)).isEmpty();
+			assertThat(isNestedClassPresent(ClassWithNestedClasses.Nested1.class)).isFalse();
 			// @formatter:on
 		}
 
@@ -1064,26 +1083,39 @@ void findNestedClasses() {
 		void findNestedClassesWithSeeminglyRecursiveHierarchies() {
 			assertThat(findNestedClasses(AbstractOuterClass.class))//
 					.containsExactly(AbstractOuterClass.InnerClass.class);
+			assertThat(isNestedClassPresent(AbstractOuterClass.class))//
+					.isTrue();
 
 			// 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();
+			assertThat(ReflectionUtils.isNestedClassPresent(OuterClass.class, clazz -> false)).isFalse();
+
 			// RecursiveInnerInnerClass is part of a recursive hierarchy, but the non-matching
 			// predicate should prevent cycle detection.
 			assertThat(ReflectionUtils.findNestedClasses(RecursiveInnerInnerClass.class, clazz -> false)).isEmpty();
+			assertThat(ReflectionUtils.isNestedClassPresent(RecursiveInnerInnerClass.class, clazz -> false)).isFalse();
 
 			// Sibling types don't actually result in cycles.
 			assertThat(findNestedClasses(StaticNestedSiblingClass.class))//
 					.containsExactly(AbstractOuterClass.InnerClass.class);
+			assertThat(isNestedClassPresent(StaticNestedSiblingClass.class))//
+					.isTrue();
 			assertThat(findNestedClasses(InnerSiblingClass.class))//
 					.containsExactly(AbstractOuterClass.InnerClass.class);
+			assertThat(isNestedClassPresent(InnerSiblingClass.class))//
+					.isTrue();
 
 			// Interfaces with static nested classes
 			assertThat(findNestedClasses(OuterClassImplementingInterface.class))//
 					.containsExactly(InnerClassImplementingInterface.class, Nested4.class);
+			assertThat(isNestedClassPresent(OuterClassImplementingInterface.class))//
+					.isTrue();
 			assertThat(findNestedClasses(InnerClassImplementingInterface.class))//
 					.containsExactly(Nested4.class);
+			assertThat(isNestedClassPresent(InnerClassImplementingInterface.class))//
+					.isTrue();
 		}
 
 		/**
@@ -1105,6 +1137,10 @@ private static List<Class<?>> findNestedClasses(Class<?> clazz) {
 			return ReflectionUtils.findNestedClasses(clazz, c -> true);
 		}
 
+		private static boolean isNestedClassPresent(Class<?> clazz) {
+			return ReflectionUtils.isNestedClassPresent(clazz, c -> true);
+		}
+
 		private void assertNestedCycle(Class<?> from, Class<?> to) {
 			assertNestedCycle(from, from, to);
 		}