search

junit-team / junit5

✅ The 5th major version of the programmer-friendly testing framework for Java and the JVM
https://junit.org
Other
6.31k stars 1.46k forks source link

`@TempDir` should fail fast if `TempDirFactory::createTempDirectory` does not return a directory #3900

Closed scordio closed 1 month ago

scordio commented 1 month ago

The contract of TempDirFactory::createTempDirectory mentions:

https://github.com/junit-team/junit5/blob/adf6d4332b8a74db3d43f90982fd87b0d5197b3c/junit-jupiter-api/src/main/java/org/junit/jupiter/api/io/TempDirFactory.java#L61

However, the non-nullability of the returned Path is not enforced.

For example, the following:

@Test
void test(@TempDir(factory = Factory.class) Path tempDir) {
}

static class Factory implements TempDirFactory {
    @Override
    public Path createTempDirectory(AnnotatedElementContext elementContext, ExtensionContext extensionContext) throws Exception {
        return null;
    }
}

fails with:

Jul 26, 2024 5:39:46 PM org.junit.jupiter.engine.execution.JupiterEngineExecutionContext close
SEVERE: Caught exception while closing extension context: org.junit.jupiter.engine.descriptor.MethodExtensionContext@7364985f
java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.getFileSystem()" because "path" is null
Stack trace

``` Jul 26, 2024 5:39:46 PM org.junit.jupiter.engine.execution.JupiterEngineExecutionContext close SEVERE: Caught exception while closing extension context: org.junit.jupiter.engine.descriptor.MethodExtensionContext@7364985f java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.getFileSystem()" because "path" is null at java.base/java.nio.file.Files.provider(Files.java:104) at java.base/java.nio.file.Files.notExists(Files.java:2551) at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:322) at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:310) at org.junit.jupiter.engine.descriptor.AbstractExtensionContext.lambda$static$0(AbstractExtensionContext.java:45) at org.junit.platform.engine.support.store.NamespacedHierarchicalStore$EvaluatedValue.close(NamespacedHierarchicalStore.java:333) at org.junit.platform.engine.support.store.NamespacedHierarchicalStore$EvaluatedValue.access$800(NamespacedHierarchicalStore.java:317) at org.junit.platform.engine.support.store.NamespacedHierarchicalStore.lambda$close$3(NamespacedHierarchicalStore.java:98) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.store.NamespacedHierarchicalStore.lambda$close$4(NamespacedHierarchicalStore.java:98) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395) at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261) at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at org.junit.platform.engine.support.store.NamespacedHierarchicalStore.close(NamespacedHierarchicalStore.java:98) at org.junit.jupiter.engine.descriptor.AbstractExtensionContext.close(AbstractExtensionContext.java:87) at org.junit.jupiter.engine.execution.JupiterEngineExecutionContext.close(JupiterEngineExecutionContext.java:53) at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.cleanUp(JupiterTestDescriptor.java:224) at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$cleanUp$1(TestMethodTestDescriptor.java:156) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:156) at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.cleanUp(TestMethodTestDescriptor.java:69) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$cleanUp$10(NodeTestTask.java:167) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.hierarchical.NodeTestTask.cleanUp(NodeTestTask.java:167) at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:98) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141) at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138) at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141) at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137) at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139) at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138) at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95) at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35) at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57) at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:198) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:169) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:93) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:58) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:141) at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:57) at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:103) at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:85) at org.junit.platform.launcher.core.DelegatingLauncher.execute(DelegatingLauncher.java:47) at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:63) at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:57) at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38) at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:232) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:55) java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.getFileSystem()" because "path" is null at java.base/java.nio.file.Files.provider(Files.java:104) at java.base/java.nio.file.Files.notExists(Files.java:2551) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.stream.SortedOps$RefSortingSink.end(SortedOps.java:395) at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261) at java.base/java.util.stream.Sink$ChainedReference.end(Sink.java:261) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:510) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) ```

While it's good that the test doesn't pass, I propose to catch the problem earlier, for example in the CloseablePath constructor:

https://github.com/junit-team/junit5/blob/adf6d4332b8a74db3d43f90982fd87b0d5197b3c/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java#L287-L293

I'm happy to submit a PR if the proposal sounds reasonable.

sbrannen commented 1 month ago

Detecting that as early as possible and preemptively failing with an exception sounds good to me. 👍

Feel free to submit a PR!

sbrannen commented 1 month ago

In light of #3902, I've repurposed this issue to enforce the contract of TempDirFactory in general.

Specifically, we should fail fast if a factory does not return an actual directory -- for example, if the factory returns null or a file instead of a directory.