`TestClassPredicates.looksLikeIntendedTestClass()` does not consistently detect nested class hierarchy cycles

[sbrannen]

Overview

TestClassPredicatesTests.StandaloneTestClasses.recursiveHierarchies() currently passes when run with the Gradle build or in IntelliJ IDEA; however, it fails in Eclipse IDE.

Specifically, the assertion on the first line of that test method does not throw an exception in Eclipse.

https://github.com/junit-team/junit5/blob/3c5b457564d559fb0f396d7f303f8effe291daa3/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java#L221-L224

The reason is twofold:

1. ReflectionUtils.visitNestedClasses(...) short-circuits the search algorithm once a @Nested test class has been detected.
2. The order in which nested classes are returned from java.lang.Class.getDeclaredClasses() apparently differs between the OpenJDK/IntelliJ compilers and the Eclipse compiler.

The result is that ReflectionUtils.detectInnerClassCycle() only gets invoked on nested classes until the first @Nested class is encountered (or until there are no more nested classes to inspect).

Thus, if a @Nested class is detected before a nested class with a class hierarchy cycle, the cycle is not detected/reported.

In Eclipse, if you rename TestClassPredicatesTests.TestCases.OuterClass.InnerClass to TestClassPredicatesTests.TestCases.OuterClass.XXXInnerClass, the test then passes because TestClassPredicatesTests.TestCases.OuterClass.RecursiveInnerClass is encountered first, resulting in the expected JUnitException stating that it "detected [a] cycle in [the] inner class hierarchy".

Related Issues

• Introduce error handling mechanism for validation and discovery errors #242

[sbrannen]

@marcphilipp, I assigned this to you since you did the work on #242 related to this.

I'm not sure if the "issue" is merely with the test or rather a true bug in the reporting of discovery issues, so I'll let you change the title and/or labels for this issue as you see fit.

[sbrannen]

2. The order in which nested classes are returned from java.lang.Class.getDeclaredClasses() apparently differs between the OpenJDK/IntelliJ compilers and the Eclipse compiler.

Regarding that, we typically should not rely on the order anyway.

[marcphilipp]

Short-circuiting was introduced in 9380195 as an optimization for checking whether a class contains @Nested test classes. However, whether or not an exception is thrown depending on the order of classes returned by Java's reflection API sounds like a bad idea. I think we'll have to remove the short-circuiting in order to provide a consistent error message in such cases.