diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.13.2.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.13.2.adoc index 667da2449d27..5dc1cb8968ed 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.13.2.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.13.2.adoc @@ -35,7 +35,8 @@ on GitHub. [[release-notes-5.13.2-junit-jupiter-bug-fixes]] ==== Bug Fixes -* ❓ +* Stop reporting discovery issues for cyclic inner class hierarchies not annotated with + `@Nested`. [[release-notes-5.13.2-junit-jupiter-deprecations-and-breaking-changes]] ==== Deprecations and Breaking Changes diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassSelectorResolver.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassSelectorResolver.java index 4470da5a543e..7495b141432b 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassSelectorResolver.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassSelectorResolver.java @@ -17,9 +17,9 @@ import static org.junit.jupiter.engine.descriptor.NestedClassTestDescriptor.getEnclosingTestClasses; import static org.junit.platform.commons.support.HierarchyTraversalMode.TOP_DOWN; import static org.junit.platform.commons.support.ReflectionSupport.findMethods; -import static org.junit.platform.commons.support.ReflectionSupport.streamNestedClasses; import static org.junit.platform.commons.util.FunctionUtils.where; import static org.junit.platform.commons.util.ReflectionUtils.isInnerClass; +import static org.junit.platform.commons.util.ReflectionUtils.streamNestedClasses; import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUniqueId; import static org.junit.platform.engine.support.discovery.SelectorResolver.Resolution.unresolved; @@ -46,6 +46,7 @@ import org.junit.jupiter.engine.discovery.predicates.TestClassPredicates; import org.junit.platform.commons.support.ReflectionSupport; import org.junit.platform.commons.util.ReflectionUtils; +import org.junit.platform.commons.util.ReflectionUtils.CycleErrorHandling; import org.junit.platform.engine.DiscoveryIssue; import org.junit.platform.engine.DiscoveryIssue.Severity; import org.junit.platform.engine.DiscoverySelector; @@ -289,9 +290,13 @@ private Supplier> expansionCallback(TestDescrip Stream methods = findMethods(testClass, this.predicates.isTestOrTestFactoryOrTestTemplateMethod, TOP_DOWN).stream() // .map(method -> selectMethod(testClasses, method)); - Stream nestedClasses = streamNestedClasses(testClass, - this.predicates.isAnnotatedWithNested.or(ReflectionUtils::isInnerClass)) // - .map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass)); + Stream> annotatedNestedClasses = streamNestedClasses(testClass, + this.predicates.isAnnotatedWithNested); + Stream> notAnnotatedInnerClasses = streamNestedClasses(testClass, + this.predicates.isAnnotatedWithNested.negate().and(ReflectionUtils::isInnerClass), + CycleErrorHandling.ABORT_VISIT); + var nestedClasses = Stream.concat(annotatedNestedClasses, notAnnotatedInnerClasses) // + .map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass)); return Stream.concat(methods, nestedClasses).collect( toCollection((Supplier>) LinkedHashSet::new)); }; diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicates.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicates.java index 5e48b659f46b..c9ee44d5bbca 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicates.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicates.java @@ -20,12 +20,15 @@ import static org.junit.platform.commons.util.ReflectionUtils.isNestedClassPresent; import java.lang.reflect.Method; +import java.util.HashSet; +import java.util.Set; 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.util.ReflectionUtils; +import org.junit.platform.commons.util.ReflectionUtils.CycleErrorHandling; import org.junit.platform.engine.DiscoveryIssue; import org.junit.platform.engine.support.descriptor.ClassSource; import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter; @@ -65,9 +68,16 @@ public TestClassPredicates(DiscoveryIssueReporter issueReporter) { } public boolean looksLikeIntendedTestClass(Class candidate) { - return this.isAnnotatedWithClassTemplate.test(candidate) // - || hasTestOrTestFactoryOrTestTemplateMethods(candidate) // - || hasNestedTests(candidate); + return looksLikeIntendedTestClass(candidate, new HashSet<>()); + } + + private boolean looksLikeIntendedTestClass(Class candidate, Set> seen) { + if (seen.add(candidate)) { + return this.isAnnotatedWithClassTemplate.test(candidate) // + || hasTestOrTestFactoryOrTestTemplateMethods(candidate) // + || hasNestedTests(candidate, seen); + } + return false; } public boolean isValidNestedTestClass(Class candidate) { @@ -84,15 +94,17 @@ private boolean hasTestOrTestFactoryOrTestTemplateMethods(Class candidate) { return isMethodPresent(candidate, this.isTestOrTestFactoryOrTestTemplateMethod); } - private boolean hasNestedTests(Class candidate) { + private boolean hasNestedTests(Class candidate, Set> seen) { + var hasAnnotatedClass = isNestedClassPresent(candidate, this.isAnnotatedWithNested, + CycleErrorHandling.THROW_EXCEPTION); + if (hasAnnotatedClass) { + return true; + } return isNestedClassPresent( // candidate, // - isNotSame(candidate).and( - this.isAnnotatedWithNested.or(it -> isInnerClass(it) && looksLikeIntendedTestClass(it)))); - } - - private static Predicate> isNotSame(Class candidate) { - return clazz -> candidate != clazz; + it -> isInnerClass(it) && looksLikeIntendedTestClass(it, seen), // + CycleErrorHandling.ABORT_VISIT // + ); } private static Condition> isNotPrivateUnlessAbstract(String prefix, DiscoveryIssueReporter issueReporter) { diff --git a/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java b/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java index 3836d2369977..d9bd8e4b4849 100644 --- a/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java +++ b/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java @@ -130,7 +130,7 @@ public enum HierarchyTraversalMode { *

This serves as a cache to avoid repeated cycle detection for classes * that have already been checked. * @since 1.6 - * @see #detectInnerClassCycle(Class) + * @see #detectInnerClassCycle(Class, CycleErrorHandling) */ private static final Set noCyclesDetectedCache = ConcurrentHashMap.newKeySet(); @@ -1117,11 +1117,17 @@ public static Stream streamAllResourcesInModule(String moduleName, Pre * @see org.junit.platform.commons.support.ReflectionSupport#findNestedClasses(Class, Predicate) */ public static List> findNestedClasses(Class clazz, Predicate> predicate) { + return findNestedClasses(clazz, predicate, CycleErrorHandling.THROW_EXCEPTION); + } + + @API(status = INTERNAL, since = "5.13.2") + public static List> findNestedClasses(Class clazz, Predicate> predicate, + CycleErrorHandling errorHandling) { Preconditions.notNull(clazz, "Class must not be null"); Preconditions.notNull(predicate, "Predicate must not be null"); Set> candidates = new LinkedHashSet<>(); - visitAllNestedClasses(clazz, predicate, candidates::add); + visitAllNestedClasses(clazz, predicate, candidates::add, errorHandling); return List.copyOf(candidates); } @@ -1144,13 +1150,15 @@ public static List> findNestedClasses(Class clazz, Predicate clazz, Predicate> predicate) { + @API(status = INTERNAL, since = "1.13.2") + public static boolean isNestedClassPresent(Class clazz, Predicate> predicate, + CycleErrorHandling errorHandling) { Preconditions.notNull(clazz, "Class must not be null"); Preconditions.notNull(predicate, "Predicate must not be null"); + Preconditions.notNull(errorHandling, "CycleErrorHandling must not be null"); AtomicBoolean foundNestedClass = new AtomicBoolean(false); - visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.setPlain(true)); + visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.setPlain(true), errorHandling); return foundNestedClass.getPlain(); } @@ -1162,27 +1170,37 @@ public static Stream> streamNestedClasses(Class clazz, Predicate> streamNestedClasses(Class clazz, Predicate> predicate, + CycleErrorHandling errorHandling) { + return findNestedClasses(clazz, predicate, errorHandling).stream(); + } + /** * Visit all nested classes without support for short-circuiting * in order to ensure all of them are checked for class cycles. */ private static void visitAllNestedClasses(Class clazz, Predicate> predicate, - Consumer> consumer) { + Consumer> consumer, CycleErrorHandling errorHandling) { if (!isSearchable(clazz)) { return; } if (isInnerClass(clazz) && predicate.test(clazz)) { - detectInnerClassCycle(clazz); + if (detectInnerClassCycle(clazz, errorHandling)) { + return; + } } try { // Candidates in current class for (Class nestedClass : clazz.getDeclaredClasses()) { if (predicate.test(nestedClass)) { - detectInnerClassCycle(nestedClass); consumer.accept(nestedClass); + if (detectInnerClassCycle(nestedClass, errorHandling)) { + return; + } } } } @@ -1191,11 +1209,11 @@ private static void visitAllNestedClasses(Class clazz, Predicate> pr } // Search class hierarchy - visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer); + visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer, errorHandling); // Search interface hierarchy for (Class ifc : clazz.getInterfaces()) { - visitAllNestedClasses(ifc, predicate, consumer); + visitAllNestedClasses(ifc, predicate, consumer, errorHandling); } } @@ -1203,10 +1221,8 @@ private static void visitAllNestedClasses(Class clazz, Predicate> pr * Detect a cycle in the inner class hierarchy in which the supplied class * resides — from the supplied class up to the outermost enclosing class * — and throw a {@link JUnitException} if a cycle is detected. - * *

This method does not detect cycles within inner class * hierarchies below the supplied class. - * *

If the supplied class is not an inner class and does not have a * searchable superclass, this method is effectively a no-op. * @@ -1214,25 +1230,26 @@ private static void visitAllNestedClasses(Class clazz, Predicate> pr * @see #isInnerClass(Class) * @see #isSearchable(Class) */ - private static void detectInnerClassCycle(Class clazz) { + private static boolean detectInnerClassCycle(Class clazz, CycleErrorHandling errorHandling) { Preconditions.notNull(clazz, "Class must not be null"); String className = clazz.getName(); if (noCyclesDetectedCache.contains(className)) { - return; + return false; } Class superclass = clazz.getSuperclass(); if (isInnerClass(clazz) && isSearchable(superclass)) { for (Class enclosing = clazz.getEnclosingClass(); enclosing != null; enclosing = enclosing.getEnclosingClass()) { if (superclass.equals(enclosing)) { - throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s", - className, enclosing.getName())); + errorHandling.handle(clazz, enclosing); + return true; } } } noCyclesDetectedCache.add(className); + return false; } /** @@ -1936,4 +1953,25 @@ static Throwable getUnderlyingCause(Throwable t) { return t; } + @API(status = INTERNAL, since = "5.13.2") + public enum CycleErrorHandling { + + THROW_EXCEPTION { + @Override + void handle(Class clazz, Class enclosing) { + throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s", + clazz.getName(), enclosing.getName())); + } + }, + + ABORT_VISIT { + @Override + void handle(Class clazz, Class enclosing) { + // ignore + } + }; + + abstract void handle(Class clazz, Class enclosing); + } + } diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java index 26db9306abb9..395d47c27d0e 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java @@ -48,6 +48,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.platform.engine.DiscoveryIssue; +import org.junit.platform.engine.DiscoveryIssue.Severity; import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.support.descriptor.ClassSource; import org.junit.platform.launcher.LauncherDiscoveryRequest; @@ -289,7 +290,35 @@ void reportsWarningForTestClassWithPotentialNestedTestClasses() { } @Test - void reportsWarningsForInvalidTags() throws NoSuchMethodException { + void ignoresUnrelatedClassDefinitionCycles() { + var results = discoverTestsForClass(UnrelatedRecursiveHierarchyTestCase.class); + + assertThat(results.getDiscoveryIssues()).isEmpty(); + } + + @Test + void ignoresRecursiveNonTestHierarchyCycles() { + var results = discoverTestsForClass(NonTestRecursiveHierarchyTestCase.class); + + assertThat(results.getDiscoveryIssues()).isEmpty(); + } + + @Test + void reportsMissingNestedAnnotationOnRecursiveHierarchy() { + var results = discoverTestsForClass(RecursiveHierarchyWithoutNestedTestCase.class); + + var discoveryIssues = results.getDiscoveryIssues(); + assertThat(discoveryIssues).hasSize(1); + assertThat(discoveryIssues.getFirst().severity()) // + .isEqualTo(Severity.WARNING); + assertThat(discoveryIssues.getFirst().message()) // + .isEqualTo( + "Inner class '%s' looks like it was intended to be a test class but will not be executed. It must be static or annotated with @Nested.", + RecursiveHierarchyWithoutNestedTestCase.Inner.class.getName()); + } + + @Test + void reportsWarningsForInvalidTags() throws Exception { var results = discoverTestsForClass(InvalidTagsTestCase.class); @@ -311,7 +340,7 @@ void reportsWarningsForInvalidTags() throws NoSuchMethodException { } @Test - void reportsWarningsForBlankDisplayNames() throws NoSuchMethodException { + void reportsWarningsForBlankDisplayNames() throws Exception { var results = discoverTestsForClass(BlankDisplayNamesTestCase.class); @@ -441,6 +470,39 @@ private class InvalidTestClassSubclassTestCase extends InvalidTestClassTestCase } + @SuppressWarnings("JUnitMalformedDeclaration") + static class UnrelatedRecursiveHierarchyTestCase { + + @Test + void test() { + } + + @SuppressWarnings({ "InnerClassMayBeStatic", "unused" }) + class Inner { + class Recursive extends Inner { + } + } + } + + @SuppressWarnings("JUnitMalformedDeclaration") + static class RecursiveHierarchyWithoutNestedTestCase { + + @Test + void test() { + } + + @SuppressWarnings({ "InnerClassMayBeStatic", "unused" }) + class Inner extends RecursiveHierarchyWithoutNestedTestCase { + } + } + + @SuppressWarnings("unused") + static class NonTestRecursiveHierarchyTestCase { + @SuppressWarnings("InnerClassMayBeStatic") + class Inner extends NonTestRecursiveHierarchyTestCase { + } + } + @SuppressWarnings("JUnitMalformedDeclaration") @Tag("") static class InvalidTagsTestCase { diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java index 30ea70b31b54..3c280e29f4ef 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java @@ -11,7 +11,6 @@ package org.junit.jupiter.engine.discovery.predicates; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -24,7 +23,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; import org.junit.jupiter.api.TestTemplate; -import org.junit.platform.commons.JUnitException; import org.junit.platform.engine.DiscoveryIssue; import org.junit.platform.engine.DiscoveryIssue.Severity; import org.junit.platform.engine.support.descriptor.ClassSource; @@ -218,11 +216,7 @@ void privateStaticTestClassEvaluatesToFalse() { */ @Test void recursiveHierarchies() { - assertThatExceptionOfType(JUnitException.class)// - .isThrownBy(() -> predicates.looksLikeIntendedTestClass(TestCases.OuterClass.class))// - .withMessage("Detected cycle in inner class hierarchy between %s and %s", - TestCases.OuterClass.RecursiveInnerClass.class.getName(), TestCases.OuterClass.class.getName()); - + assertTrue(predicates.looksLikeIntendedTestClass(TestCases.OuterClass.class)); assertTrue(predicates.isValidStandaloneTestClass(TestCases.OuterClass.class)); assertThat(discoveryIssues).isEmpty(); diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java index 87872afbb645..7b970410e226 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java +++ b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java @@ -71,6 +71,7 @@ import org.junit.platform.commons.logging.LogRecordListener; import org.junit.platform.commons.support.Resource; import org.junit.platform.commons.test.TestClassLoader; +import org.junit.platform.commons.util.ReflectionUtils.CycleErrorHandling; import org.junit.platform.commons.util.ReflectionUtilsTests.NestedClassTests.ClassWithNestedClasses.Nested1; import org.junit.platform.commons.util.ReflectionUtilsTests.NestedClassTests.ClassWithNestedClasses.Nested2; import org.junit.platform.commons.util.ReflectionUtilsTests.NestedClassTests.ClassWithNestedClasses.Nested3; @@ -1054,9 +1055,10 @@ void findNestedClassesPreconditions() { @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)); + assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(null, null, null)); + assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(null, clazz -> true, CycleErrorHandling.THROW_EXCEPTION)); + assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(getClass(), null, CycleErrorHandling.ABORT_VISIT)); + assertThrows(PreconditionViolationException.class, () -> ReflectionUtils.isNestedClassPresent(getClass(), clazz -> true, null)); // @formatter:on } @@ -1073,12 +1075,12 @@ void findNestedClasses() { assertThat(ReflectionUtils.findNestedClasses(ClassWithNestedClasses.class, clazz -> clazz.getName().contains("1"))) .containsExactly(Nested1.class); - assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, clazz -> clazz.getName().contains("1"))) + assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, clazz -> clazz.getName().contains("1"), CycleErrorHandling.THROW_EXCEPTION)) .isTrue(); assertThat(ReflectionUtils.findNestedClasses(ClassWithNestedClasses.class, ReflectionUtils::isStatic)) .containsExactly(Nested3.class); - assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, ReflectionUtils::isStatic)) + assertThat(ReflectionUtils.isNestedClassPresent(ClassWithNestedClasses.class, ReflectionUtils::isStatic, CycleErrorHandling.THROW_EXCEPTION)) .isTrue(); assertThat(findNestedClasses(ClassExtendingClassWithNestedClasses.class)) @@ -1105,12 +1107,14 @@ void findNestedClassesWithSeeminglyRecursiveHierarchies() { // 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(); + assertThat(ReflectionUtils.isNestedClassPresent(OuterClass.class, clazz -> false, + CycleErrorHandling.THROW_EXCEPTION)).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(); + assertThat(ReflectionUtils.isNestedClassPresent(RecursiveInnerInnerClass.class, clazz -> false, + CycleErrorHandling.THROW_EXCEPTION)).isFalse(); // Sibling types don't actually result in cycles. assertThat(findNestedClasses(StaticNestedSiblingClass.class))// @@ -1153,7 +1157,7 @@ private static List> findNestedClasses(Class clazz) { } private static boolean isNestedClassPresent(Class clazz) { - return ReflectionUtils.isNestedClassPresent(clazz, c -> true); + return ReflectionUtils.isNestedClassPresent(clazz, c -> true, CycleErrorHandling.THROW_EXCEPTION); } private void assertNestedCycle(Class from, Class to) {