From d6b746682ffe40475af427f9e5e65b688e067a56 Mon Sep 17 00:00:00 2001 From: Marc Philipp Date: Tue, 8 Oct 2024 13:45:01 +0200 Subject: [PATCH] Add constructor injection support for `TempDir` This was made possible by lifting the `ParameterResolver` limitation for accessing the test-scoped `ExtensionContext` for test class constructors in #3445. --- .../release-notes-5.12.0-M1.adoc | 1 + .../asciidoc/user-guide/writing-tests.adoc | 11 +- .../engine/extension/TempDirectory.java | 11 +- .../TempDirectoryPerContextTests.java | 109 +++++++++++++----- .../TempDirectoryPerDeclarationTests.java | 91 +++++++-------- 5 files changed, 127 insertions(+), 96 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc index 67393a8cd47b..08a328c14cd3 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.0-M1.adoc @@ -62,6 +62,7 @@ JUnit repository on GitHub. `ExtensionContext` while instantiating the test instance. The behavior enabled by the annotation is expected to eventually become the default in future versions of JUnit Jupiter. +* `@TempDir` is now supported on test class constructors. [[release-notes-5.12.0-M1-junit-vintage]] diff --git a/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc b/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc index d8e7c351c401..ccb0f5344588 100644 --- a/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc +++ b/documentation/src/docs/asciidoc/user-guide/writing-tests.adoc @@ -3003,7 +3003,8 @@ The built-in `{TempDirectory}` extension is used to create and clean up a tempor directory for an individual test or all tests in a test class. It is registered by default. To use it, annotate a non-final, unassigned field of type `java.nio.file.Path` or `java.io.File` with `{TempDir}` or add a parameter of type `java.nio.file.Path` or -`java.io.File` annotated with `@TempDir` to a lifecycle method or test method. +`java.io.File` annotated with `@TempDir` to a test class constructor, lifecycle method, or +test method. For example, the following test declares a parameter annotated with `@TempDir` for a single test method, creates and writes to a file in the temporary directory, and checks @@ -3028,14 +3029,10 @@ entire test class or method (depending on which level the annotation is used), y the `junit.jupiter.tempdir.scope` configuration parameter to `per_context`. However, please note that this option is deprecated and will be removed in a future release. -`@TempDir` is not supported on constructor parameters. If you wish to retain a single -reference to a temp directory across lifecycle methods and the current test method, please -use field injection by annotating an instance field with `@TempDir`. - The following example stores a _shared_ temporary directory in a `static` field. This allows the same `sharedTempDir` to be used in all lifecycle methods and test methods of -the test class. For better isolation, you should use an instance field so that each test -method uses a separate directory. +the test class. For better isolation, you should use an instance field or constructor +injection so that each test method uses a separate directory. [source,java,indent=0] .A test class that shares a temporary directory across test methods diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java index cf066e21a3ca..ff6484b960bb 100644 --- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java +++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java @@ -22,7 +22,6 @@ import java.io.File; import java.io.IOException; import java.lang.reflect.AnnotatedElement; -import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.Parameter; import java.nio.file.DirectoryNotEmptyException; @@ -45,12 +44,12 @@ import org.junit.jupiter.api.extension.AnnotatedElementContext; import org.junit.jupiter.api.extension.BeforeAllCallback; import org.junit.jupiter.api.extension.BeforeEachCallback; +import org.junit.jupiter.api.extension.EnableTestScopedConstructorContext; import org.junit.jupiter.api.extension.ExtensionConfigurationException; import org.junit.jupiter.api.extension.ExtensionContext; import org.junit.jupiter.api.extension.ExtensionContext.Namespace; import org.junit.jupiter.api.extension.ExtensionContext.Store.CloseableResource; import org.junit.jupiter.api.extension.ParameterContext; -import org.junit.jupiter.api.extension.ParameterResolutionException; import org.junit.jupiter.api.extension.ParameterResolver; import org.junit.jupiter.api.io.CleanupMode; import org.junit.jupiter.api.io.TempDir; @@ -78,6 +77,7 @@ * @see TempDir * @see Files#createTempDirectory */ +@EnableTestScopedConstructorContext class TempDirectory implements BeforeAllCallback, BeforeEachCallback, ParameterResolver { static final Namespace NAMESPACE = Namespace.create(TempDirectory.class); @@ -161,12 +161,7 @@ private void injectFields(ExtensionContext context, Object testInstance, Class stats.started(2).failed(0).succeeded(2)); + assertThat(BaseConstructorInjectionTestCase.tempDirs.getFirst()).doesNotExist(); + assertThat(BaseConstructorInjectionTestCase.tempDirs.getLast()).doesNotExist(); + } + private void assertSharedTempDirForFieldInjection( Class testClass) { @@ -296,6 +308,17 @@ void resolvesSeparateTempDirWhenAnnotationIsUsedOnAfterAllMethodParameterOnly() assertThat(AnnotationOnAfterAllMethodParameterTestCase.secondTempDir).isNotNull().doesNotExist(); } + @Test + @DisplayName("when @TempDir is used on constructor parameter") + @Order(32) + void resolvesSeparateTempDirsWhenAnnotationIsUsedOnConstructorWithTestInstancePerMethod() { + var results = executeTestsForClass(SeparateTempDirsConstructorInjectionPerMethodTestCase.class); + + results.testEvents().assertStatistics(stats -> stats.started(2).failed(0).succeeded(2)); + assertThat(BaseConstructorInjectionTestCase.tempDirs.getFirst()).doesNotExist(); + assertThat(BaseConstructorInjectionTestCase.tempDirs.getLast()).doesNotExist(); + } + } @Nested @@ -370,26 +393,6 @@ void onlySupportsParametersOfTypePathAndFile() { // @formatter:on } - @Test - @DisplayName("when @TempDir is used on constructor parameter") - @Order(30) - void doesNotSupportTempDirAnnotationOnConstructorParameter() { - var results = executeTestsForClass(AnnotationOnConstructorParameterTestCase.class); - - assertSingleFailedTest(results, ParameterResolutionException.class, - "@TempDir is not supported on constructor parameters. Please use field injection instead."); - } - - @Test - @DisplayName("when @TempDir is used on constructor parameter with @TestInstance(PER_CLASS)") - @Order(31) - void doesNotSupportTempDirAnnotationOnConstructorParameterWithTestInstancePerClass() { - var results = executeTestsForClass(AnnotationOnConstructorParameterWithTestInstancePerClassTestCase.class); - - assertSingleFailedContainer(results, ParameterResolutionException.class, - "@TempDir is not supported on constructor parameters. Please use field injection instead."); - } - @Test @DisplayName("when non-default @TempDir factory is set") @Order(32) @@ -693,26 +696,75 @@ static void check(Path tempDir) { } - static class AnnotationOnConstructorParameterTestCase { + static class BaseConstructorInjectionTestCase { - AnnotationOnConstructorParameterTestCase(@SuppressWarnings("unused") @TempDir Path tempDir) { - // never called + static final Deque tempDirs = new LinkedList<>(); + + private final Path tempDir; + + BaseConstructorInjectionTestCase(Path tempDir) { + this.tempDir = tempDir; } @Test - void test() { - // never called + void test1(@TempDir Path tempDir, TestInfo testInfo) throws Exception { + check(tempDir); + writeFile(tempDir, testInfo); + } + + @Test + void test2(TestInfo testInfo, @TempDir Path tempDir) throws Exception { + check(tempDir); + writeFile(tempDir, testInfo); + } + + @AfterEach + void afterEach(@TempDir Path tempDir) { + check(tempDir); + } + + void check(Path tempDir) { + assertThat(tempDirs.getLast())// + .isNotNull()// + .isSameAs(tempDir)// + .isSameAs(this.tempDir); + assertTrue(Files.exists(tempDir)); + } + + } + + static class SeparateTempDirsConstructorInjectionPerMethodTestCase extends BaseConstructorInjectionTestCase { + + SeparateTempDirsConstructorInjectionPerMethodTestCase(@TempDir Path tempDir) { + super(tempDir); } + @BeforeEach + void beforeEach(@TempDir Path tempDir) { + for (Path dir : tempDirs) { + assertThat(dir).doesNotExist(); + } + assertThat(tempDirs).doesNotContain(tempDir); + tempDirs.add(tempDir); + check(tempDir); + } } @TestInstance(PER_CLASS) - static class AnnotationOnConstructorParameterWithTestInstancePerClassTestCase - extends AnnotationOnConstructorParameterTestCase { + static class SharedTempDirsConstructorInjectionPerClassTestCase extends BaseConstructorInjectionTestCase { - AnnotationOnConstructorParameterWithTestInstancePerClassTestCase(@TempDir Path tempDir) { + SharedTempDirsConstructorInjectionPerClassTestCase(@TempDir Path tempDir) { super(tempDir); } + + @BeforeEach + void beforeEach(@TempDir Path tempDir) { + for (Path dir : tempDirs) { + assertThat(dir).isSameAs(tempDir).exists(); + } + tempDirs.add(tempDir); + check(tempDir); + } } static class NonDefaultFactoryTestCase { @@ -803,7 +855,6 @@ static void afterAll(@TempDir Path tempDir) { } static class BaseSeparateTempDirsFieldInjectionTestCase { - static final Deque tempDirs = new LinkedList<>(); @TempDir diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryPerDeclarationTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryPerDeclarationTests.java index cfd8fbc452af..33ba07a362d4 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryPerDeclarationTests.java +++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryPerDeclarationTests.java @@ -15,6 +15,7 @@ import static java.lang.annotation.ElementType.PARAMETER; import static java.lang.annotation.RetentionPolicy.RUNTIME; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNotSame; @@ -22,7 +23,6 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; -import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS; import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass; import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request; import static org.junit.platform.testkit.engine.EventConditions.finishedWithFailure; @@ -123,25 +123,37 @@ void resolvesSeparateTempDirsForEachAnnotationDeclaration(TestInstance.Lifecycle assertThat(AllPossibleDeclarationLocationsTestCase.tempDirs).hasSize(3); var classTempDirs = AllPossibleDeclarationLocationsTestCase.tempDirs.get("class"); - assertThat(classTempDirs).containsOnlyKeys("staticField1", "staticField2", "beforeAll1", "beforeAll2", - "afterAll1", "afterAll2"); - assertThat(classTempDirs.values()).hasSize(6).doesNotHaveDuplicates(); + assertThat(classTempDirs) // + .containsOnlyKeys("staticField1", "staticField2", "beforeAll1", "beforeAll2", "afterAll1", "afterAll2") // + .values().hasSize(6).doesNotHaveDuplicates().allMatch(Files::notExists); var testATempDirs = AllPossibleDeclarationLocationsTestCase.tempDirs.get("testA"); - assertThat(testATempDirs).containsOnlyKeys("staticField1", "staticField2", "instanceField1", "instanceField2", - "beforeEach1", "beforeEach2", "test1", "test2", "afterEach1", "afterEach2"); - assertThat(testATempDirs.values()).hasSize(10).doesNotHaveDuplicates(); + assertThat(testATempDirs) // + .containsOnlyKeys("staticField1", "staticField2", "instanceFieldSetViaConstructorInjection", + "instanceField1", "instanceField2", "beforeEach1", "beforeEach2", "test1", "test2", "afterEach1", + "afterEach2") // + .values().hasSize(11).doesNotHaveDuplicates().allMatch(Files::notExists); var testBTempDirs = AllPossibleDeclarationLocationsTestCase.tempDirs.get("testB"); - assertThat(testBTempDirs).containsOnlyKeys("staticField1", "staticField2", "instanceField1", "instanceField2", - "beforeEach1", "beforeEach2", "test1", "test2", "afterEach1", "afterEach2"); - assertThat(testBTempDirs.values()).hasSize(10).doesNotHaveDuplicates(); + assertThat(testBTempDirs) // + .containsOnlyKeys("staticField1", "staticField2", "instanceFieldSetViaConstructorInjection", + "instanceField1", "instanceField2", "beforeEach1", "beforeEach2", "test1", "test2", "afterEach1", + "afterEach2") // + .values().hasSize(11).doesNotHaveDuplicates().allMatch(Files::notExists); assertThat(testATempDirs).containsEntry("staticField1", classTempDirs.get("staticField1")); assertThat(testBTempDirs).containsEntry("staticField1", classTempDirs.get("staticField1")); assertThat(testATempDirs).containsEntry("staticField2", classTempDirs.get("staticField2")); assertThat(testBTempDirs).containsEntry("staticField2", classTempDirs.get("staticField2")); + switch (lifecycle) { + case PER_CLASS -> assertThat(testATempDirs) // + .containsEntry("instanceFieldSetViaConstructorInjection", + testBTempDirs.get("instanceFieldSetViaConstructorInjection")); + case PER_METHOD -> assertThat(testATempDirs) // + .doesNotContainEntry("instanceFieldSetViaConstructorInjection", + testBTempDirs.get("instanceFieldSetViaConstructorInjection")); + } assertThat(testATempDirs).doesNotContainEntry("instanceField1", testBTempDirs.get("instanceField1")); assertThat(testATempDirs).doesNotContainEntry("instanceField2", testBTempDirs.get("instanceField2")); assertThat(testATempDirs).doesNotContainEntry("beforeEach1", testBTempDirs.get("beforeEach1")); @@ -278,26 +290,6 @@ void onlySupportsParametersOfTypePathAndFile() { // @formatter:on } - @Test - @DisplayName("when @TempDir is used on constructor parameter") - @Order(30) - void doesNotSupportTempDirAnnotationOnConstructorParameter() { - var results = executeTestsForClass(AnnotationOnConstructorParameterTestCase.class); - - assertSingleFailedTest(results, ParameterResolutionException.class, - "@TempDir is not supported on constructor parameters. Please use field injection instead."); - } - - @Test - @DisplayName("when @TempDir is used on constructor parameter with @TestInstance(PER_CLASS)") - @Order(31) - void doesNotSupportTempDirAnnotationOnConstructorParameterWithTestInstancePerClass() { - var results = executeTestsForClass(AnnotationOnConstructorParameterWithTestInstancePerClassTestCase.class); - - assertSingleFailedContainer(results, ParameterResolutionException.class, - "@TempDir is not supported on constructor parameters. Please use field injection instead."); - } - @Test @DisplayName("when @TempDir factory does not return directory") @Order(32) @@ -574,28 +566,6 @@ void test1() { } - static class AnnotationOnConstructorParameterTestCase { - - AnnotationOnConstructorParameterTestCase(@SuppressWarnings("unused") @TempDir Path tempDir) { - // never called - } - - @Test - void test() { - // never called - } - - } - - @TestInstance(PER_CLASS) - static class AnnotationOnConstructorParameterWithTestInstancePerClassTestCase - extends AnnotationOnConstructorParameterTestCase { - - AnnotationOnConstructorParameterWithTestInstancePerClassTestCase(@TempDir Path tempDir) { - super(tempDir); - } - } - static class InvalidTestCase { @Test @@ -1148,6 +1118,8 @@ static class AllPossibleDeclarationLocationsTestCase { @TempDir static Path staticField2; + final Path instanceFieldSetViaConstructorInjection; + @TempDir Path instanceField1; @@ -1162,6 +1134,11 @@ static void beforeAll(@TempDir Path param1, @TempDir Path param2, TestInfo testI "beforeAll1", param1, // "beforeAll2", param2 // )); + assertAllTempDirsExist(testInfo); + } + + AllPossibleDeclarationLocationsTestCase(@TempDir Path tempDir) { + this.instanceFieldSetViaConstructorInjection = tempDir; } @BeforeEach @@ -1169,11 +1146,13 @@ void beforeEach(@TempDir Path param1, @TempDir Path param2, TestInfo testInfo) { getTempDirs(testInfo).putAll(Map.of( // "staticField1", staticField1, // "staticField2", staticField2, // + "instanceFieldSetViaConstructorInjection", instanceFieldSetViaConstructorInjection, // "instanceField1", instanceField1, // "instanceField2", instanceField2, // "beforeEach1", param1, // "beforeEach2", param2 // )); + assertAllTempDirsExist(testInfo); } @Test @@ -1183,6 +1162,7 @@ void testA(@TempDir Path param1, @TempDir Path param2, TestInfo testInfo) { "test1", param1, // "test2", param2 // )); + assertAllTempDirsExist(testInfo); } @Test @@ -1192,6 +1172,7 @@ void testB(@TempDir Path param1, @TempDir Path param2, TestInfo testInfo) { "test1", param1, // "test2", param2 // )); + assertAllTempDirsExist(testInfo); } @AfterEach @@ -1200,6 +1181,7 @@ void afterEach(@TempDir Path param1, @TempDir Path param2, TestInfo testInfo) { "afterEach1", param1, // "afterEach2", param2 // )); + assertAllTempDirsExist(testInfo); } @AfterAll @@ -1208,11 +1190,16 @@ static void afterAll(@TempDir Path param1, @TempDir Path param2, TestInfo testIn "afterAll1", param1, // "afterAll2", param2 // )); + assertAllTempDirsExist(testInfo); } private static Map getTempDirs(TestInfo testInfo) { return tempDirs.computeIfAbsent(testInfo.getDisplayName(), __ -> new LinkedHashMap<>()); } + + private static void assertAllTempDirsExist(TestInfo testInfo) { + assertAll(getTempDirs(testInfo).values().stream().map(tempDir -> () -> assertTrue(Files.exists(tempDir)))); + } } static class UndeletableTestCase {