Detect cycles in inner class hierarchies

[sbrannen]

Overview

Since Java allows nested classes to extend other classes, it is possible to introduce a cycle when using @Nested test classes.

For example, the following compiles in Java. So it's syntactically correct, even though it's semantically invalid. If you attempt to execute OuterTests or NestedTests, the discovery phase in JUnit Jupiter never completes (or at least it does not result in a stack overflow in a reasonable amount of time on my machine).

On the other hand, if you execute only test1() or test2(), each of those will execute as expected.

package example;

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;

class OuterTests {

	@Test
	void test1() {
	}

	@Nested
	class NestedTests extends OuterTests {
		@Test
		void test2() {
		}
	}
}

Deliverables

• Detect cycles in inner class hierarchies and abort the discovery of those nested test classes by throwing an exception.

[sbrannen]

Tentatively slated for 5.6 M1 for team discussion to determine what the outcome of cycle detection should be.

[marcphilipp]

Team Decision: Throw an exception during discovery if a cycle is detected.

[sbrannen]

It appears that this may be an issue in the Platform rather than (only?) in Jupiter, specifically in org.junit.platform.commons.util.ReflectionUtils.findNestedClasses(Class<?>, Set<Class<?>>)

[sbrannen]

With the following change to the aforementioned findNestedClasses(...) method...

// Detect cycle in hierarchy
boolean cycleDetected = false;
Class<?> superclass = clazz.getSuperclass();
if (superclass != null) {
	Class<?> enclosingClass = clazz.getEnclosingClass();
	while (enclosingClass != null) {
		if (superclass.equals(enclosingClass)) {
			cycleDetected = true;
			break;
		}
		enclosingClass = enclosingClass.getEnclosingClass();
	}
}

// Search class hierarchy
if (!cycleDetected) {
	findNestedClasses(superclass, candidates);
}

... the algorithm no longer recurses indefinitely.

This change does not throw an exception if a cycle is detected. Rather, it simply does not follow the cycle.

Thus, with this implementation, the following tests are executed.

@junit-team/junit-lambda, what do you think about supporting that instead of throwing an exception when a cycle is detected?

Keep in mind that whatever we do here will affect any extension or test engine that makes use of org.junit.platform.commons.support.ReflectionSupport.findNestedClasses(Class<?>, Predicate<Class<?>>).

[sbrannen]

FYI: current work on this issue can be seen here: /~https://github.com/junit-team/junit5/compare/issues/2039-recursive-nested-hierarchy

[marcphilipp]

Team Decision: Let's throw an exception instead of silently ignoring the cycle.

[sbrannen]

OK. The status quo now results in...

org.junit.platform.commons.JUnitException: TestEngine with ID 'junit-jupiter' failed to discover tests

...

Caused by: org.junit.platform.commons.JUnitException: ClassSelector [className = 'example.OuterTests$NestedTests'] resolution failed

...

Caused by: org.junit.platform.commons.JUnitException: Detected cycle in nested class hierarchy between example.OuterTests$NestedTests and example.OuterTests

Good like that?

[chavan77me]

Upgrading to JUnit5 is breaking a unit test I have.

private class MySillyClosure(...) extends Closure<RetryStrategy>(...) { @override RetryStrategy call(...) { return MyStrategy() } }

This class is defined in my test suite and the recursive check fails even though I have not marked any of these classes as @nested.

I think this is the suspect code
/~https://github.com/junit-team/junit5/blob/master/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/predicates/IsTestClassWithTests.java#L46

/~https://github.com/junit-team/junit5/blob/master/junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java#L1007-L1008

Notice that the code looks for all nested classes (and asserts on finding a cycle) before attempting to filter out using the predicate for @nested. Therefore any class that has this inner inheritance (in my case Groovy Closure and WriteableClosure) is causing an error even if they are not marked as @nested

For reference, here is the closure code and its inner class
/~https://github.com/groovy/groovy-core/blob/master/src/main/groovy/lang/Closure.java#L847

Is there a way for me to disable this recursion check?

[sbrannen]

@chavan77me, this issue is already closed.

Please open a new issue, and we'll gladly look into it.