Skip to content

Commit 058f2ee

Browse files
marcphilippmarcphilipp
committed
Avoid discovery errors for inner classes not annotated with @Nested
Resolves #4661.
1 parent 28a8384 commit 058f2ee

File tree

7 files changed

+130
-44
lines changed

7 files changed

+130
-44
lines changed

‎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 @@ 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;
@@ -289,9 +290,13 @@ private Supplier<Set<? extends DiscoverySelector>> expansionCallback(TestDescrip
289290
Stream<DiscoverySelector> methods = findMethods(testClass,
290291
this.predicates.isTestOrTestFactoryOrTestTemplateMethod, TOP_DOWN).stream() //
291292
.map(method -> selectMethod(testClasses, method));
292-
Stream<NestedClassSelector> nestedClasses = streamNestedClasses(testClass,
293-
this.predicates.isAnnotatedWithNested.or(ReflectionUtils::isInnerClass)) //
294-
.map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass));
293+
Stream<Class<?>> annotatedNestedClasses = streamNestedClasses(testClass,
294+
this.predicates.isAnnotatedWithNested);
295+
Stream<Class<?>> notAnnotatedInnerClasses = streamNestedClasses(testClass,
296+
this.predicates.isAnnotatedWithNested.negate().and(ReflectionUtils::isInnerClass),
297+
CycleErrorHandling.ABORT_VISIT);
298+
var nestedClasses = Stream.concat(annotatedNestedClasses, notAnnotatedInnerClasses) //
299+
.map(nestedClass -> DiscoverySelectors.selectNestedClass(testClasses, nestedClass));
295300
return Stream.concat(methods, nestedClasses).collect(
296301
toCollection((Supplier<Set<DiscoverySelector>>) LinkedHashSet::new));
297302
};

‎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+
var 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
@@ -130,7 +130,7 @@ public enum HierarchyTraversalMode {
130130
* <p>This serves as a cache to avoid repeated cycle detection for classes
131131
* that have already been checked.
132132
* @since 1.6
133-
* @see #detectInnerClassCycle(Class)
133+
* @see #detectInnerClassCycle(Class, CycleErrorHandling)
134134
*/
135135
private static final Set<String> noCyclesDetectedCache = ConcurrentHashMap.newKeySet();
136136

@@ -1117,11 +1117,17 @@ public static Stream<Resource> streamAllResourcesInModule(String moduleName, Pre
11171117
* @see org.junit.platform.commons.support.ReflectionSupport#findNestedClasses(Class, Predicate)
11181118
*/
11191119
public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate) {
1120+
return findNestedClasses(clazz, predicate, CycleErrorHandling.THROW_EXCEPTION);
1121+
}
1122+
1123+
@API(status = INTERNAL, since = "5.13.2")
1124+
public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1125+
CycleErrorHandling errorHandling) {
11201126
Preconditions.notNull(clazz, "Class must not be null");
11211127
Preconditions.notNull(predicate, "Predicate must not be null");
11221128

11231129
Set<Class<?>> candidates = new LinkedHashSet<>();
1124-
visitAllNestedClasses(clazz, predicate, candidates::add);
1130+
visitAllNestedClasses(clazz, predicate, candidates::add, errorHandling);
11251131
return List.copyOf(candidates);
11261132
}
11271133

@@ -1144,13 +1150,15 @@ public static List<Class<?>> findNestedClasses(Class<?> clazz, Predicate<Class<?
11441150
* @return {@code true} if such a nested class is present
11451151
* @throws JUnitException if a cycle is detected within an inner class hierarchy
11461152
*/
1147-
@API(status = INTERNAL, since = "1.13")
1148-
public static boolean isNestedClassPresent(Class<?> clazz, Predicate<Class<?>> predicate) {
1153+
@API(status = INTERNAL, since = "1.13.2")
1154+
public static boolean isNestedClassPresent(Class<?> clazz, Predicate<Class<?>> predicate,
1155+
CycleErrorHandling errorHandling) {
11491156
Preconditions.notNull(clazz, "Class must not be null");
11501157
Preconditions.notNull(predicate, "Predicate must not be null");
1158+
Preconditions.notNull(errorHandling, "CycleErrorHandling must not be null");
11511159

11521160
AtomicBoolean foundNestedClass = new AtomicBoolean(false);
1153-
visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.setPlain(true));
1161+
visitAllNestedClasses(clazz, predicate, __ -> foundNestedClass.setPlain(true), errorHandling);
11541162
return foundNestedClass.getPlain();
11551163
}
11561164

@@ -1162,27 +1170,37 @@ public static Stream<Class<?>> streamNestedClasses(Class<?> clazz, Predicate<Cla
11621170
return findNestedClasses(clazz, predicate).stream();
11631171
}
11641172

1173+
@API(status = INTERNAL, since = "5.13.2")
1174+
public static Stream<Class<?>> streamNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1175+
CycleErrorHandling errorHandling) {
1176+
return findNestedClasses(clazz, predicate, errorHandling).stream();
1177+
}
1178+
11651179
/**
11661180
* Visit <em>all</em> nested classes without support for short-circuiting
11671181
* in order to ensure all of them are checked for class cycles.
11681182
*/
11691183
private static void visitAllNestedClasses(Class<?> clazz, Predicate<Class<?>> predicate,
1170-
Consumer<Class<?>> consumer) {
1184+
Consumer<Class<?>> consumer, CycleErrorHandling errorHandling) {
11711185

11721186
if (!isSearchable(clazz)) {
11731187
return;
11741188
}
11751189

11761190
if (isInnerClass(clazz) && predicate.test(clazz)) {
1177-
detectInnerClassCycle(clazz);
1191+
if (detectInnerClassCycle(clazz, errorHandling)) {
1192+
return;
1193+
}
11781194
}
11791195

11801196
try {
11811197
// Candidates in current class
11821198
for (Class<?> nestedClass : clazz.getDeclaredClasses()) {
11831199
if (predicate.test(nestedClass)) {
1184-
detectInnerClassCycle(nestedClass);
11851200
consumer.accept(nestedClass);
1201+
if (detectInnerClassCycle(nestedClass, errorHandling)) {
1202+
return;
1203+
}
11861204
}
11871205
}
11881206
}
@@ -1191,48 +1209,47 @@ private static void visitAllNestedClasses(Class<?> clazz, Predicate<Class<?>> pr
11911209
}
11921210

11931211
// Search class hierarchy
1194-
visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer);
1212+
visitAllNestedClasses(clazz.getSuperclass(), predicate, consumer, errorHandling);
11951213

11961214
// Search interface hierarchy
11971215
for (Class<?> ifc : clazz.getInterfaces()) {
1198-
visitAllNestedClasses(ifc, predicate, consumer);
1216+
visitAllNestedClasses(ifc, predicate, consumer, errorHandling);
11991217
}
12001218
}
12011219

12021220
/**
12031221
* Detect a cycle in the inner class hierarchy in which the supplied class
12041222
* resides &mdash; from the supplied class up to the outermost enclosing class
12051223
* &mdash; and throw a {@link JUnitException} if a cycle is detected.
1206-
*
12071224
* <p>This method does <strong>not</strong> detect cycles within inner class
12081225
* hierarchies <em>below</em> the supplied class.
1209-
*
12101226
* <p>If the supplied class is not an inner class and does not have a
12111227
* searchable superclass, this method is effectively a no-op.
12121228
*
12131229
* @since 1.6
12141230
* @see #isInnerClass(Class)
12151231
* @see #isSearchable(Class)
12161232
*/
1217-
private static void detectInnerClassCycle(Class<?> clazz) {
1233+
private static boolean detectInnerClassCycle(Class<?> clazz, CycleErrorHandling errorHandling) {
12181234
Preconditions.notNull(clazz, "Class must not be null");
12191235
String className = clazz.getName();
12201236

12211237
if (noCyclesDetectedCache.contains(className)) {
1222-
return;
1238+
return false;
12231239
}
12241240

12251241
Class<?> superclass = clazz.getSuperclass();
12261242
if (isInnerClass(clazz) && isSearchable(superclass)) {
12271243
for (Class<?> enclosing = clazz.getEnclosingClass(); enclosing != null; enclosing = enclosing.getEnclosingClass()) {
12281244
if (superclass.equals(enclosing)) {
1229-
throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s",
1230-
className, enclosing.getName()));
1245+
errorHandling.handle(clazz, enclosing);
1246+
return true;
12311247
}
12321248
}
12331249
}
12341250

12351251
noCyclesDetectedCache.add(className);
1252+
return false;
12361253
}
12371254

12381255
/**
@@ -1936,4 +1953,25 @@ static Throwable getUnderlyingCause(Throwable t) {
19361953
return t;
19371954
}
19381955

1956+
@API(status = INTERNAL, since = "5.13.2")
1957+
public enum CycleErrorHandling {
1958+
1959+
THROW_EXCEPTION {
1960+
@Override
1961+
void handle(Class<?> clazz, Class<?> enclosing) {
1962+
throw new JUnitException(String.format("Detected cycle in inner class hierarchy between %s and %s",
1963+
clazz.getName(), enclosing.getName()));
1964+
}
1965+
},
1966+
1967+
ABORT_VISIT {
1968+
@Override
1969+
void handle(Class<?> clazz, Class<?> enclosing) {
1970+
// ignore
1971+
}
1972+
};
1973+
1974+
abstract void handle(Class<?> clazz, Class<?> enclosing);
1975+
}
1976+
19391977
}

‎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)