Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Let @TempDir fail fast if createTempDirectory does not return a directory #3901

Merged
merged 5 commits into from
Jul 31, 2024
Merged

Let @TempDir fail fast if createTempDirectory does not return a directory #3901

merged 5 commits into from
Jul 31, 2024

Conversation

scordio
Copy link
Contributor

@scordio scordio commented Jul 26, 2024 •
edited
Loading

Overview

Resolves #3900.

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

Definition of Done

@scordio scordio force-pushed the gh-3900-tempdir-fail-fast branch 2 times, most recently from b7e8586 to 5ad2075 Compare July 26, 2024 21:52
@scordio scordio marked this pull request as ready for review July 26, 2024 21:55
@scordio scordio force-pushed the gh-3900-tempdir-fail-fast branch 2 times, most recently from f8a4fa4 to 004ca27 Compare July 26, 2024 22:03
@scordio scordio mentioned this pull request Jul 26, 2024
Closed
@scordio scordio changed the title Fail fast when TempDirFactory::createTempDirectory returns null Let @TempDir fail fast if createTempDirectory returns null Jul 26, 2024
@sbrannen
Copy link
Member

If you can update this to reflect #3902 as well, we can hopefully get this into 5.11 RC1.

@scordio scordio marked this pull request as draft July 28, 2024 09:05
@scordio
Copy link
Contributor Author

scordio commented Jul 28, 2024

Hi @sbrannen / team, I'm considering changing the testing strategy for these changes.

For the non-nullability check, I introduced additional integration tests in both TempDirectoryPerDeclarationTests and TempDirectoryPerContextTests, resulting in three new test cases in total.

However, now that the directory requirement should be tested and I want to cover both regular files and symbolic links, there is probably little value in having such a duplication. It might be enough to unit test TempDirectory.CloseablePath, similar to CloseablePathCleanupTests.

Which direction do you prefer?

@marcphilipp
Copy link
Member

I would change the tests you added to check for "not a directory" (not important whether it's null, a file, or symbolic link) and add unit tests for all specific cases to where the validation happens.

@marcphilipp marcphilipp changed the title Let @TempDir fail fast if createTempDirectory returns null Let @TempDir fail fast if createTempDirectory does not return a directory Jul 30, 2024
@marcphilipp
Copy link
Member

@scordio We'd like to release 5.11 RC1 tomorrow. Do you have time today to make the requested changes? If not, please let me know so I can finish it.

@scordio
Copy link
Contributor Author

scordio commented Jul 30, 2024

@marcphilipp yes, I have time 🙂

@scordio
Copy link
Contributor Author

scordio commented Jul 30, 2024

@marcphilipp anything against adding junit-jupiter-params as a test dependency of junit-jupiter-engine?

@marcphilipp
Copy link
Member

@marcphilipp anything against adding junit-jupiter-params as a test dependency of junit-jupiter-engine?

Unfortunately, yes, since that will break Eclipse which will create a cyclic project dependency since junit-jupiter-params has a test runtime dependency on junit-jupiter-engine. I also wanted to do this last week. For now, please use a dynamic test. Maybe we should introduce a jupiter-tests project similar to platform-tests to avoid that, but not in this PR.

@scordio scordio marked this pull request as ready for review July 30, 2024 14:25
@scordio
Copy link
Contributor Author

scordio commented Jul 31, 2024

For now, please use a dynamic test.

In the end I opted for standard tests, as the setup and cleanup differ between each test case.

marcphilipp
marcphilipp approved these changes Jul 31, 2024
View reviewed changes
documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-RC1.adoc
@@ -60,7 +60,8 @@ repository on GitHub.
`assertInstanceOf` methods introduced in `5.8` version.
* New generators in `DynamicTest` that take a `Stream`/`Iterator` of `Named<Executable>`
along with a convenient `NamedExecutable` interface that can simplify writing dynamic
tests, in particular in recent version of Java that support records.
tests, in particular in recent versions of Java that support records.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing my typo! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks for catching the release notes gap!

documentation/src/docs/asciidoc/release-notes/release-notes-5.11.0-RC1.adoc Outdated Show resolved Hide resolved
documentation/src/test/java/example/TempDirectoryDemo.java
@@ -137,7 +137,7 @@ static class JimfsTempDirFactory implements TempDirFactory {
@Override
public Path createTempDirectory(AnnotatedElementContext elementContext, ExtensionContext extensionContext)
throws IOException {
return Files.createTempDirectory(fileSystem.getPath("/"), "junit");
return Files.createTempDirectory(fileSystem.getPath("/"), "junit-");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@marcphilipp marcphilipp merged commit 2cdd597 into junit-team:main Jul 31, 2024
16 checks passed
@marcphilipp
Copy link
Member

@scordio Thanks for your quick and high-quality work on this! 👍

@scordio scordio deleted the gh-3900-tempdir-fail-fast branch July 31, 2024 10:49
scordio
scordio commented Aug 1, 2024
View reviewed changes
junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java
Comment on lines +287 to 293
private CloseablePath(TempDirFactory factory, CleanupMode cleanupMode, AnnotatedElementContext elementContext,
ExtensionContext extensionContext) throws Exception {
this.dir = factory.createTempDirectory(elementContext, extensionContext);
this.dir = validateTempDirectory(factory.createTempDirectory(elementContext, extensionContext));
this.factory = factory;
this.cleanupMode = cleanupMode;
this.extensionContext = extensionContext;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcphilipp I just realized a PreconditionViolationException thrown in the constructor would interrupt the execution without closing the factory.

Should we wrap it in a try/catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised #3911 for evaluation.

This was referenced Aug 1, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcphilipp marcphilipp marcphilipp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@TempDir should fail fast if TempDirFactory::createTempDirectory does not return a directory
3 participants
@scordio @sbrannen @marcphilipp