Skip to content

Commit d7ce21b

Browse files
marcphilippmarcphilipp
committed
Avoid discovery errors for inner classes not annotated with @Nested
Resolves #4661. (cherry picked from commit 8738f1b)
1 parent 0ce3a48 commit d7ce21b

7 files changed

Lines changed: 130 additions & 44 deletions

File tree

‎documentation/src/docs/asciidoc/release-notes/release-notes-5.13.2.adoc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ repository on GitHub.
3535
[[release-notes-5.13.2-junit-jupiter-bug-fixes]]
3636
==== Bug Fixes
3737

38-
* ❓
38+
* Stop reporting discovery issues for cyclic inner class hierarchies not annotated with
39+
`@Nested`.
3940

4041
[[release-notes-5.13.2-junit-jupiter-deprecations-and-breaking-changes]]
4142
==== Deprecations and Breaking Changes

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassSelectorResolver.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@
1717
import static org.junit.jupiter.engine.descriptor.NestedClassTestDescriptor.getEnclosingTestClasses;
1818
import static org.junit.platform.commons.support.HierarchyTraversalMode.TOP_DOWN;
1919
import static org.junit.platform.commons.support.ReflectionSupport.findMethods;
20-
import static org.junit.platform.commons.support.ReflectionSupport.streamNestedClasses;
2120
import static org.junit.platform.commons.util.FunctionUtils.where;
2221
import static org.junit.platform.commons.util.ReflectionUtils.isInnerClass;
22+
import static org.junit.platform.commons.util.ReflectionUtils.streamNestedClasses;
2323
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUniqueId;
2424
import static org.junit.platform.engine.support.discovery.SelectorResolver.Resolution.unresolved;
2525

@@ -46,6 +46,7 @@
4646
import org.junit.jupiter.engine.discovery.predicates.TestClassPredicates;
4747
import org.junit.platform.commons.support.ReflectionSupport;
4848
import org.junit.platform.commons.util.ReflectionUtils;
49+
import org.junit.platform.commons.util.ReflectionUtils.CycleErrorHandling;
4950
import org.junit.platform.engine.DiscoveryIssue;
5051
import org.junit.platform.engine.DiscoveryIssue.Severity;
5152
import org.junit.platform.engine.DiscoverySelector;
@@ -294,9 +295,13 @@ private Supplier<Set<? extends DiscoverySelector>> expansionCallback(TestDescrip
294295
Stream<DiscoverySelector> methods = findMethods(testClass,
295296
this.predicates.isTestOrTestFactoryOrTestTemplateMethod, TOP_DOWN).stream() //
296297
.map(method -> selectMethod(testClasses, method));
297-
Stream<NestedClassSelector> nestedClasses = streamNestedClasses(testClass,
298-
this.predicates.isAnnotatedWithNested.or(ReflectionUtils::isInnerClass)) //
299-
.map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass));
298+
Stream<Class<?>> annotatedNestedClasses = streamNestedClasses(testClass,
299+
this.predicates.isAnnotatedWithNested);
300+
Stream<Class<?>> notAnnotatedInnerClasses = streamNestedClasses(testClass,
301+
this.predicates.isAnnotatedWithNested.negate().and(ReflectionUtils::isInnerClass),
302+
CycleErrorHandling.ABORT_VISIT);
303+
Stream<NestedClassSelector> nestedClasses = Stream.concat(annotatedNestedClasses, notAnnotatedInnerClasses) //
304+
.map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass));
300305
return Stream.concat(methods, nestedClasses).collect(
301306
toCollection((Supplier<Set<DiscoverySelector>>) LinkedHashSet::new));
302307
};

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.junit.jupiter.api.ClassTemplate;
2929
import org.junit.jupiter.api.Nested;
3030
import org.junit.platform.commons.util.ReflectionUtils;
31+
import org.junit.platform.commons.util.ReflectionUtils.CycleErrorHandling;
3132
import org.junit.platform.engine.DiscoveryIssue;
3233
import org.junit.platform.engine.support.descriptor.ClassSource;
3334
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
@@ -94,14 +95,16 @@ private boolean hasTestOrTestFactoryOrTestTemplateMethods(Class<?> candidate) {
9495
}
9596

9697
private boolean hasNestedTests(Class<?> candidate, Set<Class<?>> seen) {
98+
boolean hasAnnotatedClass = isNestedClassPresent(candidate, this.isAnnotatedWithNested,
99+
CycleErrorHandling.THROW_EXCEPTION);
100+
if (hasAnnotatedClass) {
101+
return true;
102+
}
97103
return isNestedClassPresent( //
98104
candidate, //
99-
isNotSame(candidate).and(
100-
this.isAnnotatedWithNested.or(it -> isInnerClass(it) && looksLikeIntendedTestClass(it, seen))));
101-
}
102-
103-
private static Predicate<Class<?>> isNotSame(Class<?> candidate) {
104-
return clazz -> candidate != clazz;
105+
it -> isInnerClass(it) && looksLikeIntendedTestClass(it, seen), //
106+
CycleErrorHandling.ABORT_VISIT //
107+
);
105108
}
106109

107110
private static Condition<Class<?>> isNotPrivateUnlessAbstract(String prefix, DiscoveryIssueReporter issueReporter) {

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

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public enum HierarchyTraversalMode {
153153
* <p>This serves as a cache to avoid repeated cycle detection for classes
154154
* that have already been checked.
155155
* @since 1.6
156-
* @see #detectInnerClassCycle(Class)
156+
* @see #detectInnerClassCycle(Class, CycleErrorHandling)
157157
*/
158158
private static final Set<String> noCyclesDetectedCache = ConcurrentHashMap.newKeySet();
159159

@@ -1234,11 +1234,17 @@ public static Stream<Resource> streamAllResourcesInModule(String moduleName, Pre
12341234
* @see org.junit.platform.commons.support.ReflectionSupport#findNestedClasses(Class, Predicate)
12351235
*/
12361236
public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate) {
1237+
return findNestedClasses(clazz, predicate, CycleErrorHandling.THROW_EXCEPTION);
1238+
}
1239+
1240+
@API(status = INTERNAL, since = "5.13.2")
1241+
public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1242+
CycleErrorHandling errorHandling) {
12371243
Preconditions.notNull(clazz, "Class must not be null");
12381244
Preconditions.notNull(predicate, "Predicate must not be null");
12391245

12401246
Set<Class<?>> candidates = new LinkedHashSet<>();
1241-
visitAllNestedClasses(clazz, predicate, candidates::add);
1247+
visitAllNestedClasses(clazz, predicate, candidates::add, errorHandling);
12421248
return Collections.unmodifiableList(new ArrayList<>(candidates));
12431249
}
12441250

@@ -1255,13 +1261,15 @@ public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?
12551261
* @return {@code true} if such a nested class is present
12561262
* @throws JUnitException if a cycle is detected within an inner class hierarchy
12571263
*/
1258-
@API(status = INTERNAL, since = "1.13")
1259-
public static boolean isNestedClassPresent(Class<?> clazz, Predicate<Class<?>> predicate) {
1264+
@API(status = INTERNAL, since = "1.13.2")
1265+
public static boolean isNestedClassPresent(Class<?> clazz, Predicate<Class<?>> predicate,
1266+
CycleErrorHandling errorHandling) {
12601267
Preconditions.notNull(clazz, "Class must not be null");
12611268
Preconditions.notNull(predicate, "Predicate must not be null");
1269+
Preconditions.notNull(errorHandling, "CycleErrorHandling must not be null");
12621270

12631271
AtomicBoolean foundNestedClass = new AtomicBoolean(false);
1264-
visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.set(true));
1272+
visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.set(true), errorHandling);
12651273
return foundNestedClass.get();
12661274
}
12671275

@@ -1273,27 +1281,37 @@ public static Stream<Class<?>> streamNestedClasses(Class<?> clazz, Predicate<Cla
12731281
return findNestedClasses(clazz, predicate).stream();
12741282
}
12751283

1284+
@API(status = INTERNAL, since = "5.13.2")
1285+
public static Stream<Class<?>> streamNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1286+
CycleErrorHandling errorHandling) {
1287+
return findNestedClasses(clazz, predicate, errorHandling).stream();
1288+
}
1289+
12761290
/**
12771291
* Visit <em>all</em> nested classes without support for short-circuiting
12781292
* in order to ensure all of them are checked for class cycles.
12791293
*/
12801294
private static void visitAllNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1281-
Consumer<Class<?>> consumer) {
1295+
Consumer<Class<?>> consumer, CycleErrorHandling errorHandling) {
12821296

12831297
if (!isSearchable(clazz)) {
12841298
return;
12851299
}
12861300

12871301
if (isInnerClass(clazz) && predicate.test(clazz)) {
1288-
detectInnerClassCycle(clazz);
1302+
if (detectInnerClassCycle(clazz, errorHandling)) {
1303+
return;
1304+
}
12891305
}
12901306

12911307
try {
12921308
// Candidates in current class
12931309
for (Class<?> nestedClass : clazz.getDeclaredClasses()) {
12941310
if (predicate.test(nestedClass)) {
1295-
detectInnerClassCycle(nestedClass);
12961311
consumer.accept(nestedClass);
1312+
if (detectInnerClassCycle(nestedClass, errorHandling)) {
1313+
return;
1314+
}
12971315
}
12981316
}
12991317
}
@@ -1302,48 +1320,47 @@ private static void visitAllNestedClasses(Class<?> clazz, Predicate<Class<?>> pr
13021320
}
13031321

13041322
// Search class hierarchy
1305-
visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer);
1323+
visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer, errorHandling);
13061324

13071325
// Search interface hierarchy
13081326
for (Class<?> ifc : clazz.getInterfaces()) {
1309-
visitAllNestedClasses(ifc, predicate, consumer);
1327+
visitAllNestedClasses(ifc, predicate, consumer, errorHandling);
13101328
}
13111329
}
13121330

13131331
/**
13141332
* Detect a cycle in the inner class hierarchy in which the supplied class
13151333
* resides &mdash; from the supplied class up to the outermost enclosing class
13161334
* &mdash; and throw a {@link JUnitException} if a cycle is detected.
1317-
*
13181335
* <p>This method does <strong>not</strong> detect cycles within inner class
13191336
* hierarchies <em>below</em> the supplied class.
1320-
*
13211337
* <p>If the supplied class is not an inner class and does not have a
13221338
* searchable superclass, this method is effectively a no-op.
13231339
*
13241340
* @since 1.6
13251341
* @see #isInnerClass(Class)
13261342
* @see #isSearchable(Class)
13271343
*/
1328-
private static void detectInnerClassCycle(Class<?> clazz) {
1344+
private static boolean detectInnerClassCycle(Class<?> clazz, CycleErrorHandling errorHandling) {
13291345
Preconditions.notNull(clazz, "Class must not be null");
13301346
String className = clazz.getName();
13311347

13321348
if (noCyclesDetectedCache.contains(className)) {
1333-
return;
1349+
return false;
13341350
}
13351351

13361352
Class<?> superclass = clazz.getSuperclass();
13371353
if (isInnerClass(clazz) && isSearchable(superclass)) {
13381354
for (Class<?> enclosing = clazz.getEnclosingClass(); enclosing != null; enclosing = enclosing.getEnclosingClass()) {
13391355
if (superclass.equals(enclosing)) {
1340-
throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s",
1341-
className, enclosing.getName()));
1356+
errorHandling.handle(clazz, enclosing);
1357+
return true;
13421358
}
13431359
}
13441360
}
13451361

13461362
noCyclesDetectedCache.add(className);
1363+
return false;
13471364
}
13481365

13491366
/**
@@ -2113,4 +2130,25 @@ private static boolean getLegacySearchSemanticsFlag() {
21132130
return isTrue;
21142131
}
21152132

2133+
@API(status = INTERNAL, since = "5.13.2")
2134+
public enum CycleErrorHandling {
2135+
2136+
THROW_EXCEPTION {
2137+
@Override
2138+
void handle(Class<?> clazz, Class<?> enclosing) {
2139+
throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s",
2140+
clazz.getName(), enclosing.getName()));
2141+
}
2142+
},
2143+
2144+
ABORT_VISIT {
2145+
@Override
2146+
void handle(Class<?> clazz, Class<?> enclosing) {
2147+
// ignore
2148+
}
2149+
};
2150+
2151+
abstract void handle(Class<?> clazz, Class<?> enclosing);
2152+
}
2153+
21162154
}

‎jupiter-tests/src/test/java/org/junit/jupiter/engine/discovery/DiscoveryTests.java‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import org.junit.jupiter.params.provider.Arguments;
4949
import org.junit.jupiter.params.provider.MethodSource;
5050
import org.junit.platform.engine.DiscoveryIssue;
51+
import org.junit.platform.engine.DiscoveryIssue.Severity;
5152
import org.junit.platform.engine.TestDescriptor;
5253
import org.junit.platform.engine.support.descriptor.ClassSource;
5354
import org.junit.platform.launcher.LauncherDiscoveryRequest;
@@ -296,7 +297,28 @@ void ignoresUnrelatedClassDefinitionCycles() {
296297
}
297298

298299
@Test
299-
void reportsWarningsForInvalidTags() throws NoSuchMethodException {
300+
void ignoresRecursiveNonTestHierarchyCycles() {
301+
var results = discoverTestsForClass(NonTestRecursiveHierarchyTestCase.class);
302+
303+
assertThat(results.getDiscoveryIssues()).isEmpty();
304+
}
305+
306+
@Test
307+
void reportsMissingNestedAnnotationOnRecursiveHierarchy() {
308+
var results = discoverTestsForClass(RecursiveHierarchyWithoutNestedTestCase.class);
309+
310+
var discoveryIssues = results.getDiscoveryIssues();
311+
assertThat(discoveryIssues).hasSize(1);
312+
assertThat(discoveryIssues.getFirst().severity()) //
313+
.isEqualTo(Severity.WARNING);
314+
assertThat(discoveryIssues.getFirst().message()) //
315+
.isEqualTo(
316+
"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.",
317+
RecursiveHierarchyWithoutNestedTestCase.Inner.class.getName());
318+
}
319+
320+
@Test
321+
void reportsWarningsForInvalidTags() throws Exception {
300322

301323
var results = discoverTestsForClass(InvalidTagsTestCase.class);
302324

@@ -318,7 +340,7 @@ void reportsWarningsForInvalidTags() throws NoSuchMethodException {
318340
}
319341

320342
@Test
321-
void reportsWarningsForBlankDisplayNames() throws NoSuchMethodException {
343+
void reportsWarningsForBlankDisplayNames() throws Exception {
322344

323345
var results = discoverTestsForClass(BlankDisplayNamesTestCase.class);
324346

@@ -462,6 +484,25 @@ class Recursive extends Inner {
462484
}
463485
}
464486

487+
@SuppressWarnings("JUnitMalformedDeclaration")
488+
static class RecursiveHierarchyWithoutNestedTestCase {
489+
490+
@Test
491+
void test() {
492+
}
493+
494+
@SuppressWarnings({ "InnerClassMayBeStatic", "unused" })
495+
class Inner extends RecursiveHierarchyWithoutNestedTestCase {
496+
}
497+
}
498+
499+
@SuppressWarnings("unused")
500+
static class NonTestRecursiveHierarchyTestCase {
501+
@SuppressWarnings("InnerClassMayBeStatic")
502+
class Inner extends NonTestRecursiveHierarchyTestCase {
503+
}
504+
}
505+
465506
@SuppressWarnings("JUnitMalformedDeclaration")
466507
@Tag("")
467508
static class InvalidTagsTestCase {

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
package org.junit.jupiter.engine.discovery.predicates;
1212

1313
import static org.assertj.core.api.Assertions.assertThat;
14-
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
1514
import static org.junit.jupiter.api.Assertions.assertFalse;
1615
import static org.junit.jupiter.api.Assertions.assertTrue;
1716

@@ -24,7 +23,6 @@
2423
import org.junit.jupiter.api.Test;
2524
import org.junit.jupiter.api.TestFactory;
2625
import org.junit.jupiter.api.TestTemplate;
27-
import org.junit.platform.commons.JUnitException;
2826
import org.junit.platform.engine.DiscoveryIssue;
2927
import org.junit.platform.engine.DiscoveryIssue.Severity;
3028
import org.junit.platform.engine.support.descriptor.ClassSource;
@@ -218,11 +216,7 @@ void privateStaticTestClassEvaluatesToFalse() {
218216
*/
219217
@Test
220218
void recursiveHierarchies() {
221-
assertThatExceptionOfType(JUnitException.class)//
222-
.isThrownBy(() -> predicates.looksLikeIntendedTestClass(TestCases.OuterClass.class))//
223-
.withMessage("Detected cycle in inner class hierarchy between %s and %s",
224-
TestCases.OuterClass.RecursiveInnerClass.class.getName(), TestCases.OuterClass.class.getName());
225-
219+
assertTrue(predicates.looksLikeIntendedTestClass(TestCases.OuterClass.class));
226220
assertTrue(predicates.isValidStandaloneTestClass(TestCases.OuterClass.class));
227221
assertThat(discoveryIssues).isEmpty();
228222

0 commit comments

Comments
 (0)