diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java index 2e97cc2d385f..7186b34f3226 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassBasedTestDescriptor.java @@ -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; /** @@ -84,21 +86,21 @@ */ @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, @@ -106,6 +108,7 @@ public abstract class ClassBasedTestDescriptor extends JupiterTestDescriptor super(uniqueId, displayName, ClassSource.from(testClass), configuration); this.classInfo = new ClassInfo(testClass, configuration); + this.lifecycleMethods = new LifecycleMethods(this.classInfo); } // --- TestClassAware ------------------------------------------------------ @@ -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 @@ -166,10 +178,7 @@ 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, @@ -177,7 +186,7 @@ public final JupiterEngineExecutionContext prepare(JupiterEngineExecutionContext // 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(); @@ -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); @@ -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, @@ -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)); @@ -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, @@ -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; @@ -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); + } + } + } diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java index 83f60a06ef9a..a73d9fc2f491 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java @@ -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; @@ -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. @@ -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(); } } diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java new file mode 100644 index 000000000000..b4c83e1944ad --- /dev/null +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/Validatable.java @@ -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); + +} diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java index 89a79c679dec..1198d6da5a6d 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolver.java @@ -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; @@ -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(); diff --git a/junit-jupiter-engine/src/nativeImage/initialize-at-build-time b/junit-jupiter-engine/src/nativeImage/initialize-at-build-time index df2adf10a9ba..4e405ef9656f 100644 --- a/junit-jupiter-engine/src/nativeImage/initialize-at-build-time +++ b/junit-jupiter-engine/src/nativeImage/initialize-at-build-time @@ -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 diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DefaultDiscoveryIssueReporter.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DefaultDiscoveryIssueReporter.java deleted file mode 100644 index 1aa686b29a4f..000000000000 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DefaultDiscoveryIssueReporter.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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.platform.engine.support.discovery; - -import org.junit.platform.engine.DiscoveryIssue; -import org.junit.platform.engine.EngineDiscoveryListener; -import org.junit.platform.engine.UniqueId; - -/** - * @since 1.13 - */ -class DefaultDiscoveryIssueReporter implements DiscoveryIssueReporter { - - private final EngineDiscoveryListener discoveryListener; - private final UniqueId engineId; - - DefaultDiscoveryIssueReporter(EngineDiscoveryListener discoveryListener, UniqueId engineId) { - this.discoveryListener = discoveryListener; - this.engineId = engineId; - } - - @Override - public void reportIssue(DiscoveryIssue issue) { - this.discoveryListener.issueEncountered(this.engineId, issue); - } - -} diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DiscoveryIssueReporter.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DiscoveryIssueReporter.java index 39a931891c09..1f17d4128b5c 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DiscoveryIssueReporter.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/DiscoveryIssueReporter.java @@ -12,8 +12,15 @@ import static org.apiguardian.api.API.Status.EXPERIMENTAL; +import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.Predicate; + import org.apiguardian.api.API; +import org.junit.platform.commons.util.Preconditions; import org.junit.platform.engine.DiscoveryIssue; +import org.junit.platform.engine.EngineDiscoveryListener; +import org.junit.platform.engine.UniqueId; /** * {@code DiscoveryIssueReporter} defines the API for reporting @@ -23,8 +30,23 @@ * @see SelectorResolver.Context */ @API(status = EXPERIMENTAL, since = "1.13") +@FunctionalInterface public interface DiscoveryIssueReporter { + /** + * Create a new {@code DiscoveryIssueReporter} that reports issues to the + * supplied {@link EngineDiscoveryListener} for the specified engine. + * + * @param engineDiscoveryListener the listener to report issues to; never + * {@code null} + * @param engineId the unique identifier of the engine; never {@code null} + */ + static DiscoveryIssueReporter create(EngineDiscoveryListener engineDiscoveryListener, UniqueId engineId) { + Preconditions.notNull(engineDiscoveryListener, "engineDiscoveryListener must not be null"); + Preconditions.notNull(engineId, "engineId must not be null"); + return issue -> engineDiscoveryListener.issueEncountered(engineId, issue); + } + /** * Build the supplied {@link DiscoveryIssue.Builder Builder} and report the * resulting {@link DiscoveryIssue}. @@ -38,4 +60,77 @@ default void reportIssue(DiscoveryIssue.Builder builder) { */ void reportIssue(DiscoveryIssue issue); + /** + * Create a {@link Condition} that reports a {@link DiscoveryIssue} when the + * supplied {@link Predicate} is not met. + * + * @param predicate the predicate to test; never {@code null} + * @param issueCreator the function to create the issue with; never {@code null} + * @return a new {@code Condition}; never {@code null} + */ + default <T> Condition<T> createReportingCondition(Predicate<T> predicate, + Function<T, DiscoveryIssue> issueCreator) { + Preconditions.notNull(predicate, "predicate must not be null"); + Preconditions.notNull(issueCreator, "issueCreator must not be null"); + return value -> { + if (predicate.test(value)) { + return true; + } + else { + reportIssue(issueCreator.apply(value)); + return false; + } + }; + } + + /** + * A {@code Condition} is a union of {@link Predicate} and {@link Consumer}. + * + * <p>Instances of this type may be used as {@link Predicate Predicates} or + * {@link Consumer Consumers}. For example, a {@code Condition} may be + * passed to {@link java.util.stream.Stream#filter(Predicate)} if it is used + * for filtering, or to {@link java.util.stream.Stream#peek(Consumer)} if it + * is only used for reporting or other side effects. + * + * @see #createReportingCondition(Predicate, Function) + */ + @FunctionalInterface + interface Condition<T> extends Predicate<T>, Consumer<T> { + + /** + * Return a composed condition that represents a logical AND of the + * supplied conditions without short-circuiting. + * + * <p><em>All</em> of the supplied conditions will be evaluated even if + * one or more of them return {@code false} to ensure that all issues + * are reported. + * + * @param conditions the conditions to compose; never {@code null}, not + * empty, and must not contain any {@code null} elements + * @return the composed condition; never {@code null} + */ + @SafeVarargs + @SuppressWarnings("varargs") + static <T> Condition<T> allOf(Condition<? super T>... conditions) { + Preconditions.notNull(conditions, "conditions must not be null"); + Preconditions.notEmpty(conditions, "conditions must not be empty"); + Preconditions.containsNoNullElements(conditions, "conditions must not contain null elements"); + return value -> { + boolean result = true; + for (Condition<? super T> condition : conditions) { + result &= condition.test(value); + } + return result; + }; + } + + /** + * Evaluate the {@code #test(Object)} method of this condition to + * potentially report an issue. + */ + @Override + default void accept(T value) { + test(value); + } + } } diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/EngineDiscoveryRequestResolver.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/EngineDiscoveryRequestResolver.java index 431975615370..28e24d09a705 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/EngineDiscoveryRequestResolver.java +++ b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/discovery/EngineDiscoveryRequestResolver.java @@ -110,7 +110,7 @@ private EngineDiscoveryRequestResolver(List<Function<InitializationContext<T>, S public void resolve(EngineDiscoveryRequest request, T engineDescriptor) { Preconditions.notNull(request, "request must not be null"); Preconditions.notNull(engineDescriptor, "engineDescriptor must not be null"); - DiscoveryIssueReporter issueReporter = new DefaultDiscoveryIssueReporter(request.getDiscoveryListener(), + DiscoveryIssueReporter issueReporter = DiscoveryIssueReporter.create(request.getDiscoveryListener(), engineDescriptor.getUniqueId()); InitializationContext<T> initializationContext = new DefaultInitializationContext<>(request, engineDescriptor, issueReporter); diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/InvalidLifecycleMethodConfigurationTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/InvalidLifecycleMethodConfigurationTests.java index e1d7a47cfce6..a17b1fb8d4a6 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/InvalidLifecycleMethodConfigurationTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/InvalidLifecycleMethodConfigurationTests.java @@ -10,82 +10,64 @@ package org.junit.jupiter.engine; -import static org.junit.jupiter.api.Assertions.assertAll; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; +import static java.util.function.Predicate.isEqual; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.platform.commons.util.FunctionUtils.where; + +import java.lang.annotation.Annotation; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.platform.testkit.engine.EngineExecutionResults; -import org.junit.platform.testkit.engine.Events; +import org.junit.platform.engine.DiscoveryIssue; +import org.junit.platform.engine.DiscoveryIssue.Severity; /** * Integration tests that verify proper handling of invalid configuration for * lifecycle methods in conjunction with the {@link JupiterTestEngine}. * - * <p>In general, configuration errors should not be thrown until the - * execution phase, thereby giving all containers a chance to execute. - * * @since 5.0 */ class InvalidLifecycleMethodConfigurationTests extends AbstractJupiterTestEngineTests { @Test void executeValidTestCaseAlongsideTestCaseWithInvalidNonStaticBeforeAllDeclaration() { - assertContainerFailed(TestCaseWithInvalidNonStaticBeforeAllMethod.class); + assertReportsError(TestCaseWithInvalidNonStaticBeforeAllMethod.class, BeforeAll.class); } @Test void executeValidTestCaseAlongsideTestCaseWithInvalidNonStaticAfterAllDeclaration() { - assertContainerFailed(TestCaseWithInvalidNonStaticAfterAllMethod.class); + assertReportsError(TestCaseWithInvalidNonStaticAfterAllMethod.class, AfterAll.class); } @Test void executeValidTestCaseAlongsideTestCaseWithInvalidStaticBeforeEachDeclaration() { - assertContainerFailed(TestCaseWithInvalidStaticBeforeEachMethod.class); + assertReportsError(TestCaseWithInvalidStaticBeforeEachMethod.class, BeforeEach.class); } @Test void executeValidTestCaseAlongsideTestCaseWithInvalidStaticAfterEachDeclaration() { - assertContainerFailed(TestCaseWithInvalidStaticAfterEachMethod.class); + assertReportsError(TestCaseWithInvalidStaticAfterEachMethod.class, AfterEach.class); } - private void assertContainerFailed(Class<?> invalidTestClass) { - EngineExecutionResults executionResults = executeTests(selectClass(TestCase.class), - selectClass(invalidTestClass)); - Events containers = executionResults.containerEvents(); - Events tests = executionResults.testEvents(); - - // @formatter:off - assertAll( - () -> assertEquals(3, containers.started().count(), "# containers started"), - () -> assertEquals(1, tests.started().count(), "# tests started"), - () -> assertEquals(1, tests.succeeded().count(), "# tests succeeded"), - () -> assertEquals(0, tests.failed().count(), "# tests failed"), - () -> assertEquals(3, containers.finished().count(), "# containers finished"), - () -> assertEquals(1, containers.failed().count(), "# containers failed") - ); - // @formatter:on + private void assertReportsError(Class<?> invalidTestClass, Class<? extends Annotation> annotationType) { + var results = discoverTestsForClass(invalidTestClass); + + assertThat(results.getDiscoveryIssues()) // + .filteredOn(where(DiscoveryIssue::severity, isEqual(Severity.ERROR))) // + .extracting(DiscoveryIssue::message) // + .asString().contains("@%s method".formatted(annotationType.getSimpleName())); } // ------------------------------------------------------------------------- - @SuppressWarnings("JUnitMalformedDeclaration") - static class TestCase { - - @Test - void test() { - } - } - @SuppressWarnings("JUnitMalformedDeclaration") static class TestCaseWithInvalidNonStaticBeforeAllMethod { // must be static - @SuppressWarnings("JUnitMalformedDeclaration") + @SuppressWarnings("unused") @BeforeAll void beforeAll() { } @@ -99,7 +81,7 @@ void test() { static class TestCaseWithInvalidNonStaticAfterAllMethod { // must be static - @SuppressWarnings("JUnitMalformedDeclaration") + @SuppressWarnings("unused") @AfterAll void afterAll() { } @@ -113,7 +95,7 @@ void test() { static class TestCaseWithInvalidStaticBeforeEachMethod { // must NOT be static - @SuppressWarnings("JUnitMalformedDeclaration") + @SuppressWarnings("unused") @BeforeEach static void beforeEach() { } @@ -127,7 +109,7 @@ void test() { static class TestCaseWithInvalidStaticAfterEachMethod { // must NOT be static - @SuppressWarnings("JUnitMalformedDeclaration") + @SuppressWarnings("unused") @AfterEach static void afterEach() { } diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/TestInstanceLifecycleConfigurationTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/TestInstanceLifecycleConfigurationTests.java index fdb7e512ce75..8090690f32f6 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/TestInstanceLifecycleConfigurationTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/TestInstanceLifecycleConfigurationTests.java @@ -73,7 +73,7 @@ void instancePerClassConfiguredViaSystemProperty() { Class<?> testClass = AssumedInstancePerClassTestCase.class; // Should fail by default... - performAssertions(testClass, 2, 1, 0); + performAssertions(testClass, 1, 1, 0); // Should pass with the system property set System.setProperty(KEY, PER_CLASS.name()); @@ -85,7 +85,7 @@ void instancePerClassConfiguredViaConfigParam() { Class<?> testClass = AssumedInstancePerClassTestCase.class; // Should fail by default... - performAssertions(testClass, 2, 1, 0); + performAssertions(testClass, 1, 1, 0); // Should pass with the config param performAssertions(testClass, singletonMap(KEY, PER_CLASS.name()), 2, 0, 1, "beforeAll", "test", "afterAll"); @@ -97,7 +97,7 @@ void instancePerClassConfiguredViaConfigParamThatOverridesSystemProperty() { // Should fail with system property System.setProperty(KEY, PER_METHOD.name()); - performAssertions(testClass, 2, 1, 0); + performAssertions(testClass, 1, 1, 0); // Should pass with the config param performAssertions(testClass, singletonMap(KEY, PER_CLASS.name()), 2, 0, 1, "beforeAll", "test", "afterAll"); diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtilsTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtilsTests.java index 719f335679bf..2da2aa759d2e 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtilsTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtilsTests.java @@ -10,15 +10,16 @@ package org.junit.jupiter.engine.descriptor; +import static java.util.function.Predicate.isEqual; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.engine.descriptor.LifecycleMethodUtils.findAfterAllMethods; import static org.junit.jupiter.engine.descriptor.LifecycleMethodUtils.findAfterEachMethods; import static org.junit.jupiter.engine.descriptor.LifecycleMethodUtils.findBeforeAllMethods; import static org.junit.jupiter.engine.descriptor.LifecycleMethodUtils.findBeforeEachMethods; +import static org.junit.platform.commons.util.FunctionUtils.where; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.List; import org.junit.jupiter.api.AfterAll; @@ -28,7 +29,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.TestInstance.Lifecycle; -import org.junit.platform.commons.JUnitException; +import org.junit.platform.engine.DiscoveryIssue; +import org.junit.platform.engine.DiscoveryIssue.Severity; +import org.junit.platform.engine.support.descriptor.MethodSource; /** * Unit tests for {@link LifecycleMethodUtils}. @@ -37,94 +40,148 @@ */ class LifecycleMethodUtilsTests { + List<DiscoveryIssue> discoveryIssues = new ArrayList<>(); + @Test - void findNonVoidBeforeAllMethodsWithStandardLifecycle() { - JUnitException exception = assertThrows(JUnitException.class, - () -> findBeforeAllMethods(TestCaseWithNonVoidLifecyleMethods.class, true)); - assertEquals( - "@BeforeAll method 'java.lang.Double org.junit.jupiter.engine.descriptor.TestCaseWithNonVoidLifecyleMethods.cc()' must not return a value.", - exception.getMessage()); + void findNonVoidBeforeAllMethodsWithStandardLifecycle() throws Exception { + var methods = findBeforeAllMethods(TestCaseWithInvalidLifecycleMethods.class, true, discoveryIssues::add); + assertThat(methods).isEmpty(); + + var methodSource = MethodSource.from(TestCaseWithInvalidLifecycleMethods.class.getDeclaredMethod("cc")); + var notVoidIssue = DiscoveryIssue.builder(Severity.ERROR, + "@BeforeAll method 'private java.lang.Double org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.cc()' must not return a value.") // + .source(methodSource) // + .build(); + var notStaticIssue = DiscoveryIssue.builder(Severity.ERROR, + "@BeforeAll method 'private java.lang.Double org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.cc()' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).") // + .source(methodSource) // + .build(); + var privateIssue = DiscoveryIssue.builder(Severity.DEPRECATION, + "@BeforeAll method 'private java.lang.Double org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.cc()' should not be private. This will be disallowed in a future release.") // + .source(methodSource) // + .build(); + assertThat(discoveryIssues).containsExactlyInAnyOrder(notVoidIssue, notStaticIssue, privateIssue); } @Test - void findNonVoidAfterAllMethodsWithStandardLifecycle() { - JUnitException exception = assertThrows(JUnitException.class, - () -> findAfterAllMethods(TestCaseWithNonVoidLifecyleMethods.class, true)); - assertEquals( - "@AfterAll method 'java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithNonVoidLifecyleMethods.dd()' must not return a value.", - exception.getMessage()); + void findNonVoidAfterAllMethodsWithStandardLifecycle() throws Exception { + var methods = findAfterAllMethods(TestCaseWithInvalidLifecycleMethods.class, true, discoveryIssues::add); + assertThat(methods).isEmpty(); + + var methodSource = MethodSource.from(TestCaseWithInvalidLifecycleMethods.class.getDeclaredMethod("dd")); + var notVoidIssue = DiscoveryIssue.builder(Severity.ERROR, + "@AfterAll method 'private java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.dd()' must not return a value.") // + .source(methodSource) // + .build(); + var notStaticIssue = DiscoveryIssue.builder(Severity.ERROR, + "@AfterAll method 'private java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.dd()' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).") // + .source(methodSource) // + .build(); + var privateIssue = DiscoveryIssue.builder(Severity.DEPRECATION, + "@AfterAll method 'private java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.dd()' should not be private. This will be disallowed in a future release.") // + .source(methodSource) // + .build(); + assertThat(discoveryIssues).containsExactlyInAnyOrder(notVoidIssue, notStaticIssue, privateIssue); } @Test - void findNonVoidBeforeEachMethodsWithStandardLifecycle() { - JUnitException exception = assertThrows(JUnitException.class, - () -> findBeforeEachMethods(TestCaseWithNonVoidLifecyleMethods.class)); - assertEquals( - "@BeforeEach method 'java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithNonVoidLifecyleMethods.aa()' must not return a value.", - exception.getMessage()); + void findNonVoidBeforeEachMethodsWithStandardLifecycle() throws Exception { + var methods = findBeforeEachMethods(TestCaseWithInvalidLifecycleMethods.class, discoveryIssues::add); + assertThat(methods).isEmpty(); + + var methodSource = MethodSource.from(TestCaseWithInvalidLifecycleMethods.class.getDeclaredMethod("aa")); + var notVoidIssue = DiscoveryIssue.builder(Severity.ERROR, + "@BeforeEach method 'private java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.aa()' must not return a value.") // + .source(methodSource) // + .build(); + var privateIssue = DiscoveryIssue.builder(Severity.DEPRECATION, + "@BeforeEach method 'private java.lang.String org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.aa()' should not be private. This will be disallowed in a future release.") // + .source(methodSource) // + .build(); + assertThat(discoveryIssues).containsExactlyInAnyOrder(notVoidIssue, privateIssue); } @Test - void findNonVoidAfterEachMethodsWithStandardLifecycle() { - JUnitException exception = assertThrows(JUnitException.class, - () -> findAfterEachMethods(TestCaseWithNonVoidLifecyleMethods.class)); - assertEquals( - "@AfterEach method 'int org.junit.jupiter.engine.descriptor.TestCaseWithNonVoidLifecyleMethods.bb()' must not return a value.", - exception.getMessage()); + void findNonVoidAfterEachMethodsWithStandardLifecycle() throws Exception { + var methods = findAfterEachMethods(TestCaseWithInvalidLifecycleMethods.class, discoveryIssues::add); + assertThat(methods).isEmpty(); + + var methodSource = MethodSource.from(TestCaseWithInvalidLifecycleMethods.class.getDeclaredMethod("bb")); + var notVoidIssue = DiscoveryIssue.builder(Severity.ERROR, + "@AfterEach method 'private int org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.bb()' must not return a value.") // + .source(methodSource) // + .build(); + var privateIssue = DiscoveryIssue.builder(Severity.DEPRECATION, + "@AfterEach method 'private int org.junit.jupiter.engine.descriptor.TestCaseWithInvalidLifecycleMethods.bb()' should not be private. This will be disallowed in a future release.") // + .source(methodSource) // + .build(); + assertThat(discoveryIssues).containsExactlyInAnyOrder(notVoidIssue, privateIssue); } @Test void findBeforeEachMethodsWithStandardLifecycle() { - List<Method> methods = findBeforeEachMethods(TestCaseWithStandardLifecycle.class); + List<Method> methods = findBeforeEachMethods(TestCaseWithStandardLifecycle.class, discoveryIssues::add); assertThat(namesOf(methods)).containsExactlyInAnyOrder("nine", "ten"); + assertThat(discoveryIssues).isEmpty(); } @Test void findAfterEachMethodsWithStandardLifecycle() { - List<Method> methods = findAfterEachMethods(TestCaseWithStandardLifecycle.class); + List<Method> methods = findAfterEachMethods(TestCaseWithStandardLifecycle.class, discoveryIssues::add); assertThat(namesOf(methods)).containsExactlyInAnyOrder("eleven", "twelve"); } @Test void findBeforeAllMethodsWithStandardLifecycleAndWithoutRequiringStatic() { - List<Method> methods = findBeforeAllMethods(TestCaseWithStandardLifecycle.class, false); + List<Method> methods = findBeforeAllMethods(TestCaseWithStandardLifecycle.class, false, discoveryIssues::add); assertThat(namesOf(methods)).containsExactly("one"); + assertThat(discoveryIssues).isEmpty(); } @Test - void findBeforeAllMethodsWithStandardLifecycleAndRequiringStatic() { - JUnitException exception = assertThrows(JUnitException.class, - () -> findBeforeAllMethods(TestCaseWithStandardLifecycle.class, true)); - assertEquals( - "@BeforeAll method 'void org.junit.jupiter.engine.descriptor.TestCaseWithStandardLifecycle.one()' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).", - exception.getMessage()); + void findBeforeAllMethodsWithStandardLifecycleAndRequiringStatic() throws Exception { + var methods = findBeforeAllMethods(TestCaseWithStandardLifecycle.class, true, discoveryIssues::add); + assertThat(methods).isEmpty(); + + var expectedIssue = DiscoveryIssue.builder(Severity.ERROR, + "@BeforeAll method 'void org.junit.jupiter.engine.descriptor.TestCaseWithStandardLifecycle.one()' must be static unless the test class is annotated with @TestInstance(Lifecycle.PER_CLASS).") // + .source(MethodSource.from(TestCaseWithStandardLifecycle.class.getDeclaredMethod("one"))) // + .build(); + assertThat(discoveryIssues).containsExactly(expectedIssue); } @Test void findBeforeAllMethodsWithLifeCyclePerClassAndRequiringStatic() { - List<Method> methods = findBeforeAllMethods(TestCaseWithLifecyclePerClass.class, false); + List<Method> methods = findBeforeAllMethods(TestCaseWithLifecyclePerClass.class, false, discoveryIssues::add); assertThat(namesOf(methods)).containsExactlyInAnyOrder("three", "four"); + assertThat(discoveryIssues).isEmpty(); } @Test void findAfterAllMethodsWithStandardLifecycleAndWithoutRequiringStatic() { - List<Method> methods = findAfterAllMethods(TestCaseWithStandardLifecycle.class, false); + List<Method> methods = findAfterAllMethods(TestCaseWithStandardLifecycle.class, false, discoveryIssues::add); assertThat(namesOf(methods)).containsExactlyInAnyOrder("five", "six"); + assertThat(discoveryIssues).isEmpty(); } @Test void findAfterAllMethodsWithStandardLifecycleAndRequiringStatic() { - assertThrows(JUnitException.class, () -> findAfterAllMethods(TestCaseWithStandardLifecycle.class, true)); + var methods = findAfterAllMethods(TestCaseWithStandardLifecycle.class, true, discoveryIssues::add); + assertThat(methods).isEmpty(); + + assertThat(discoveryIssues) // + .filteredOn(where(DiscoveryIssue::severity, isEqual(Severity.ERROR))) // + .isNotEmpty(); } @Test void findAfterAllMethodsWithLifeCyclePerClassAndRequiringStatic() { - List<Method> methods = findAfterAllMethods(TestCaseWithLifecyclePerClass.class, false); + List<Method> methods = findAfterAllMethods(TestCaseWithLifecyclePerClass.class, false, discoveryIssues::add); assertThat(namesOf(methods)).containsExactlyInAnyOrder("seven", "eight"); } @@ -191,29 +248,26 @@ void eight() { } -class TestCaseWithNonVoidLifecyleMethods { +@SuppressWarnings("JUnitMalformedDeclaration") +class TestCaseWithInvalidLifecycleMethods { - @SuppressWarnings("JUnitMalformedDeclaration") @BeforeEach - String aa() { + private String aa() { return null; } - @SuppressWarnings("JUnitMalformedDeclaration") @AfterEach - int bb() { + private int bb() { return 1; } - @SuppressWarnings("JUnitMalformedDeclaration") @BeforeAll - Double cc() { + private Double cc() { return null; } - @SuppressWarnings("JUnitMalformedDeclaration") @AfterAll - String dd() { + private String dd() { return ""; }