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.

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

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

Merged
merged 5 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ 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!

* `@TempDir` now fails fast in case `TempDirFactory::createTempDirectory` returns
`null`, a file, or a symbolic link to a file.


[[release-notes-5.11.0-RC1-junit-vintage]]
Expand Down
2 changes: 1 addition & 1 deletion documentation/src/test/java/example/TempDirectoryDemo.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.

👍

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,14 +284,19 @@ static class CloseablePath implements CloseableResource {
private final CleanupMode cleanupMode;
private final ExtensionContext extensionContext;

CloseablePath(TempDirFactory factory, CleanupMode cleanupMode, AnnotatedElementContext elementContext,
private CloseablePath(TempDirFactory factory, CleanupMode cleanupMode, AnnotatedElementContext elementContext,
scordio marked this conversation as resolved.
Show resolved Hide resolved
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;
}
Comment on lines +287 to 293
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.


private static Path validateTempDirectory(Path dir) {
Preconditions.condition(dir != null && Files.isDirectory(dir), "temp directory must be a directory");
return dir;
}

Path get() {
return dir;
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
/*
* Copyright 2015-2024 the original author or authors.
*
* All rights reserved. This program and the accompanying materials are
* made available under the terms of the Eclipse Public License v2.0 which
* accompanies this distribution and is available at
*
* https://www.eclipse.org/legal/epl-v20.html
*/

package org.junit.jupiter.engine.extension;

import static java.nio.file.Files.createDirectory;
import static java.nio.file.Files.createFile;
import static java.nio.file.Files.createSymbolicLink;
import static java.nio.file.Files.createTempDirectory;
import static java.nio.file.Files.delete;
import static java.nio.file.Files.deleteIfExists;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.junit.jupiter.api.io.CleanupMode.ALWAYS;
import static org.junit.jupiter.api.io.CleanupMode.DEFAULT;
import static org.junit.jupiter.api.io.CleanupMode.NEVER;
import static org.junit.jupiter.api.io.CleanupMode.ON_SUCCESS;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.io.IOException;
import java.nio.file.Path;
import java.util.Optional;

import org.assertj.core.api.ThrowableAssert.ThrowingCallable;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.AnnotatedElementContext;
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.io.CleanupMode;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.api.io.TempDirFactory;
import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
import org.junit.jupiter.engine.execution.NamespaceAwareStore;
import org.junit.platform.commons.PreconditionViolationException;
import org.junit.platform.engine.support.store.NamespacedHierarchicalStore;

/**
* Integration tests for the creation and cleanup of the {@link TempDirectory}.
*
* @since 5.9
*/
@DisplayName("Temporary directory")
class CloseablePathTests extends AbstractJupiterTestEngineTests {

private final AnnotatedElementContext elementContext = mock();
private final ExtensionContext extensionContext = mock();

private TempDirectory.CloseablePath closeablePath;

@BeforeEach
void setUpExtensionContext() {
var store = new NamespaceAwareStore(new NamespacedHierarchicalStore<>(null), Namespace.GLOBAL);
when(extensionContext.getStore(any())).thenReturn(store);
}

/**
* Integration tests for the creation of the {@link TempDirectory} based on the different result
* that {@link TempDirFactory#createTempDirectory(AnnotatedElementContext, ExtensionContext)} may provide.
*
* @since 5.11
*
* @see TempDirFactory
*/
@Nested
@DisplayName("creation")
class Creation {

private Path root;

@BeforeEach
void setUpRootFolder() throws IOException {
root = createTempDirectory("root");
}

@AfterEach
void cleanupRoot() throws IOException {
delete(root);
}

@Test
@DisplayName("succeeds if the factory returns a directory")
void factoryReturnsDirectory() throws Exception {
TempDirFactory factory = (elementContext, extensionContext) -> createDirectory(root.resolve("directory"));

closeablePath = TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

delete(closeablePath.get());
}

@Test
@DisplayName("succeeds if the factory returns a symbolic link to a directory")
void factoryReturnsSymbolicLinkToDirectory() throws Exception {
Path directory = createDirectory(root.resolve("directory"));
TempDirFactory factory = (elementContext,
extensionContext) -> createSymbolicLink(root.resolve("symbolicLink"), directory);

closeablePath = TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

delete(closeablePath.get());
delete(directory);
}

@Test
@DisplayName("fails if the factory returns null")
void factoryReturnsNull() {
TempDirFactory factory = (elementContext, extensionContext) -> null;

assertThatExtensionConfigurationExceptionIsThrownBy(
() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));
}

@Test
@DisplayName("fails if the factory returns a file")
void factoryReturnsFile() throws IOException {
Path file = createFile(root.resolve("file"));
TempDirFactory factory = (elementContext, extensionContext) -> file;

assertThatExtensionConfigurationExceptionIsThrownBy(
() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));

delete(file);
}

@Test
@DisplayName("fails if the factory returns a symbolic link to a file")
void factoryReturnsSymbolicLinkToFile() throws IOException {
Path file = createFile(root.resolve("file"));
Path symbolicLink = createSymbolicLink(root.resolve("symbolicLink"), file);
TempDirFactory factory = (elementContext, extensionContext) -> symbolicLink;

assertThatExtensionConfigurationExceptionIsThrownBy(
() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));

delete(symbolicLink);
delete(file);
}

private static void assertThatExtensionConfigurationExceptionIsThrownBy(ThrowingCallable callable) {
assertThatExceptionOfType(ExtensionConfigurationException.class)//
.isThrownBy(callable)//
.withMessage("Failed to create default temp directory")//
.withCauseInstanceOf(PreconditionViolationException.class)//
.havingCause().withMessage("temp directory must be a directory");
}

}

/**
* Integration tests for cleanup of the {@link TempDirectory} when the {@link CleanupMode} is
* set to {@link CleanupMode#ALWAYS}, {@link CleanupMode#NEVER}, or {@link CleanupMode#ON_SUCCESS}.
*
* @since 5.9
*
* @see TempDir
* @see CleanupMode
*/
@Nested
@DisplayName("cleanup")
class Cleanup {

private final TempDirFactory factory = spy(TempDirFactory.Standard.INSTANCE);

@AfterEach
void cleanupTempDirectory() throws IOException {
deleteIfExists(closeablePath.get());
}

@Test
@DisplayName("is done for a cleanup mode of ALWAYS")
void always() throws IOException {
closeablePath = TempDirectory.createTempDir(factory, ALWAYS, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

closeablePath.close();
assertThat(closeablePath.get()).doesNotExist();
verify(factory).close();
}

@Test
@DisplayName("is not done for a cleanup mode of NEVER")
void never() throws IOException {
closeablePath = TempDirectory.createTempDir(factory, NEVER, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

closeablePath.close();
assertThat(closeablePath.get()).exists();
verify(factory).close();
}

@Test
@DisplayName("is not done for a cleanup mode of ON_SUCCESS, if there is an exception")
void onSuccessWithException() throws IOException {
when(extensionContext.getExecutionException()).thenReturn(Optional.of(new Exception()));

closeablePath = TempDirectory.createTempDir(factory, ON_SUCCESS, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

closeablePath.close();
assertThat(closeablePath.get()).exists();
verify(factory).close();
}

@Test
@DisplayName("is done for a cleanup mode of ON_SUCCESS, if there is no exception")
void onSuccessWithNoException() throws IOException {
when(extensionContext.getExecutionException()).thenReturn(Optional.empty());

closeablePath = TempDirectory.createTempDir(factory, ON_SUCCESS, elementContext, extensionContext);
assertThat(closeablePath.get()).isDirectory();

closeablePath.close();
assertThat(closeablePath.get()).doesNotExist();
verify(factory).close();
}

}

}
Loading
Loading