`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.

junit5/jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/predicates/TestClassPredicatesTests.java

Lines 221 to 224 in 3c5b457

	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());

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.