Skip to content

Draft API for reporting discovery issues #4380

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

Draft API for reporting discovery issues #4380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f0807a1
Draft API for reporting discovery issues
marcphilipp Mar 10, 2025
a5b4ff5
Apply feedback from team call
marcphilipp Mar 11, 2025
0dae85d
Add missing `API` annotations
marcphilipp Mar 11, 2025
232466a
Remove issue collecting from `abortOnFailure` listener
marcphilipp Mar 11, 2025
97aba67
Implement new method
marcphilipp Mar 11, 2025
d79419a
Handle error severity when logging
marcphilipp Mar 11, 2025
57d5170
Report discovery issues as engine-level execution failure
marcphilipp Mar 11, 2025
dfaa88a
No longer abort on selector processing issues
marcphilipp Mar 11, 2025
7c8d2ba
List issues in exception message
marcphilipp Mar 11, 2025
8b76090
Avoid failed assert in IntelliJ
marcphilipp Mar 11, 2025
f3946a9
Use original display name from engine descriptor
marcphilipp Mar 11, 2025
e414c39
Don't log by default to avoid issues from being reported multiple times
marcphilipp Mar 11, 2025
cee6064
Print IDE-compatible links for method/class sources
marcphilipp Mar 11, 2025
a89235c
Polishing
marcphilipp Mar 13, 2025
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
32 changes: 26 additions & 6 deletions ...er-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,17 @@
import org.junit.jupiter.engine.extension.ExtensionRegistry;
import org.junit.jupiter.engine.extension.MutableExtensionRegistry;
import org.junit.platform.commons.JUnitException;
import org.junit.platform.commons.support.ModifierSupport;
import org.junit.platform.commons.util.ExceptionUtils;
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.Severity;
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.descriptor.MethodSource;
import org.junit.platform.engine.support.hierarchical.ThrowableCollector;

/**
Expand All @@ -84,7 +87,7 @@
*/
@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();

Expand All @@ -95,8 +98,8 @@ public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor

private ExecutionMode defaultChildExecutionMode;
private TestInstanceFactory testInstanceFactory;
private List<Method> beforeAllMethods;
private List<Method> afterAllMethods;
private final List<Method> beforeAllMethods;
private final List<Method> afterAllMethods;

ClassBasedTestDescriptor(UniqueId uniqueId, Class<?> testClass, Supplier<String> displayNameSupplier,
JupiterConfiguration configuration) {
Expand All @@ -107,6 +110,8 @@ public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor
this.lifecycle = getTestInstanceLifecycle(testClass, configuration);
this.defaultChildExecutionMode = (this.lifecycle == Lifecycle.PER_CLASS ? ExecutionMode.SAME_THREAD : null);
this.exclusiveResourceCollector = ExclusiveResourceCollector.from(testClass);
this.beforeAllMethods = findBeforeAllMethods(this.testClass, this.lifecycle == Lifecycle.PER_METHOD);
this.afterAllMethods = findAfterAllMethods(this.testClass, this.lifecycle == Lifecycle.PER_METHOD);
}

ClassBasedTestDescriptor(UniqueId uniqueId, Class<?> testClass, String displayName,
Expand All @@ -118,6 +123,8 @@ public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor
this.lifecycle = getTestInstanceLifecycle(testClass, configuration);
this.defaultChildExecutionMode = (this.lifecycle == Lifecycle.PER_CLASS ? ExecutionMode.SAME_THREAD : null);
this.exclusiveResourceCollector = ExclusiveResourceCollector.from(testClass);
this.beforeAllMethods = findBeforeAllMethods(this.testClass, this.lifecycle == Lifecycle.PER_METHOD);
this.afterAllMethods = findAfterAllMethods(this.testClass, this.lifecycle == Lifecycle.PER_METHOD);
}

// --- TestClassAware ------------------------------------------------------
Expand All @@ -139,6 +146,22 @@ public final String getLegacyReportingName() {
return this.testClass.getName();
}

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

@Override
public void validate(IssueReporter reporter) {
beforeAllMethods.stream() //
.filter(ModifierSupport::isPrivate) //
.forEach(method -> reporter.reportIssue(Severity.DEPRECATION,
String.format("@BeforeAll method [%s] should not be private.", method.toGenericString()),
b -> b.source(MethodSource.from(method))));
afterAllMethods.stream() //
.filter(ModifierSupport::isPrivate) //
.forEach(method -> reporter.reportIssue(Severity.DEPRECATION,
String.format("@AfterAll method [%s] should not be private.", method.toGenericString()),
b -> b.source(MethodSource.from(method))));
Comment on lines +158 to +162
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Example of a engine-specific "post-discovery" validation that reports private lifecycle methods as deprecated (also would need to be done for @BeforeEach etc.).

}

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

@Override
Expand Down Expand Up @@ -178,9 +201,6 @@ public final JupiterEngineExecutionContext prepare(JupiterEngineExecutionContext
registerExtensionsFromConstructorParameters(registry, this.testClass);
}

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

this.beforeAllMethods.forEach(method -> registerExtensionsFromExecutableParameters(registry, method));
// Since registerBeforeEachMethodAdapters() and registerAfterEachMethodAdapters() also
// invoke registerExtensionsFromExecutableParameters(), we invoke those methods before
Expand Down
40 changes: 40 additions & 0 deletions junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* 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 java.util.function.UnaryOperator.identity;
import static org.apiguardian.api.API.Status.INTERNAL;

import java.util.function.UnaryOperator;

import org.apiguardian.api.API;
import org.junit.platform.engine.DiscoveryIssue;
import org.junit.platform.engine.DiscoveryIssue.Severity;

/**
* @since 5.13
*/
@API(status = INTERNAL, since = "5.13")
public interface Validatable {

void validate(IssueReporter issueReporter);

interface IssueReporter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚲🏠 Why did you choose issue instead of problem?


default void reportIssue(Severity severity, String message) {
reportIssue(severity, message, identity());
}

void reportIssue(Severity severity, String message, UnaryOperator<DiscoveryIssue.Builder> issueBuilder);

}

}
40 changes: 28 additions & 12 deletions ...iter-engine/src/main/java/org/junit/jupiter/engine/discovery/AbstractOrderingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@
import java.util.stream.Stream;

import org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor;
import org.junit.platform.commons.logging.Logger;
import org.junit.platform.commons.logging.LoggerFactory;
import org.junit.platform.commons.util.UnrecoverableExceptions;
import org.junit.platform.engine.DiscoveryIssue;
import org.junit.platform.engine.DiscoveryIssue.Severity;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;

/**
* Abstract base class for {@linkplain TestDescriptor.Visitor visitors} that
Expand All @@ -40,7 +41,11 @@
abstract class AbstractOrderingVisitor<PARENT extends TestDescriptor, CHILD extends TestDescriptor, WRAPPER extends AbstractAnnotatedDescriptorWrapper<?>>
implements TestDescriptor.Visitor {

private static final Logger logger = LoggerFactory.getLogger(AbstractOrderingVisitor.class);
private final DiscoveryIssueReporter issueReporter;

AbstractOrderingVisitor(DiscoveryIssueReporter issueReporter) {
this.issueReporter = issueReporter;
}

@SuppressWarnings("unchecked")
protected void doWithMatchingDescriptor(Class<PARENT> parentTestDescriptorType, TestDescriptor testDescriptor,
Expand All @@ -53,7 +58,12 @@ protected void doWithMatchingDescriptor(Class<PARENT> parentTestDescriptorType,
}
catch (Throwable t) {
UnrecoverableExceptions.rethrowIfUnrecoverable(t);
logger.error(t, () -> errorMessageBuilder.apply(parentTestDescriptor));
String message = errorMessageBuilder.apply(parentTestDescriptor);
AbstractOrderingVisitor.this.issueReporter.reportIssue( //
DiscoveryIssue.builder(Severity.WARNING, message) //
.source(testDescriptor.getSource()) //
.cause(t) //
.build());
}
}
}
Expand All @@ -78,7 +88,7 @@ protected void orderChildrenTestDescriptors(TestDescriptor parentTestDescriptor,
Stream<TestDescriptor> nonMatchingTestDescriptors = children.stream()//
.filter(childTestDescriptor -> !matchingChildrenType.isInstance(childTestDescriptor));

descriptorWrapperOrderer.orderWrappers(matchingDescriptorWrappers);
descriptorWrapperOrderer.orderWrappers(parentTestDescriptor, matchingDescriptorWrappers);

Stream<TestDescriptor> orderedTestDescriptors = matchingDescriptorWrappers.stream()//
.map(AbstractAnnotatedDescriptorWrapper::getTestDescriptor);
Expand Down Expand Up @@ -149,18 +159,18 @@ private boolean canOrderWrappers() {
return this.orderingAction != null;
}

private void orderWrappers(List<WRAPPER> wrappers) {
private void orderWrappers(TestDescriptor parentTestDescriptor, List<WRAPPER> wrappers) {
List<WRAPPER> orderedWrappers = new ArrayList<>(wrappers);
this.orderingAction.accept(orderedWrappers);
Map<Object, Integer> distinctWrappersToIndex = distinctWrappersToIndex(orderedWrappers);

int difference = orderedWrappers.size() - wrappers.size();
int distinctDifference = distinctWrappersToIndex.size() - wrappers.size();
if (difference > 0) { // difference >= distinctDifference
logDescriptorsAddedWarning(difference);
logDescriptorsAddedWarning(parentTestDescriptor, difference);
}
if (distinctDifference < 0) { // distinctDifference <= difference
logDescriptorsRemovedWarning(distinctDifference);
logDescriptorsRemovedWarning(parentTestDescriptor, distinctDifference);
}

wrappers.sort(comparing(wrapper -> distinctWrappersToIndex.getOrDefault(wrapper, -1)));
Expand All @@ -178,12 +188,18 @@ private Map<Object, Integer> distinctWrappersToIndex(List<?> wrappers) {
return toIndex;
}

private void logDescriptorsAddedWarning(int number) {
logger.warn(() -> this.descriptorsAddedMessageGenerator.generateMessage(number));
private void logDescriptorsAddedWarning(TestDescriptor parentTestDescriptor, int number) {
reportWarning(parentTestDescriptor, this.descriptorsAddedMessageGenerator.generateMessage(number));
}

private void logDescriptorsRemovedWarning(TestDescriptor parentTestDescriptor, int number) {
reportWarning(parentTestDescriptor, this.descriptorsRemovedMessageGenerator.generateMessage(number));
}

private void logDescriptorsRemovedWarning(int number) {
logger.warn(() -> this.descriptorsRemovedMessageGenerator.generateMessage(Math.abs(number)));
private void reportWarning(TestDescriptor parentTestDescriptor, String message) {
AbstractOrderingVisitor.this.issueReporter //
.reportIssue(DiscoveryIssue.builder(Severity.WARNING, message) //
.source(parentTestDescriptor.getSource()));
}

}
Expand Down
4 changes: 3 additions & 1 deletion ...jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/ClassOrderingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.platform.commons.support.AnnotationSupport;
import org.junit.platform.commons.support.ReflectionSupport;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;

/**
* @since 5.8
Expand All @@ -31,7 +32,8 @@ class ClassOrderingVisitor

private final JupiterConfiguration configuration;

ClassOrderingVisitor(JupiterConfiguration configuration) {
ClassOrderingVisitor(DiscoveryIssueReporter issueReporter, JupiterConfiguration configuration) {
super(issueReporter);
this.configuration = configuration;
}

Expand Down
5 changes: 3 additions & 2 deletions ...er-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@ public class DiscoverySelectorResolver {
.addClassContainerSelectorResolver(new IsTestClassWithTests()) //
.addSelectorResolver(ctx -> new ClassSelectorResolver(ctx.getClassNameFilter(), getConfiguration(ctx))) //
.addSelectorResolver(ctx -> new MethodSelectorResolver(getConfiguration(ctx))) //
.addTestDescriptorVisitor(ctx -> new ClassOrderingVisitor(getConfiguration(ctx))) //
.addTestDescriptorVisitor(ctx -> new MethodOrderingVisitor(getConfiguration(ctx))) //
.addTestDescriptorVisitor(ctx -> new ClassOrderingVisitor(ctx.getIssueReporter(), getConfiguration(ctx))) //
.addTestDescriptorVisitor(ctx -> new MethodOrderingVisitor(ctx.getIssueReporter(), getConfiguration(ctx))) //
.addTestDescriptorVisitor(ctx -> new ValidatingVisitor(ctx.getIssueReporter())) //
Copy link
Member Author

@marcphilipp marcphilipp Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Visitors registered via EngineDiscoveryRequestResolver can use the DiscoveryIssueReporter to report issues.

.addTestDescriptorVisitor(ctx -> descriptor -> {
if (descriptor instanceof JupiterTestDescriptor) {
((JupiterTestDescriptor) descriptor).prunePriorToFiltering();
Expand Down
4 changes: 3 additions & 1 deletion ...upiter-engine/src/main/java/org/junit/jupiter/engine/discovery/MethodOrderingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.junit.jupiter.engine.descriptor.MethodBasedTestDescriptor;
import org.junit.platform.commons.support.ReflectionSupport;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.support.discovery.DiscoveryIssueReporter;

/**
* @since 5.5
Expand All @@ -33,7 +34,8 @@ class MethodOrderingVisitor

private final JupiterConfiguration configuration;

MethodOrderingVisitor(JupiterConfiguration configuration) {
MethodOrderingVisitor(DiscoveryIssueReporter issueReporter, JupiterConfiguration configuration) {
super(issueReporter);
this.configuration = configuration;
}

Expand Down
34 changes: 17 additions & 17 deletions ...piter-engine/src/main/java/org/junit/jupiter/engine/discovery/MethodSelectorResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
import org.junit.jupiter.engine.discovery.predicates.IsTestFactoryMethod;
import org.junit.jupiter.engine.discovery.predicates.IsTestMethod;
import org.junit.jupiter.engine.discovery.predicates.IsTestTemplateMethod;
import org.junit.platform.commons.logging.Logger;
import org.junit.platform.commons.logging.LoggerFactory;
import org.junit.platform.commons.util.ClassUtils;
import org.junit.platform.engine.DiscoveryIssue;
import org.junit.platform.engine.DiscoveryIssue.Severity;
import org.junit.platform.engine.DiscoverySelector;
import org.junit.platform.engine.TestDescriptor;
import org.junit.platform.engine.UniqueId;
Expand All @@ -52,14 +52,14 @@
import org.junit.platform.engine.discovery.MethodSelector;
import org.junit.platform.engine.discovery.NestedMethodSelector;
import org.junit.platform.engine.discovery.UniqueIdSelector;
import org.junit.platform.engine.support.descriptor.MethodSource;
import org.junit.platform.engine.support.discovery.SelectorResolver;

/**
* @since 5.5
*/
class MethodSelectorResolver implements SelectorResolver {

private static final Logger logger = LoggerFactory.getLogger(MethodSelectorResolver.class);
private static final MethodFinder methodFinder = new MethodFinder();
private static final Predicate<Class<?>> testClassPredicate = new IsTestClassWithTests().or(
new IsNestedTestClass());
Expand All @@ -72,17 +72,17 @@ class MethodSelectorResolver implements SelectorResolver {

@Override
public Resolution resolve(MethodSelector selector, Context context) {
return resolve(context, emptyList(), selector.getJavaClass(), selector::getJavaMethod, Match::exact);
return resolve(selector, context, emptyList(), selector.getJavaClass(), selector::getJavaMethod, Match::exact);
}

@Override
public Resolution resolve(NestedMethodSelector selector, Context context) {
return resolve(context, selector.getEnclosingClasses(), selector.getNestedClass(), selector::getMethod,
Match::exact);
return resolve(selector, context, selector.getEnclosingClasses(), selector.getNestedClass(),
selector::getMethod, Match::exact);
}

private Resolution resolve(Context context, List<Class<?>> enclosingClasses, Class<?> testClass,
Supplier<Method> methodSupplier,
private Resolution resolve(DiscoverySelector selector, Context context, List<Class<?>> enclosingClasses,
Class<?> testClass, Supplier<Method> methodSupplier,
BiFunction<TestDescriptor, Supplier<Set<? extends DiscoverySelector>>, Match> matchFactory) {
if (!testClassPredicate.test(testClass)) {
return unresolved();
Expand All @@ -97,14 +97,14 @@ private Resolution resolve(Context context, List<Class<?>> enclosingClasses, Cla
.collect(toSet());
// @formatter:on
if (matches.size() > 1) {
logger.warn(() -> {
Stream<TestDescriptor> testDescriptors = matches.stream().map(Match::getTestDescriptor);
return String.format(
"Possible configuration error: method [%s] resulted in multiple TestDescriptors %s. "
+ "This is typically the result of annotating a method with multiple competing annotations "
+ "such as @Test, @RepeatedTest, @ParameterizedTest, @TestFactory, etc.",
method.toGenericString(), testDescriptors.map(d -> d.getClass().getName()).collect(toList()));
});
Stream<TestDescriptor> testDescriptors = matches.stream().map(Match::getTestDescriptor);
String message = String.format(
"Possible configuration error: method [%s] resulted in multiple TestDescriptors %s. " //
+ "This is typically the result of annotating a method with multiple competing annotations " //
+ "such as @Test, @RepeatedTest, @ParameterizedTest, @TestFactory, etc.",
method.toGenericString(), testDescriptors.map(d -> d.getClass().getName()).collect(toList()));
context.reportIssue(DiscoveryIssue.builder(Severity.NOTICE, message) //
.source(MethodSource.from(testClass, method)));
}
return matches.isEmpty() ? unresolved() : matches(matches);
}
Expand Down Expand Up @@ -139,7 +139,7 @@ public Resolution resolve(UniqueIdSelector selector, Context context) {
public Resolution resolve(IterationSelector selector, Context context) {
if (selector.getParentSelector() instanceof MethodSelector) {
MethodSelector methodSelector = (MethodSelector) selector.getParentSelector();
return resolve(context, emptyList(), methodSelector.getJavaClass(), methodSelector::getJavaMethod,
return resolve(selector, context, emptyList(), methodSelector.getJavaClass(), methodSelector::getJavaMethod,
(testDescriptor, childSelectorsSupplier) -> {
if (testDescriptor instanceof Filterable) {
Filterable filterable = (Filterable) testDescriptor;
Expand Down
Loading
Loading