Skip to content

Commit c848479

Browse files
marcphilippmarcphilipp
committed
Report invalid lifecycle methods as discovery issues
Issue: #242
1 parent 797e202 commit c848479

File tree

8 files changed

+158
-90
lines changed

8 files changed

+158
-90
lines changed

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java

+49-23
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,12 @@
7171
import org.junit.platform.commons.util.ReflectionUtils;
7272
import org.junit.platform.commons.util.StringUtils;
7373
import org.junit.platform.commons.util.UnrecoverableExceptions;
74+
import org.junit.platform.engine.DiscoveryIssue;
7475
import org.junit.platform.engine.TestDescriptor;
7576
import org.junit.platform.engine.TestTag;
7677
import org.junit.platform.engine.UniqueId;
7778
import org.junit.platform.engine.support.descriptor.ClassSource;
79+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
7880
import org.junit.platform.engine.support.hierarchical.ThrowableCollector;
7981

8082
/**
@@ -84,28 +86,29 @@
8486
*/
8587
@API(status = INTERNAL, since = "5.5")
8688
public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor
87-
implements ResourceLockAware, TestClassAware {
89+
implements ResourceLockAware, TestClassAware, Validatable {
8890

8991
private static final InterceptingExecutableInvoker executableInvoker = new InterceptingExecutableInvoker();
9092

9193
protected final ClassInfo classInfo;
9294

95+
private LifecycleMethods lifecycleMethods;
9396
private TestInstanceFactory testInstanceFactory;
94-
private List<Method> beforeAllMethods;
95-
private List<Method> afterAllMethods;
9697

9798
ClassBasedTestDescriptor(UniqueId uniqueId, Class<?> testClass, Supplier<String> displayNameSupplier,
9899
JupiterConfiguration configuration) {
99100
super(uniqueId, testClass, displayNameSupplier, ClassSource.from(testClass), configuration);
100101

101102
this.classInfo = new ClassInfo(testClass, configuration);
103+
this.lifecycleMethods = new LifecycleMethods(this.classInfo);
102104
}
103105

104106
ClassBasedTestDescriptor(UniqueId uniqueId, Class<?> testClass, String displayName,
105107
JupiterConfiguration configuration) {
106108
super(uniqueId, displayName, ClassSource.from(testClass), configuration);
107109

108110
this.classInfo = new ClassInfo(testClass, configuration);
111+
this.lifecycleMethods = new LifecycleMethods(this.classInfo);
109112
}
110113

111114
// --- TestClassAware ------------------------------------------------------
@@ -127,6 +130,15 @@ public final String getLegacyReportingName() {
127130
return getTestClass().getName();
128131
}
129132

133+
// --- Validatable ---------------------------------------------------------
134+
135+
@Override
136+
public void validate(DiscoveryIssueReporter reporter) {
137+
List<DiscoveryIssue> discoveryIssues = this.lifecycleMethods.discoveryIssues;
138+
discoveryIssues.forEach(reporter::reportIssue);
139+
discoveryIssues.clear();
140+
}
141+
130142
// --- Node ----------------------------------------------------------------
131143

132144
@Override
@@ -166,24 +178,15 @@ public final JupiterEngineExecutionContext prepare(JupiterEngineExecutionContext
166178
registerExtensionsFromConstructorParameters(registry, getTestClass());
167179
}
168180

169-
this.beforeAllMethods = findBeforeAllMethods(getTestClass(), this.classInfo.lifecycle == Lifecycle.PER_METHOD,
170-
issue -> {
171-
throw new JUnitException(issue.message());
172-
});
173-
this.afterAllMethods = findAfterAllMethods(getTestClass(), this.classInfo.lifecycle == Lifecycle.PER_METHOD,
174-
issue -> {
175-
throw new JUnitException(issue.message());
176-
});
177-
178-
this.beforeAllMethods.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
181+
this.lifecycleMethods.beforeAll.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
179182
// Since registerBeforeEachMethodAdapters() and registerAfterEachMethodAdapters() also
180183
// invoke registerExtensionsFromExecutableParameters(), we invoke those methods before
181184
// invoking registerExtensionsFromExecutableParameters() for @AfterAll methods,
182185
// thereby ensuring proper registration order for extensions registered via @ExtendWith
183186
// on parameters in lifecycle methods.
184187
registerBeforeEachMethodAdapters(registry);
185188
registerAfterEachMethodAdapters(registry);
186-
this.afterAllMethods.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
189+
this.lifecycleMethods.afterAll.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
187190
registerExtensionsFromInstanceFields(registry, getTestClass());
188191

189192
ThrowableCollector throwableCollector = createThrowableCollector();
@@ -258,6 +261,13 @@ public final void after(JupiterEngineExecutionContext context) {
258261
}
259262
}
260263

264+
@Override
265+
public void cleanUp(JupiterEngineExecutionContext context) throws Exception {
266+
super.cleanUp(context);
267+
this.lifecycleMethods = null;
268+
this.testInstanceFactory = null;
269+
}
270+
261271
private TestInstanceFactory resolveTestInstanceFactory(ExtensionRegistry registry) {
262272
List<TestInstanceFactory> factories = registry.getExtensions(TestInstanceFactory.class);
263273

@@ -410,7 +420,7 @@ private void invokeBeforeAllMethods(JupiterEngineExecutionContext context) {
410420
ThrowableCollector throwableCollector = context.getThrowableCollector();
411421
Object testInstance = extensionContext.getTestInstance().orElse(null);
412422

413-
for (Method method : this.beforeAllMethods) {
423+
for (Method method : this.lifecycleMethods.beforeAll) {
414424
throwableCollector.execute(() -> {
415425
try {
416426
executableInvoker.invoke(method, testInstance, extensionContext, registry,
@@ -439,7 +449,7 @@ private void invokeAfterAllMethods(JupiterEngineExecutionContext context) {
439449
ThrowableCollector throwableCollector = context.getThrowableCollector();
440450
Object testInstance = extensionContext.getTestInstance().orElse(null);
441451

442-
this.afterAllMethods.forEach(method -> throwableCollector.execute(() -> {
452+
this.lifecycleMethods.afterAll.forEach(method -> throwableCollector.execute(() -> {
443453
try {
444454
executableInvoker.invoke(method, testInstance, extensionContext, registry,
445455
ReflectiveInterceptorCall.ofVoidMethod(InvocationInterceptor::interceptAfterAllMethod));
@@ -472,17 +482,13 @@ private boolean isPerClassLifecycle(JupiterEngineExecutionContext context) {
472482
}
473483

474484
private void registerBeforeEachMethodAdapters(ExtensionRegistrar registrar) {
475-
List<Method> beforeEachMethods = findBeforeEachMethods(getTestClass(), issue -> {
476-
throw new JUnitException(issue.message());
477-
});
478-
registerMethodsAsExtensions(beforeEachMethods, registrar, this::synthesizeBeforeEachMethodAdapter);
485+
registerMethodsAsExtensions(this.lifecycleMethods.beforeEach, registrar,
486+
this::synthesizeBeforeEachMethodAdapter);
479487
}
480488

481489
private void registerAfterEachMethodAdapters(ExtensionRegistrar registrar) {
482490
// Make a local copy since findAfterEachMethods() returns an immutable list.
483-
List<Method> afterEachMethods = new ArrayList<>(findAfterEachMethods(getTestClass(), issue -> {
484-
throw new JUnitException(issue.message());
485-
}));
491+
List<Method> afterEachMethods = new ArrayList<>(this.lifecycleMethods.afterEach);
486492

487493
// Since the bottom-up ordering of afterEachMethods will later be reversed when the
488494
// synthesized AfterEachMethodAdapters are executed within TestMethodTestDescriptor,
@@ -523,6 +529,7 @@ private void invokeMethodInExtensionContext(Method method, ExtensionContext cont
523529
}
524530

525531
protected static class ClassInfo {
532+
526533
final Class<?> testClass;
527534
final Set<TestTag> tags;
528535
final Lifecycle lifecycle;
@@ -538,4 +545,23 @@ protected static class ClassInfo {
538545
}
539546
}
540547

548+
private static class LifecycleMethods {
549+
550+
private final List<DiscoveryIssue> discoveryIssues = new ArrayList<>();
551+
552+
private final List<Method> beforeAll;
553+
private final List<Method> afterAll;
554+
private final List<Method> beforeEach;
555+
private final List<Method> afterEach;
556+
557+
LifecycleMethods(ClassInfo classInfo) {
558+
Class<?> testClass = classInfo.testClass;
559+
boolean requireStatic = classInfo.lifecycle == Lifecycle.PER_METHOD;
560+
this.beforeAll = findBeforeAllMethods(testClass, requireStatic, discoveryIssues::add);
561+
this.afterAll = findAfterAllMethods(testClass, requireStatic, discoveryIssues::add);
562+
this.beforeEach = findBeforeEachMethods(testClass, discoveryIssues::add);
563+
this.afterEach = findAfterEachMethods(testClass, discoveryIssues::add);
564+
}
565+
}
566+
541567
}

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java

+30-15
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@
1818
import java.util.List;
1919
import java.util.function.Function;
2020
import java.util.function.Predicate;
21-
import java.util.stream.Stream;
2221

2322
import org.junit.jupiter.api.AfterAll;
2423
import org.junit.jupiter.api.AfterEach;
2524
import org.junit.jupiter.api.BeforeAll;
2625
import org.junit.jupiter.api.BeforeEach;
2726
import org.junit.platform.commons.support.HierarchyTraversalMode;
2827
import org.junit.platform.commons.support.ModifierSupport;
28+
import org.junit.platform.commons.util.Preconditions;
2929
import org.junit.platform.commons.util.ReflectionUtils;
3030
import org.junit.platform.engine.DiscoveryIssue;
3131
import org.junit.platform.engine.DiscoveryIssue.Severity;
@@ -69,31 +69,31 @@ private static List<Method> findMethodsAndCheckStatic(Class<?> testClass, boolea
6969
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode,
7070
DiscoveryIssueReporter issueReporter) {
7171

72-
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter) //
73-
.filter(requireStatic ? isStatic(annotationType, issueReporter) : __ -> true) //
74-
.collect(toUnmodifiableList());
72+
Predicate<Method> additionalCondition = requireStatic ? isStatic(annotationType, issueReporter) : __ -> true;
73+
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter,
74+
additionalCondition);
7575
}
7676

7777
private static List<Method> findMethodsAndCheckNonStatic(Class<?> testClass,
7878
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode,
7979
DiscoveryIssueReporter issueReporter) {
8080

81-
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter) //
82-
.filter(isNotStatic(annotationType, issueReporter)) //
83-
.collect(toUnmodifiableList());
81+
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter,
82+
isNotStatic(annotationType, issueReporter));
8483
}
8584

86-
private static Stream<Method> findMethodsAndCheckVoidReturnType(Class<?> testClass,
85+
private static List<Method> findMethodsAndCheckVoidReturnType(Class<?> testClass,
8786
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode,
88-
DiscoveryIssueReporter issueReporter) {
87+
DiscoveryIssueReporter issueReporter, Predicate<? super Method> additionalCondition) {
8988

9089
return findAnnotatedMethods(testClass, annotationType, traversalMode).stream() //
91-
.filter(returnsPrimitiveVoid(annotationType, issueReporter));
90+
.filter(allOf(returnsPrimitiveVoid(annotationType, issueReporter), additionalCondition)) //
91+
.collect(toUnmodifiableList());
9292
}
9393

9494
private static Predicate<Method> isStatic(Class<? extends Annotation> annotationType,
9595
DiscoveryIssueReporter issueReporter) {
96-
return DiscoveryIssueReportingPredicate.from(issueReporter, ModifierSupport::isStatic, method -> {
96+
return DiscoveryIssueReportingPredicate.from(ModifierSupport::isStatic, issueReporter, method -> {
9797
String message = String.format(
9898
"@%s method '%s' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).",
9999
annotationType.getSimpleName(), method.toGenericString());
@@ -103,7 +103,7 @@ private static Predicate<Method> isStatic(Class<? extends Annotation> annotation
103103

104104
private static Predicate<Method> isNotStatic(Class<? extends Annotation> annotationType,
105105
DiscoveryIssueReporter issueReporter) {
106-
return DiscoveryIssueReportingPredicate.from(issueReporter, ModifierSupport::isNotStatic, method -> {
106+
return DiscoveryIssueReportingPredicate.from(ModifierSupport::isNotStatic, issueReporter, method -> {
107107
String message = String.format("@%s method '%s' must not be static.", annotationType.getSimpleName(),
108108
method.toGenericString());
109109
return DiscoveryIssue.builder(Severity.ERROR, message).source(MethodSource.from(method)).build();
@@ -112,16 +112,31 @@ private static Predicate<Method> isNotStatic(Class<? extends Annotation> annotat
112112

113113
private static Predicate<Method> returnsPrimitiveVoid(Class<? extends Annotation> annotationType,
114114
DiscoveryIssueReporter issueReporter) {
115-
return DiscoveryIssueReportingPredicate.from(issueReporter, ReflectionUtils::returnsPrimitiveVoid, method -> {
115+
return DiscoveryIssueReportingPredicate.from(ReflectionUtils::returnsPrimitiveVoid, issueReporter, method -> {
116116
String message = String.format("@%s method '%s' must not return a value.", annotationType.getSimpleName(),
117117
method.toGenericString());
118118
return DiscoveryIssue.builder(Severity.ERROR, message).source(MethodSource.from(method)).build();
119119
});
120120
}
121121

122-
abstract static class DiscoveryIssueReportingPredicate<T> implements Predicate<T> {
122+
@SafeVarargs
123+
@SuppressWarnings("varargs")
124+
private static <T> Predicate<T> allOf(Predicate<? super T>... predicates) {
125+
Preconditions.notNull(predicates, "predicates must not be null");
126+
Preconditions.notEmpty(predicates, "predicates must not be empty");
127+
Preconditions.containsNoNullElements(predicates, "predicates must not contain null elements");
128+
return value -> {
129+
boolean result = true;
130+
for (Predicate<? super T> predicate : predicates) {
131+
result &= predicate.test(value);
132+
}
133+
return result;
134+
};
135+
}
136+
137+
private abstract static class DiscoveryIssueReportingPredicate<T> implements Predicate<T> {
123138

124-
static <T> DiscoveryIssueReportingPredicate<T> from(DiscoveryIssueReporter reporter, Predicate<T> predicate,
139+
static <T> DiscoveryIssueReportingPredicate<T> from(Predicate<T> predicate, DiscoveryIssueReporter reporter,
125140
Function<T, DiscoveryIssue> issueBuilder) {
126141
return new DiscoveryIssueReportingPredicate<T>(reporter) {
127142
@Override

‎junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright 2015-2025 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.jupiter.engine.descriptor;
12+
13+
import static org.apiguardian.api.API.Status.INTERNAL;
14+
15+
import org.apiguardian.api.API;
16+
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
17+
18+
/**
19+
* Interface for descriptors that can be validated during discovery.
20+
*
21+
* @since 5.13
22+
*/
23+
@API(status = INTERNAL, since = "5.13")
24+
public interface Validatable {
25+
26+
/**
27+
* Validate the state of this descriptor and report any issues found to the
28+
* supplied {@link DiscoveryIssueReporter}.
29+
*/
30+
void validate(DiscoveryIssueReporter reporter);
31+
32+
}

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

+9-5
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.junit.jupiter.engine.config.JupiterConfiguration;
1717
import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor;
1818
import org.junit.jupiter.engine.descriptor.JupiterTestDescriptor;
19+
import org.junit.jupiter.engine.descriptor.Validatable;
1920
import org.junit.jupiter.engine.discovery.predicates.IsTestClassWithTests;
2021
import org.junit.platform.engine.EngineDiscoveryRequest;
2122
import org.junit.platform.engine.TestDescriptor;
@@ -44,12 +45,15 @@ public class DiscoverySelectorResolver {
4445
new ClassOrderingVisitor(getConfiguration(ctx)), //
4546
new MethodOrderingVisitor(getConfiguration(ctx)), //
4647
descriptor -> {
47-
if (descriptor instanceof JupiterTestDescriptor) {
48-
((JupiterTestDescriptor) descriptor).prunePriorToFiltering();
48+
if (descriptor instanceof Validatable) {
49+
((Validatable) descriptor).validate(ctx.getIssueReporter());
4950
}
50-
} //
51-
)) //
52-
.build();
51+
})) //
52+
.addTestDescriptorVisitor(ctx -> descriptor -> {
53+
if (descriptor instanceof JupiterTestDescriptor) {
54+
((JupiterTestDescriptor) descriptor).prunePriorToFiltering();
55+
}
56+
}).build();
5357

5458
private static JupiterConfiguration getConfiguration(InitializationContext<JupiterEngineDescriptor> context) {
5559
return context.getEngineDescriptor().getConfiguration();

‎junit-jupiter-engine/src/nativeImage/initialize-at-build-time

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ org.junit.jupiter.engine.config.InstantiatingConfigurationParameterConverter
66
org.junit.jupiter.engine.descriptor.ClassTestDescriptor
77
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor
88
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor$ClassInfo
9+
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor$LifecycleMethods
910
org.junit.jupiter.engine.descriptor.DynamicDescendantFilter
1011
org.junit.jupiter.engine.descriptor.ExclusiveResourceCollector$1
1112
org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor

0 commit comments

Comments
 (0)