Add factory SPI for @TempDir

[scordio]

TL;DR

This could solve:

• Provide support for setting a custom parent directory for @TempDir #2088
• Support custom file system for @TempDir #2400
• Set a custom extension for @TempDir via a configuration parameter #2889
• create temp directory using system property (if available) java.io.tmpdir, use case is building some projects and be sure tmp directories are created under a configurable directory #3168

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(ExtensionContext context) throws IOException {
        return Files.createTempDirectory("junit");
    }

}

@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(ExtensionContext context) throws IOException {
        return Files.createTempDirectory(Paths.get("/tmp-dir-root"), "junit");
    }

}

Example with Custom File System

Using marschall/memoryfilesystem:

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

class MemoryFileSystemFactory implements TempDirFactory {

    private FileSystem fileSystem;

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

    @Override
    public void close() throws IOException {
        fileSystem.close();
    }
}

Using google/jimfs:

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

class JimfsFactory implements TempDirFactory {

    private FileSystem fileSystem;

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

    @Override
    public void close() throws IOException {
        fileSystem.close();
    }
}

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.default in a separate PR.

Open points

• Behavior in case of junit.jupiter.tempdir.scope=PER_CONTEXT: the extension fails if a custom factory is set
• 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)

To solve this, I catched the UnsupportedOperationException in TempDirectory.CloseablePath::resetPermissions and ignored it (b284505). In support of this, the Path::toFile javadoc mentions that the exception can be thrown if the Path is not associated with the default file system, which can be a potential use case once this SPI is offered.

---

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

[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]

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.

[marcphilipp]

Looks very promising! Could you please add an integration test using jimfs or memoryfilesystem by introducing an additional test dependency?

[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 scordio/junit5@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! 🎉