From 359761e38cc043e18986baf58874349ad672184a Mon Sep 17 00:00:00 2001 From: BassemElMasry Date: Fri, 21 Apr 2023 13:55:28 +0200 Subject: [PATCH 1/3] Improving error message for non-static @MethodSource factory methods issue: https://github.com/junit-team/junit5/issues/3215 --- .../params/provider/MethodArgumentsProvider.java | 15 +++++++++++++++ .../provider/MethodArgumentsProviderTests.java | 13 ++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java b/junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java index b92e19d46b6d..d16e4decdbfc 100644 --- a/junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java +++ b/junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; +import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.platform.commons.JUnitException; @@ -46,12 +47,26 @@ protected Stream provideArguments(ExtensionContext context, // @formatter:off return stream(methodNames) .map(factoryMethodName -> getFactoryMethod(testClass, testMethod, factoryMethodName)) + .map(factoryMethod -> validateStaticFactoryMethod(context, factoryMethod)) .map(factoryMethod -> context.getExecutableInvoker().invoke(factoryMethod, testInstance)) .flatMap(CollectionUtils::toStream) .map(MethodArgumentsProvider::toArguments); // @formatter:on } + private Method validateStaticFactoryMethod(ExtensionContext context, Method factoryMethod) { + if (isPerMethodLifecycle(context)) { + Preconditions.condition(ReflectionUtils.isStatic(factoryMethod), + factoryMethod + " method must not be static"); + } + return factoryMethod; + } + + private boolean isPerMethodLifecycle(ExtensionContext context) { + return context.getTestInstanceLifecycle().orElse( + TestInstance.Lifecycle.PER_CLASS) == TestInstance.Lifecycle.PER_METHOD; + } + private Method getFactoryMethod(Class testClass, Method testMethod, String factoryMethodName) { if (!StringUtils.isBlank(factoryMethodName)) { if (looksLikeAFullyQualifiedMethodName(factoryMethodName)) { diff --git a/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java b/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java index 281670a4435f..11197ec7db75 100644 --- a/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java +++ b/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java @@ -33,6 +33,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.jupiter.api.extension.ParameterContext; import org.junit.jupiter.api.extension.ParameterResolver; @@ -198,7 +199,7 @@ void throwsExceptionWhenNonStaticFactoryMethodIsReferencedAndStaticIsRequired() var exception = assertThrows(JUnitException.class, () -> provideArguments(NonStaticTestCase.class, null, false, "nonStaticStringStreamProvider").toArray()); - assertThat(exception).hasMessageContaining("Cannot invoke non-static method"); + assertThat(exception).hasMessageContaining("method must not be static"); } @Test @@ -609,6 +610,9 @@ private Stream provideArguments(Class testClass, Method testMethod, var testInstance = allowNonStaticMethod ? ReflectionUtils.newInstance(testClass) : null; when(extensionContext.getTestInstance()).thenReturn(Optional.ofNullable(testInstance)); + var lifeCycle = allowNonStaticMethod ? TestInstance.Lifecycle.PER_CLASS : TestInstance.Lifecycle.PER_METHOD; + when(extensionContext.getTestInstanceLifecycle()).thenReturn(Optional.of(lifeCycle)); + var provider = new MethodArgumentsProvider(); provider.accept(methodSource); return provider.provideArguments(extensionContext).map(Arguments::get); @@ -796,6 +800,13 @@ Stream nonStaticStringStreamProvider() { } } + @TestInstance(TestInstance.Lifecycle.PER_METHOD) + static class NonStaticTestCaseForPerMethodLifecycle { + Stream nonStaticStringStreamProvider() { + return Stream.of("foo", "bar"); + } + } + static class ExternalFactoryMethods { static Stream stringsProvider() { From 27c34b17a7b42841afa06388c652909d8091fc67 Mon Sep 17 00:00:00 2001 From: BassemElMasry Date: Fri, 21 Apr 2023 14:38:39 +0200 Subject: [PATCH 2/3] removing unneccessary test method --- .../params/provider/MethodArgumentsProviderTests.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java b/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java index 11197ec7db75..83b1a47394ab 100644 --- a/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java +++ b/junit-jupiter-params/src/test/java/org/junit/jupiter/params/provider/MethodArgumentsProviderTests.java @@ -800,13 +800,6 @@ Stream nonStaticStringStreamProvider() { } } - @TestInstance(TestInstance.Lifecycle.PER_METHOD) - static class NonStaticTestCaseForPerMethodLifecycle { - Stream nonStaticStringStreamProvider() { - return Stream.of("foo", "bar"); - } - } - static class ExternalFactoryMethods { static Stream stringsProvider() { From 3f5100ad97a09d83084c1ec69f3491c6a081c98e Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Wed, 26 Apr 2023 13:15:36 +0200 Subject: [PATCH 3/3] Add to release notes --- .../src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc | 1 + 1 file changed, 1 insertion(+) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc index 56c41b3286f7..e24673c75e2a 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.0-M1.adoc @@ -82,6 +82,7 @@ default. parameter to set the maximum pool size factor. * New `junit.jupiter.execution.parallel.config.dynamic.saturate` configuration parameter to disable pool saturation. +* Improve error message for non-static `@MethodSource` factory methods [[release-notes-5.10.0-M1-junit-vintage]]