Ensure TempDir cleanup is executed when validation fails (junit-team#3911)

Author: scordio

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java

@@ -57,6 +57,7 @@
 import org.junit.jupiter.engine.config.EnumConfigurationParameterConverter;
 import org.junit.jupiter.engine.config.JupiterConfiguration;
 import org.junit.platform.commons.JUnitException;
+import org.junit.platform.commons.PreconditionViolationException;
 import org.junit.platform.commons.logging.Logger;
 import org.junit.platform.commons.logging.LoggerFactory;
 import org.junit.platform.commons.util.ExceptionUtils;
@@ -286,15 +287,15 @@ static class CloseablePath implements CloseableResource {
 
 		private CloseablePath(TempDirFactory factory, CleanupMode cleanupMode, AnnotatedElementContext elementContext,
 				ExtensionContext extensionContext) throws Exception {
-			this.dir = validateTempDirectory(factory.createTempDirectory(elementContext, extensionContext));
+			this.dir = factory.createTempDirectory(elementContext, extensionContext);
 			this.factory = factory;
 			this.cleanupMode = cleanupMode;
 			this.extensionContext = extensionContext;
-		}
 
-		private static Path validateTempDirectory(Path dir) {
-			Preconditions.condition(dir != null && Files.isDirectory(dir), "temp directory must be a directory");
-			return dir;
+			if (dir == null || !Files.isDirectory(dir)) {
+				close();
+				throw new PreconditionViolationException("temp directory must be a directory");
+			}
 		}
 
 		Path get() {
@@ -324,7 +325,7 @@ public void close() throws IOException {
 
 		private SortedMap<Path, IOException> deleteAllFilesAndDirectories(FileOperations fileOperations)
 				throws IOException {
-			if (Files.notExists(dir)) {
+			if (dir == null || Files.notExists(dir)) {
 				return Collections.emptySortedMap();
 			}
 

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/extension/CloseablePathTests.java

@@ -98,7 +98,7 @@ void cleanupRoot() throws IOException {
 		@Test
 		@DisplayName("succeeds if the factory returns a directory")
 		void factoryReturnsDirectory() throws Exception {
-			TempDirFactory factory = (elementContext, extensionContext) -> createDirectory(root.resolve("directory"));
+			TempDirFactory factory = spy(new Factory(createDirectory(root.resolve("directory"))));
 
 			closeablePath = TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext);
 			assertThat(closeablePath.get()).isDirectory();
@@ -111,8 +111,7 @@ void factoryReturnsDirectory() throws Exception {
 		@DisabledOnOs(OS.WINDOWS)
 		void factoryReturnsSymbolicLinkToDirectory() throws Exception {
 			Path directory = createDirectory(root.resolve("directory"));
-			TempDirFactory factory = (elementContext,
-					extensionContext) -> createSymbolicLink(root.resolve("symbolicLink"), directory);
+			TempDirFactory factory = spy(new Factory(createSymbolicLink(root.resolve("symbolicLink"), directory)));
 
 			closeablePath = TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext);
 			assertThat(closeablePath.get()).isDirectory();
@@ -123,23 +122,26 @@ void factoryReturnsSymbolicLinkToDirectory() throws Exception {
 
 		@Test
 		@DisplayName("fails if the factory returns null")
-		void factoryReturnsNull() {
-			TempDirFactory factory = (elementContext, extensionContext) -> null;
+		void factoryReturnsNull() throws IOException {
+			TempDirFactory factory = spy(new Factory(null));
 
 			assertThatExtensionConfigurationExceptionIsThrownBy(
 				() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));
+
+			verify(factory).close();
 		}
 
 		@Test
 		@DisplayName("fails if the factory returns a file")
 		void factoryReturnsFile() throws IOException {
 			Path file = createFile(root.resolve("file"));
-			TempDirFactory factory = (elementContext, extensionContext) -> file;
+			TempDirFactory factory = spy(new Factory(file));
 
 			assertThatExtensionConfigurationExceptionIsThrownBy(
 				() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));
 
-			delete(file);
+			verify(factory).close();
+			assertThat(file).doesNotExist();
 		}
 
 		@Test
@@ -148,15 +150,27 @@ void factoryReturnsFile() throws IOException {
 		void factoryReturnsSymbolicLinkToFile() throws IOException {
 			Path file = createFile(root.resolve("file"));
 			Path symbolicLink = createSymbolicLink(root.resolve("symbolicLink"), file);
-			TempDirFactory factory = (elementContext, extensionContext) -> symbolicLink;
+			TempDirFactory factory = spy(new Factory(symbolicLink));
 
 			assertThatExtensionConfigurationExceptionIsThrownBy(
 				() -> TempDirectory.createTempDir(factory, DEFAULT, elementContext, extensionContext));
 
-			delete(symbolicLink);
+			verify(factory).close();
+			assertThat(symbolicLink).doesNotExist();
+
 			delete(file);
 		}
 
+		// Mockito spying a lambda fails with: VM does not support modification of given type
+		private record Factory(Path path) implements TempDirFactory {
+
+			@Override
+			public Path createTempDirectory(AnnotatedElementContext elementContext, ExtensionContext extensionContext) {
+				return path;
+			}
+
+		}
+
 		private static void assertThatExtensionConfigurationExceptionIsThrownBy(ThrowingCallable callable) {
 			assertThatExceptionOfType(ExtensionConfigurationException.class)//
 					.isThrownBy(callable)//
@@ -194,8 +208,9 @@ void always() throws IOException {
 			assertThat(closeablePath.get()).isDirectory();
 
 			closeablePath.close();
-			assertThat(closeablePath.get()).doesNotExist();
+
 			verify(factory).close();
+			assertThat(closeablePath.get()).doesNotExist();
 		}
 
 		@Test
@@ -205,8 +220,9 @@ void never() throws IOException {
 			assertThat(closeablePath.get()).isDirectory();
 
 			closeablePath.close();
-			assertThat(closeablePath.get()).exists();
+
 			verify(factory).close();
+			assertThat(closeablePath.get()).exists();
 		}
 
 		@Test
@@ -218,8 +234,9 @@ void onSuccessWithException() throws IOException {
 			assertThat(closeablePath.get()).isDirectory();
 
 			closeablePath.close();
-			assertThat(closeablePath.get()).exists();
+
 			verify(factory).close();
+			assertThat(closeablePath.get()).exists();
 		}
 
 		@Test
@@ -231,8 +248,9 @@ void onSuccessWithNoException() throws IOException {
 			assertThat(closeablePath.get()).isDirectory();
 
 			closeablePath.close();
-			assertThat(closeablePath.get()).doesNotExist();
+
 			verify(factory).close();
+			assertThat(closeablePath.get()).doesNotExist();
 		}
 
 	}