Repository: junit-team/junit5 (language: java)

Issue:
`@TempDir` directories of successful tests are not cleaned with `ON_SUCCESS` mode
<!-- Please provide us the version of JUnit 5 you are using and, if possible, a failing unit test with your bug report. Don't forget to describe the rationale for this issue (e.g. expected vs. actual behavior). Please also mention where it's a regression compared to a previous version. -->

In our test suite we’ve noticed that if any test fails (for whatever reason), then a lot of subsequent tests fail due to out-of-disk problems. This disk space is taken by temporary JUnit directories ([`org.junit.jupiter.api.io.TempDir`](https://github.com/junit-team/junit5/blob/main/junit-jupiter-api/src/main/java/org/junit/jupiter/api/io/TempDir.java)). We use `junit.jupiter.tempdir.cleanup.mode.default=ON_SUCCESS` property, so theoretically only directories for failed tests should be preserved, but we also see many directories for successful ones.

### Expected behavior

All temporary directories of successful tests should be deleted. It worked this way in `5.10.0`.

### Actual behavior

If one test fails, then temporary directories of subsequent successful tests are preserved. First observed in `5.10.1`.

I think it was introduced in https://github.com/junit-team/junit5/issues/3510 . Looks like [`selfOrChildFailed`](https://github.com/junit-team/junit5/blob/c83bfbc7e2739baae273051de79473b2e3ed640c/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java#L330-L335) starts treating all subsequent tests as failed.

## Steps to reproduce

<!-- Please insert a code snippet or a link to another repo along with instructions how to reproduce the issue here. The example should be minimal, complete and verifiable (see https://stackoverflow.com/help/mcve). -->

I’ve put a minimal reproducible example in this repository: https://github.com/msulima/junit-tmpdir-cleanup

## Context

 - JUnit: 5.10.1, 5.11.0, 5.12.1
 - Maven 3.9.9 + surefire 3.3.0



Discussion / hints:

Fix patch:
diff --git a/documentation/src/docs/asciidoc/release-notes/index.adoc b/documentation/src/docs/asciidoc/release-notes/index.adoc
index 6dc87c50c5de..af8a4feabdaf 100644
--- a/documentation/src/docs/asciidoc/release-notes/index.adoc
+++ b/documentation/src/docs/asciidoc/release-notes/index.adoc
@@ -23,6 +23,8 @@ include::{basedir}/release-notes-5.13.0-M2.adoc[]
 
 include::{basedir}/release-notes-5.13.0-M1.adoc[]
 
+include::{basedir}/release-notes-5.12.2.adoc[]
+
 include::{basedir}/release-notes-5.12.1.adoc[]
 
 include::{basedir}/release-notes-5.12.0.adoc[]
diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.2.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.2.adoc
new file mode 100644
index 000000000000..1c9ebf820b7e
--- /dev/null
+++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.12.2.adoc
@@ -0,0 +1,69 @@
+[[release-notes-5.12.2]]
+== 5.12.2
+
+*Date of Release:* ❓
+
+*Scope:* Bug fixes and enhancements since 5.12.1
+
+For a complete list of all _closed_ issues and pull requests for this release, consult the
+link:{junit5-repo}+/milestone/95?closed=1+[5.12.2] milestone page in the JUnit repository
+on GitHub.
+
+
+[[release-notes-5.12.2-junit-platform]]
+=== JUnit Platform
+
+[[release-notes-5.12.2-junit-platform-bug-fixes]]
+==== Bug Fixes
+
+* ❓
+
+[[release-notes-5.12.2-junit-platform-deprecations-and-breaking-changes]]
+==== Deprecations and Breaking Changes
+
+* ❓
+
+[[release-notes-5.12.2-junit-platform-new-features-and-improvements]]
+==== New Features and Improvements
+
+* ❓
+
+
+[[release-notes-5.12.2-junit-jupiter]]
+=== JUnit Jupiter
+
+[[release-notes-5.12.2-junit-jupiter-bug-fixes]]
+==== Bug Fixes
+
+* Fix handling of `CleanupMode.ON_SUCCESS` with `@TempDir` that caused no temporary
+  directories (using that mode) to be deleted after the first failure even if the
+  corresponding tests passed.
+
+[[release-notes-5.12.2-junit-jupiter-deprecations-and-breaking-changes]]
+==== Deprecations and Breaking Changes
+
+* ❓
+
+[[release-notes-5.12.2-junit-jupiter-new-features-and-improvements]]
+==== New Features and Improvements
+
+* ❓
+
+
+[[release-notes-5.12.2-junit-vintage]]
+=== JUnit Vintage
+
+[[release-notes-5.12.2-junit-vintage-bug-fixes]]
+==== Bug Fixes
+
+* ❓
+
+[[release-notes-5.12.2-junit-vintage-deprecations-and-breaking-changes]]
+==== Deprecations and Breaking Changes
+
+* ❓
+
+[[release-notes-5.12.2-junit-vintage-new-features-and-improvements]]
+==== New Features and Improvements
+
+* ❓
diff --git a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java
index 5349499d4f09..7d00f097e345 100644
--- a/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java
+++ b/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TempDirectory.java
@@ -129,12 +129,17 @@ public void beforeEach(ExtensionContext context) {
 	}
 
 	private static void installFailureTracker(ExtensionContext context) {
-		context.getStore(NAMESPACE).put(FAILURE_TRACKER, (CloseableResource) () -> context.getParent() //
-				.ifPresent(parentContext -> {
-					if (selfOrChildFailed(context)) {
-						parentContext.getStore(NAMESPACE).put(CHILD_FAILED, true);
-					}
-				}));
+		context.getParent() //
+				.filter(parentContext -> !context.getRoot().equals(parentContext)) //
+				.ifPresent(parentContext -> installFailureTracker(context, parentContext));
+	}
+
+	private static void installFailureTracker(ExtensionContext context, ExtensionContext parentContext) {
+		context.getStore(NAMESPACE).put(FAILURE_TRACKER, (CloseableResource) () -> {
+			if (selfOrChildFailed(context)) {
+				getContextSpecificStore(parentContext).put(CHILD_FAILED, true);
+			}
+		});
 	}
 
 	private void injectStaticFields(ExtensionContext context, Class<?> testClass) {
@@ -286,7 +291,11 @@ static CloseablePath createTempDir(TempDirFactory factory, CleanupMode cleanupMo
 
 	private static boolean selfOrChildFailed(ExtensionContext context) {
 		return context.getExecutionException().isPresent() //
-				|| context.getStore(NAMESPACE).getOrDefault(CHILD_FAILED, Boolean.class, false);
+				|| getContextSpecificStore(context).getOrDefault(CHILD_FAILED, Boolean.class, false);
+	}
+
+	private static ExtensionContext.Store getContextSpecificStore(ExtensionContext context) {
+		return context.getStore(NAMESPACE.append(context));
 	}
 
 	static class CloseablePath implements CloseableResource {


Test patch:
diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java
index d6d161db3381..b8d0b90468aa 100644
--- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java
+++ b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TempDirectoryCleanupTests.java
@@ -27,7 +27,7 @@
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 
-import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.MethodOrderer;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Order;
@@ -60,6 +60,7 @@ class TempDirFieldTests {
 		private static Path alwaysFieldDir;
 		private static Path onSuccessFailingFieldDir;
 		private static Path onSuccessPassingFieldDir;
+		private static Path onSuccessPassingParameterDir;
 
 		/**
 		 * Ensure the cleanup mode defaults to ALWAYS for fields.
@@ -152,6 +153,14 @@ void cleanupModeOnSuccessFailingField() {
 			assertThat(onSuccessFailingFieldDir).exists();
 		}
 
+		@Test
+		void cleanupModeOnSuccessFailingThenPassingField() {
+			executeTests(selectClass(OnSuccessFailingFieldCase.class), selectClass(OnSuccessPassingFieldCase.class));
+
+			assertThat(onSuccessFailingFieldDir).exists();
+			assertThat(onSuccessPassingFieldDir).doesNotExist();
+		}
+
 		/**
 		 * Ensure that ON_SUCCESS cleanup modes are obeyed for static fields when tests are failing.
 		 * <p/>
@@ -174,21 +183,20 @@ void cleanupModeOnSuccessFailingStaticField() {
 		 */
 		@Test
 		void cleanupModeOnSuccessFailingStaticFieldWithNesting() {
-			LauncherDiscoveryRequest request = request()//
-					.selectors(selectClass(OnSuccessFailingStaticFieldWithNestingCase.class))//
-					.build();
-			executeTests(request);
+			executeTestsForClass(OnSuccessFailingStaticFieldWithNestingCase.class);
 
 			assertThat(onSuccessFailingFieldDir).exists();
+			assertThat(onSuccessPassingParameterDir).doesNotExist();
 		}
 
-		@AfterAll
-		static void afterAll() throws IOException {
+		@AfterEach
+		void deleteTempDirs() throws IOException {
 			deleteIfNotNullAndExists(defaultFieldDir);
 			deleteIfNotNullAndExists(neverFieldDir);
 			deleteIfNotNullAndExists(alwaysFieldDir);
 			deleteIfNotNullAndExists(onSuccessFailingFieldDir);
 			deleteIfNotNullAndExists(onSuccessPassingFieldDir);
+			deleteIfNotNullAndExists(onSuccessPassingParameterDir);
 		}
 
 		static void deleteIfNotNullAndExists(Path dir) throws IOException {
@@ -286,13 +294,21 @@ static class OnSuccessFailingStaticFieldWithNestingCase {
 			static Path onSuccessFailingFieldDir;
 
 			@Nested
+			@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
 			class NestedTestCase {
 
 				@Test
-				void test() {
+				@Order(1)
+				void failingTest() {
 					TempDirFieldTests.onSuccessFailingFieldDir = onSuccessFailingFieldDir;
 					fail();
 				}
+
+				@Test
+				@Order(2)
+				void passingTest(@TempDir(cleanup = ON_SUCCESS) Path tempDir) {
+					TempDirFieldTests.onSuccessPassingParameterDir = tempDir;
+				}
 			}
 		}
 
@@ -400,8 +416,16 @@ void cleanupModeOnSuccessFailingParameter() {
 			assertThat(onSuccessFailingParameterDir).exists();
 		}
 
-		@AfterAll
-		static void afterAll() throws IOException {
+		@Test
+		void cleanupModeOnSuccessFailingThenPassingParameter() {
+			executeTestsForClass(OnSuccessFailingThenPassingParameterCase.class);
+
+			assertThat(onSuccessFailingParameterDir).exists();
+			assertThat(onSuccessPassingParameterDir).doesNotExist();
+		}
+
+		@AfterEach
+		void deleteTempDirs() throws IOException {
 			TempDirFieldTests.deleteIfNotNullAndExists(defaultParameterDir);
 			TempDirFieldTests.deleteIfNotNullAndExists(neverParameterDir);
 			TempDirFieldTests.deleteIfNotNullAndExists(alwaysParameterDir);
@@ -457,6 +481,24 @@ void testOnSuccessFailingParameter(@TempDir(cleanup = ON_SUCCESS) Path onSuccess
 			}
 		}
 
+		@SuppressWarnings({ "JUnitMalformedDeclaration", "NewClassNamingConvention" })
+		@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+		static class OnSuccessFailingThenPassingParameterCase {
+
+			@Test
+			@Order(1)
+			void testOnSuccessFailingParameter(@TempDir(cleanup = ON_SUCCESS) Path onSuccessFailingParameterDir) {
+				TempDirParameterTests.onSuccessFailingParameterDir = onSuccessFailingParameterDir;
+				fail();
+			}
+
+			@Test
+			@Order(2)
+			void testOnSuccessPassingParameter(@TempDir(cleanup = ON_SUCCESS) Path onSuccessPassingParameterDir) {
+				TempDirParameterTests.onSuccessPassingParameterDir = onSuccessPassingParameterDir;
+			}
+		}
+
 	}
 
 	@Nested