Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report discovery issues for Jupiter lifecycle methods #4398

Merged
merged 4 commits into from
Mar 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,12 @@
import org.junit.platform.commons.util.ReflectionUtils;
import org.junit.platform.commons.util.StringUtils;
import org.junit.platform.commons.util.UnrecoverableExceptions;
import org.junit.platform.engine.DiscoveryIssue;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.TestTag;
import org.junit.platform.engine.UniqueId;
import org.junit.platform.engine.support.descriptor.ClassSource;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
import org.junit.platform.engine.support.hierarchical.ThrowableCollector;

/**
Expand All @@ -84,28 +86,29 @@
*/
@API(status = INTERNAL, since = "5.5")
public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor
implements ResourceLockAware, TestClassAware {
implements ResourceLockAware, TestClassAware, Validatable {

private static final InterceptingExecutableInvoker executableInvoker = new InterceptingExecutableInvoker();

protected final ClassInfo classInfo;

private LifecycleMethods lifecycleMethods;
private TestInstanceFactory testInstanceFactory;
private List<Method> beforeAllMethods;
private List<Method> afterAllMethods;

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

this.classInfo = new ClassInfo(testClass, configuration);
this.lifecycleMethods = new LifecycleMethods(this.classInfo);
}

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

this.classInfo = new ClassInfo(testClass, configuration);
this.lifecycleMethods = new LifecycleMethods(this.classInfo);
}

// --- TestClassAware ------------------------------------------------------
Expand All @@ -127,6 +130,15 @@ public final String getLegacyReportingName() {
return getTestClass().getName();
}

// --- Validatable ---------------------------------------------------------

@Override
public void validate(DiscoveryIssueReporter reporter) {
List<DiscoveryIssue> discoveryIssues = this.lifecycleMethods.discoveryIssues;
discoveryIssues.forEach(reporter::reportIssue);
discoveryIssues.clear();
}

// --- Node ----------------------------------------------------------------

@Override
Expand Down Expand Up @@ -166,18 +178,15 @@ public final JupiterEngineExecutionContext prepare(JupiterEngineExecutionContext
registerExtensionsFromConstructorParameters(registry, getTestClass());
}

this.beforeAllMethods = findBeforeAllMethods(getTestClass(), this.classInfo.lifecycle == Lifecycle.PER_METHOD);
this.afterAllMethods = findAfterAllMethods(getTestClass(), this.classInfo.lifecycle == Lifecycle.PER_METHOD);

this.beforeAllMethods.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
this.lifecycleMethods.beforeAll.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
// Since registerBeforeEachMethodAdapters() and registerAfterEachMethodAdapters() also
// invoke registerExtensionsFromExecutableParameters(), we invoke those methods before
// invoking registerExtensionsFromExecutableParameters() for @AfterAll methods,
// thereby ensuring proper registration order for extensions registered via @ExtendWith
// on parameters in lifecycle methods.
registerBeforeEachMethodAdapters(registry);
registerAfterEachMethodAdapters(registry);
this.afterAllMethods.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
this.lifecycleMethods.afterAll.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
registerExtensionsFromInstanceFields(registry, getTestClass());

ThrowableCollector throwableCollector = createThrowableCollector();
Expand Down Expand Up @@ -252,6 +261,13 @@ public final void after(JupiterEngineExecutionContext context) {
}
}

@Override
public void cleanUp(JupiterEngineExecutionContext context) throws Exception {
super.cleanUp(context);
this.lifecycleMethods = null;
this.testInstanceFactory = null;
}

private TestInstanceFactory resolveTestInstanceFactory(ExtensionRegistry registry) {
List<TestInstanceFactory> factories = registry.getExtensions(TestInstanceFactory.class);

Expand Down Expand Up @@ -404,7 +420,7 @@ private void invokeBeforeAllMethods(JupiterEngineExecutionContext context) {
ThrowableCollector throwableCollector = context.getThrowableCollector();
Object testInstance = extensionContext.getTestInstance().orElse(null);

for (Method method : this.beforeAllMethods) {
for (Method method : this.lifecycleMethods.beforeAll) {
throwableCollector.execute(() -> {
try {
executableInvoker.invoke(method, testInstance, extensionContext, registry,
Expand Down Expand Up @@ -433,7 +449,7 @@ private void invokeAfterAllMethods(JupiterEngineExecutionContext context) {
ThrowableCollector throwableCollector = context.getThrowableCollector();
Object testInstance = extensionContext.getTestInstance().orElse(null);

this.afterAllMethods.forEach(method -> throwableCollector.execute(() -> {
this.lifecycleMethods.afterAll.forEach(method -> throwableCollector.execute(() -> {
try {
executableInvoker.invoke(method, testInstance, extensionContext, registry,
ReflectiveInterceptorCall.ofVoidMethod(InvocationInterceptor::interceptAfterAllMethod));
Expand Down Expand Up @@ -466,13 +482,13 @@ private boolean isPerClassLifecycle(JupiterEngineExecutionContext context) {
}

private void registerBeforeEachMethodAdapters(ExtensionRegistrar registrar) {
List<Method> beforeEachMethods = findBeforeEachMethods(getTestClass());
registerMethodsAsExtensions(beforeEachMethods, registrar, this::synthesizeBeforeEachMethodAdapter);
registerMethodsAsExtensions(this.lifecycleMethods.beforeEach, registrar,
this::synthesizeBeforeEachMethodAdapter);
}

private void registerAfterEachMethodAdapters(ExtensionRegistrar registrar) {
// Make a local copy since findAfterEachMethods() returns an immutable list.
List<Method> afterEachMethods = new ArrayList<>(findAfterEachMethods(getTestClass()));
List<Method> afterEachMethods = new ArrayList<>(this.lifecycleMethods.afterEach);

// Since the bottom-up ordering of afterEachMethods will later be reversed when the
// synthesized AfterEachMethodAdapters are executed within TestMethodTestDescriptor,
Expand Down Expand Up @@ -513,6 +529,7 @@ private void invokeMethodInExtensionContext(Method method, ExtensionContext cont
}

protected static class ClassInfo {

final Class<?> testClass;
final Set<TestTag> tags;
final Lifecycle lifecycle;
Expand All @@ -528,4 +545,23 @@ protected static class ClassInfo {
}
}

private static class LifecycleMethods {

private final List<DiscoveryIssue> discoveryIssues = new ArrayList<>();

private final List<Method> beforeAll;
private final List<Method> afterAll;
private final List<Method> beforeEach;
private final List<Method> afterEach;

LifecycleMethods(ClassInfo classInfo) {
Class<?> testClass = classInfo.testClass;
boolean requireStatic = classInfo.lifecycle == Lifecycle.PER_METHOD;
this.beforeAll = findBeforeAllMethods(testClass, requireStatic, discoveryIssues::add);
this.afterAll = findAfterAllMethods(testClass, requireStatic, discoveryIssues::add);
this.beforeEach = findBeforeEachMethods(testClass, discoveryIssues::add);
this.afterEach = findAfterEachMethods(testClass, discoveryIssues::add);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
package org.junit.jupiter.engine.descriptor;

import static org.junit.platform.commons.support.AnnotationSupport.findAnnotatedMethods;
import static org.junit.platform.commons.util.ReflectionUtils.returnsPrimitiveVoid;
import static org.junit.platform.commons.util.CollectionUtils.toUnmodifiableList;
import static org.junit.platform.engine.support.discovery.DiscoveryIssueReporter.Condition.allOf;

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
Expand All @@ -21,9 +22,14 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.platform.commons.JUnitException;
import org.junit.platform.commons.support.HierarchyTraversalMode;
import org.junit.platform.commons.support.ModifierSupport;
import org.junit.platform.commons.util.ReflectionUtils;
import org.junit.platform.engine.DiscoveryIssue;
import org.junit.platform.engine.DiscoveryIssue.Severity;
import org.junit.platform.engine.support.descriptor.MethodSource;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter.Condition;

/**
* Collection of utilities for working with test lifecycle methods.
Expand All @@ -36,68 +42,95 @@ private LifecycleMethodUtils() {
/* no-op */
}

static List<Method> findBeforeAllMethods(Class<?> testClass, boolean requireStatic) {
return findMethodsAndAssertStatic(testClass, requireStatic, BeforeAll.class, HierarchyTraversalMode.TOP_DOWN);
static List<Method> findBeforeAllMethods(Class<?> testClass, boolean requireStatic,
DiscoveryIssueReporter issueReporter) {
return findMethodsAndCheckStatic(testClass, requireStatic, BeforeAll.class, HierarchyTraversalMode.TOP_DOWN,
issueReporter);
}

static List<Method> findAfterAllMethods(Class<?> testClass, boolean requireStatic) {
return findMethodsAndAssertStatic(testClass, requireStatic, AfterAll.class, HierarchyTraversalMode.BOTTOM_UP);
static List<Method> findAfterAllMethods(Class<?> testClass, boolean requireStatic,
DiscoveryIssueReporter issueReporter) {
return findMethodsAndCheckStatic(testClass, requireStatic, AfterAll.class, HierarchyTraversalMode.BOTTOM_UP,
issueReporter);
}

static List<Method> findBeforeEachMethods(Class<?> testClass) {
return findMethodsAndAssertNonStatic(testClass, BeforeEach.class, HierarchyTraversalMode.TOP_DOWN);
static List<Method> findBeforeEachMethods(Class<?> testClass, DiscoveryIssueReporter issueReporter) {
return findMethodsAndCheckNonStatic(testClass, BeforeEach.class, HierarchyTraversalMode.TOP_DOWN,
issueReporter);
}

static List<Method> findAfterEachMethods(Class<?> testClass) {
return findMethodsAndAssertNonStatic(testClass, AfterEach.class, HierarchyTraversalMode.BOTTOM_UP);
static List<Method> findAfterEachMethods(Class<?> testClass, DiscoveryIssueReporter issueReporter) {
return findMethodsAndCheckNonStatic(testClass, AfterEach.class, HierarchyTraversalMode.BOTTOM_UP,
issueReporter);
}

private static List<Method> findMethodsAndAssertStatic(Class<?> testClass, boolean requireStatic,
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode) {
private static List<Method> findMethodsAndCheckStatic(Class<?> testClass, boolean requireStatic,
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode,
DiscoveryIssueReporter issueReporter) {

List<Method> methods = findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode);
if (requireStatic) {
methods.forEach(method -> assertStatic(annotationType, method));
}
return methods;
Condition<Method> additionalCondition = requireStatic ? isStatic(annotationType, issueReporter) : __ -> true;
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter,
additionalCondition);
}

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

List<Method> methods = findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode);
methods.forEach(method -> assertNonStatic(annotationType, method));
return methods;
return findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode, issueReporter,
isNotStatic(annotationType, issueReporter));
}

private static List<Method> findMethodsAndCheckVoidReturnType(Class<?> testClass,
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode) {
Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode,
DiscoveryIssueReporter issueReporter, Condition<? super Method> additionalCondition) {

List<Method> methods = findAnnotatedMethods(testClass, annotationType, traversalMode);
methods.forEach(method -> assertVoid(annotationType, method));
return methods;
return findAnnotatedMethods(testClass, annotationType, traversalMode).stream() //
.peek(isNotPrivate(annotationType, issueReporter)) //
.filter(allOf(returnsPrimitiveVoid(annotationType, issueReporter), additionalCondition)) //
.collect(toUnmodifiableList());
}

private static void assertStatic(Class<? extends Annotation> annotationType, Method method) {
if (ModifierSupport.isNotStatic(method)) {
throw new JUnitException(String.format(
private static Condition<Method> isStatic(Class<? extends Annotation> annotationType,
DiscoveryIssueReporter issueReporter) {
return issueReporter.createReportingCondition(ModifierSupport::isStatic, method -> {
String message = String.format(
"@%s method '%s' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).",
annotationType.getSimpleName(), method.toGenericString()));
}
annotationType.getSimpleName(), method.toGenericString());
return createIssue(Severity.ERROR, message, method);
});
}

private static void assertNonStatic(Class<? extends Annotation> annotationType, Method method) {
if (ModifierSupport.isStatic(method)) {
throw new JUnitException(String.format("@%s method '%s' must not be static.",
annotationType.getSimpleName(), method.toGenericString()));
}
private static Condition<Method> isNotStatic(Class<? extends Annotation> annotationType,
DiscoveryIssueReporter issueReporter) {
return issueReporter.createReportingCondition(ModifierSupport::isNotStatic, method -> {
String message = String.format("@%s method '%s' must not be static.", annotationType.getSimpleName(),
method.toGenericString());
return createIssue(Severity.ERROR, message, method);
});
}

private static void assertVoid(Class<? extends Annotation> annotationType, Method method) {
if (!returnsPrimitiveVoid(method)) {
throw new JUnitException(String.format("@%s method '%s' must not return a value.",
annotationType.getSimpleName(), method.toGenericString()));
}
private static Condition<Method> isNotPrivate(Class<? extends Annotation> annotationType,
DiscoveryIssueReporter issueReporter) {
return issueReporter.createReportingCondition(ModifierSupport::isNotPrivate, method -> {
String message = String.format(
"@%s method '%s' should not be private. This will be disallowed in a future release.",
annotationType.getSimpleName(), method.toGenericString());
return createIssue(Severity.DEPRECATION, message, method);
});
}

private static Condition<Method> returnsPrimitiveVoid(Class<? extends Annotation> annotationType,
DiscoveryIssueReporter issueReporter) {
return issueReporter.createReportingCondition(ReflectionUtils::returnsPrimitiveVoid, method -> {
String message = String.format("@%s method '%s' must not return a value.", annotationType.getSimpleName(),
method.toGenericString());
return createIssue(Severity.ERROR, message, method);
});
}

private static DiscoveryIssue createIssue(Severity severity, String message, Method method) {
return DiscoveryIssue.builder(severity, message).source(MethodSource.from(method)).build();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2015-2025 the original author or authors.
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License v2.0 which
* accompanies this distribution and is available at
*
* https://www.eclipse.org/legal/epl-v20.html
*/

package org.junit.jupiter.engine.descriptor;

import static org.apiguardian.api.API.Status.INTERNAL;

import org.apiguardian.api.API;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;

/**
* Interface for descriptors that can be validated during discovery.
*
* @since 5.13
*/
@API(status = INTERNAL, since = "5.13")
public interface Validatable {

/**
* Validate the state of this descriptor and report any issues found to the
* supplied {@link DiscoveryIssueReporter}.
*/
void validate(DiscoveryIssueReporter reporter);

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.junit.jupiter.engine.config.JupiterConfiguration;
import org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor;
import org.junit.jupiter.engine.descriptor.JupiterTestDescriptor;
import org.junit.jupiter.engine.descriptor.Validatable;
import org.junit.jupiter.engine.discovery.predicates.IsTestClassWithTests;
import org.junit.platform.engine.EngineDiscoveryRequest;
import org.junit.platform.engine.TestDescriptor;
Expand Down Expand Up @@ -44,12 +45,15 @@ public class DiscoverySelectorResolver {
new ClassOrderingVisitor(getConfiguration(ctx)), //
new MethodOrderingVisitor(getConfiguration(ctx)), //
descriptor -> {
if (descriptor instanceof JupiterTestDescriptor) {
((JupiterTestDescriptor) descriptor).prunePriorToFiltering();
if (descriptor instanceof Validatable) {
((Validatable) descriptor).validate(ctx.getIssueReporter());
}
} //
)) //
.build();
})) //
.addTestDescriptorVisitor(ctx -> descriptor -> {
if (descriptor instanceof JupiterTestDescriptor) {
((JupiterTestDescriptor) descriptor).prunePriorToFiltering();
}
}).build();

private static JupiterConfiguration getConfiguration(InitializationContext<JupiterEngineDescriptor> context) {
return context.getEngineDescriptor().getConfiguration();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ org.junit.jupiter.engine.config.InstantiatingConfigurationParameterConverter
org.junit.jupiter.engine.descriptor.ClassTestDescriptor
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor$ClassInfo
org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor$LifecycleMethods
org.junit.jupiter.engine.descriptor.DynamicDescendantFilter
org.junit.jupiter.engine.descriptor.ExclusiveResourceCollector$1
org.junit.jupiter.engine.descriptor.JupiterEngineDescriptor
Expand Down
Loading
Loading