Add factory SPI for `@TempDir`

[scordio]

TL;DR

This could solve:

• #1786
• #2088
• #2400
• #2889

Overview

This adds a factory SPI to @TempDir, allowing to define how the temporary directory is created.

My initial target was to support a custom file system (#2400), but it may also be a solution for the issues mentioned above.

The intended usage is to extend TempDirFactory and provide the class to the new factory attribute of @TempDir:

...
import java.nio.file.Files;
...

class CustomFactory implements TempDirFactory {

    @Override
    public Path createTempDirectory(String prefix) throws IOException {
        return Files.createTempDirectory("custom-prefix");
    }

}

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

Example with Custom Parent Directory

...
import java.nio.file.Files;
...

class CustomParentFactory implements TempDirFactory {

    @Override
    public Path createTempDirectory(String prefix) throws IOException {
        return Files.createTempDirectory(Paths.get("/tmp-dir-root"), prefix);
    }

}

Example with Custom File System

Using marschall/memoryfilesystem:

...
import java.nio.file.Files;
...

class MemoryFileSystemFactory implements TempDirFactory {

    @Override
    public Path createTempDirectory(String prefix) throws IOException {
        FileSystem fileSystem = MemoryFileSystemBuilder.newEmpty().build();
        return Files.createTempDirectory(fileSystem.getPath("/"), prefix);
    }

}

Using google/jimfs:

...
import java.nio.file.Files;
...

class JimfsFactory implements TempDirFactory {

    @Override
    public Path createTempDirectory(String prefix) throws IOException {
        FileSystem fileSystem = Jimfs.newFileSystem(Configuration.unix());
        return Files.createTempDirectory(fileSystem.getPath("/"), prefix);
    }

}

I would be happy to receive any feedback on the direction I have taken.

If this feature gets accepted, I would like to add a global configuration parameter like junit.jupiter.tempdir.factory in a separate PR.

Open points

• [x] Testing strategy: new test cases in both TempDirectoryPerDeclarationTests and TempDirectoryPerContextTests, or separate class? --> new test cases in existing classes
• [ ] Behavior in case of junit.jupiter.tempdir.scope=PER_CONTEXT
• [x] Both in-memory file system libraries currently fail due to an unsupported Path::toFile call:

java.lang.UnsupportedOperationException: memory file system does not support #toFile()
	at com.github.marschall.memoryfilesystem.AbstractPath.toFile(AbstractPath.java:74)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.resetPermissions(TempDirectory.java:353)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:282)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:268)

java.lang.UnsupportedOperationException
	at com.google.common.jimfs.JimfsPath.toFile(JimfsPath.java:387)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.resetPermissions(TempDirectory.java:353)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:282)
	at org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:268)

Would it be an option to catch the UnsupportedOperationException in TempDirectory.CloseablePath::resetPermissions and ignore it? In support of this, the Path::toFile javadoc mentions that the exception can be thrown if the Path is not associated with the default provider, which can be a potential use case once this SPI is offered.

• [ ] Add test to cover b28450594c576d0433801fe73c0b9d377ea893a9
• [ ] Similar to #2969, should TempDirFactory::createTempDirectory allow returning null for falling back to the default factory?

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

---

Definition of Done

• [ ] There are no TODOs left in the code
• [ ] Method preconditions are checked and documented in the method's Javadoc
• [ ] Coding conventions (e.g. for logging) have been followed
• [ ] Change is covered by automated tests including corner cases, errors, and exception handling
• [ ] Public API has Javadoc and @API annotations
• [ ] Change is documented in the User Guide and Release Notes
• [ ] All continuous integration builds pass

[scordio]

Both in-memory file system libraries currently fail due to an unsupported Path::toFile call

After b28450594c576d0433801fe73c0b9d377ea893a9, both in-memory file system libraries work correctly.

[lacasseio]

I like what you did with this PR. I wonder if passing prefix is necessary given that all new examples ignore it and the standard implementation always uses the same prefix.

I also see this PR as a good way to customize the behaviour of the temporary directory, such as including a space. We have a custom test directory provider that namespace the directory per-class/method, i.e. test-files/Foo/myTestMethod, and the idea in this PR could help with that. However, we would need access to some information from the ExecutionContext, mainly the test method and/or test class. I won't say to include this feature in this PR, but I think it adds additional use cases to the TempDirFactory design. I don't think passing the ExecutionContext instead of the prefix is wise, but maybe something like TempDirContext could double for the prefix (if required). What do you think?

Finally, I think having a way to pass a global TempDirFactory implementation via system property would be useful. Users could still override the factory per @TempDir declaration. Maybe something like junit.jupiter.tempdir.factory=com.example.MyTempDirFactory.

I hope this PR gets a bit more traction because it provides the last bit of customization that TempDir needs.

[scordio]

I like what you did with this PR.

@lacasseio thanks for the feedback! I have to admit I've put this topic a bit aside lately, there are still some points to clarify and test coverage to improve... I'll get it finalized sooner or later 🙂

I wonder if passing prefix is necessary given that all new examples ignore it and the standard implementation always uses the same prefix.

Indeed, the prefix doesn't seem so useful right now but I'd let the JUnit team judge what to do there.

I also see this PR as a good way to customize the behaviour of the temporary directory, such as including a space. We have a custom test directory provider that namespace the directory per-class/method, i.e. test-files/Foo/myTestMethod, and the idea in this PR could help with that. However, we would need access to some information from the ExecutionContext, mainly the test method and/or test class. I won't say to include this feature in this PR, but I think it adds additional use cases to the TempDirFactory design. I don't think passing the ExecutionContext instead of the prefix is wise, but maybe something like TempDirContext could double for the prefix (if required). What do you think?

I agree TempDirFactory could have more input/control but I didn't feel the silver bullet yet, so I preferred starting with the smallest scope possible and for sure it could be extended with concrete use cases like yours. Especially here, I'd wait for some feedback from the core team before introducing something more complex.

Finally, I think having a way to pass a global TempDirFactory implementation via system property would be useful. Users could still override the factory per @TempDir declaration. Maybe something like junit.jupiter.tempdir.factory=com.example.MyTempDirFactory.

That was exactly my idea, I've mentioned it in the middle of the description:

If this feature gets accepted, I would like to add a global configuration parameter like junit.jupiter.tempdir.factory in a separate PR.

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions.

[lacasseio]

Looking at implementing something similar as part of my current "TempDir" extension, I think it would be important to ensure Factory can be AutoCloseable as, for example, Jimfs are AutoCloseable which signals that the file system should be closed once created (potentially after a test).

[scordio]

Thanks for your feedback, @lacasseio! I need to refresh this topic and will check how to apply what you mentioned, as what the factory produces is already wrapped in a CloseablePath.

[lacasseio]

Great, I just noticed the example didn't close the FileSystem object and though about mentioning it for completeness. You are right that it won't change the current architecture.

[marcphilipp]

Team Decision: Check whether TempDirFactory implements AutoCloseable and, if so, close it in CloseablePath after deleting the temp dir.

[olamy]

very good idea! This would be fantastic to have this available soon. A global configuration would be good as well such junit.jupiter.execution.tempdir.factory or any similar name.

[scordio]

@marcphilipp @sormuras may I ask for an initial review?

[marcphilipp]

@scordio This PR is a great next step but I don't think it solves #1786, does it?

[scordio]

Thanks, @marcphilipp for all your feedback. I think only documentation and changelog are left, I plan to work on them in the evening and I'll also raise a separate PR for the global configuration property.

[scordio]

@scordio This PR is a great next step but I don't think it solves #1786, does it?

You're right, I guess I had something else in mind when I mentioned it. I take it out from the description.

[scordio]

@marcphilipp I think I'm done with my changes, all the open points should be covered. Please, let me know if there is anything to adjust.

[scordio]

If this feature gets accepted, I would like to add a global configuration parameter like junit.jupiter.tempdir.factory.default in a separate PR.

Initial draft at https://github.com/scordio/junit5/compare/2400_temp_dir_factory...scordio:junit5:gh-2400_temp_dir_factory_config_property.

I'll raise a PR once this one is merged.

[marcphilipp]

@scordio Thanks a lot for the productive collaboration on this one! 🎉