Introduce feature flag for auto-closing `AutoCloseable` in Jupiter's ExtensionContext.Store #4452

YongGoose · 2025-04-06T14:12:44Z

Overview

fix #4434

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

Issue junit-team#4434

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/ResourceAutoClosingTests.java

marcphilipp · 2025-04-08T13:56:26Z

...piter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java

-			((CloseableResource) value).close();
-		}
-	};
+	private static NamespacedHierarchicalStore.CloseAction<org.junit.platform.engine.support.store.Namespace> CLOSE_RESOURCES;


This should no longer be static since it depends on the instance-specific JupiterConfiguration.

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/ResourceAutoClosingTests.java

jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TestInstanceFactoryTests.java

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/ResourceAutoClosingTests.java

…r/ResourceAutoClosingTests.java Co-authored-by: Marc Philipp <[email protected]>

Issue junit-team#4434

marcphilipp · 2025-04-11T13:18:14Z

documentation/src/docs/asciidoc/user-guide/extensions.adoc

+To determine if your extension is running under JUnit Jupiter 5.13+, you can check if auto-close is enabled via the configuration:
+
+[source,java,indent=0]
+----
+boolean isJupiter513OrNewer = context.getConfigurationParameter("junit.jupiter.extensions.autoclose.enabled")
+    .map(Boolean::parseBoolean)
+    .orElse(true); // Default is true in 5.13+
+----
+


I'd omit that as it's not reliable since the config param could be set on earlier versions.

...piter-api/src/main/java/org/junit/jupiter/api/extension/TestInstantiationAwareExtension.java

marcphilipp · 2025-04-11T13:19:55Z

...piter-api/src/main/java/org/junit/jupiter/api/extension/TestInstantiationAwareExtension.java

@@ -65,7 +64,7 @@ public interface TestInstantiationAwareExtension extends Extension {
 	 * <li>{@link ExtensionContext#getTestMethod() getTestMethod()} is no longer
 	 * empty, unless the {@link TestInstance.Lifecycle#PER_CLASS PER_CLASS}
 	 * lifecycle is used.</li>
-	 * <li>If the callback adds a new {@link CloseableResource} to the
+	 * <li>If the callback adds a new {@link Store.CloseableResource} to the


Please also mention AutoCloseable here as in the other places you've adapted the Javadoc.

marcphilipp · 2025-04-11T13:22:55Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/config/JupiterConfiguration.java

@@ -42,14 +42,15 @@ public interface JupiterConfiguration {
 	String EXTENSIONS_AUTODETECTION_EXCLUDE_PROPERTY_NAME = "junit.jupiter.extensions.autodetection.exclude";
 	String DEACTIVATE_CONDITIONS_PATTERN_PROPERTY_NAME = "junit.jupiter.conditions.deactivate";
 	String PARALLEL_EXECUTION_ENABLED_PROPERTY_NAME = "junit.jupiter.execution.parallel.enabled";
+	String AUTOCLOSE_ENABLED_PROPERTY_NAME = "junit.jupiter.extensions.autoclose.enabled";


I'm not sure about the name. I'll discuss it with the team.

Suggested change

String AUTOCLOSE_ENABLED_PROPERTY_NAME = "junit.jupiter.extensions.autoclose.enabled";

String CLOSING_STORED_AUTO_CLOSEABLE_ENABLED_PROPERTY_NAME = "junit.jupiter.extensions.store.close.autocloseable.enabled";

IMHO, I think it's a good name.
Please discuss it with the team and let me know the outcome!

marcphilipp · 2025-04-11T13:23:18Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/config/JupiterConfiguration.java

@@ -60,6 +61,8 @@ public interface JupiterConfiguration {

 	boolean isParallelExecutionEnabled();

+	boolean isAutoCloseEnabled();


🤔

Suggested change

boolean isAutoCloseEnabled();

boolean isClosingStoredAutoCloseablesEnabled();

marcphilipp · 2025-04-11T13:27:45Z

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

@@ -289,7 +288,7 @@ private static boolean selfOrChildFailed(ExtensionContext context) {
 				|| context.getStore(NAMESPACE).getOrDefault(CHILD_FAILED, Boolean.class, false);
 	}

-	static class CloseablePath implements CloseableResource {
+	static class CloseablePath implements AutoCloseable {


We need to implement both here in case the new config param is set to false (the same goes for all implementations of CloseableResource in production code).

We need to implement both here in case the new config param is set to false (the same goes for all implementations of CloseableResource in production code).

I overlooked the case where the config param could be set to false.
I've updated the code to implement both, as you suggested. 🙂

Thank you @marcphilipp !!

marcphilipp · 2025-04-11T13:28:22Z

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

@@ -129,7 +128,7 @@ public void beforeEach(ExtensionContext context) {
 	}

 	private static void installFailureTracker(ExtensionContext context) {
-		context.getStore(NAMESPACE).put(FAILURE_TRACKER, (CloseableResource) () -> context.getParent() //
+		context.getStore(NAMESPACE).put(FAILURE_TRACKER, (AutoCloseable) () -> context.getParent() //


I think we should extract a class here that also implements both.

marcphilipp · 2025-04-11T13:28:36Z

junit-jupiter-params/src/main/java/org/junit/jupiter/params/ParameterizedInvocationContext.java

@@ -76,6 +76,7 @@ private void storeParameterInfo(ExtensionContext context) {
 		new DefaultParameterInfo(declarations, accessor).store(context);
 	}

+	@SuppressWarnings("deprecation")
 	private static class CloseableArgument implements ExtensionContext.Store.CloseableResource {


Should also implement both

marcphilipp · 2025-04-11T13:28:42Z

...r-tests/src/test/java/org/junit/jupiter/api/extension/CloseableResourceIntegrationTests.java

@@ -80,7 +79,7 @@ static class ThrowingOnCloseExtension implements BeforeEachCallback {

 		@Override
 		public void beforeEach(ExtensionContext context) {
-			context.getStore(GLOBAL).put("throwingResource", (ExtensionContext.Store.CloseableResource) () -> {
+			context.getStore(GLOBAL).put("throwingResource", (AutoCloseable) () -> {


marcphilipp · 2025-04-11T13:28:57Z

jupiter-tests/src/test/java/org/junit/jupiter/engine/ClassTemplateInvocationTests.java

@@ -1175,7 +1174,7 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte
 			}
 		}

-		static class SomeResource implements CloseableResource {
+		static class SomeResource implements AutoCloseable {


This is fine in test code. 👍

Issue junit-team#4434

marcphilipp · 2025-04-14T08:45:19Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ExtensionContext.java

 		 * <p>If {@code type} implements {@link ExtensionContext.Store.CloseableResource}
-		 * the {@code close()} method will be invoked on the stored object when
+		 * or {@link AutoCloseable} the {@code close()} method will be invoked on the stored object when
 		 * the store is closed.


* <p>If {@code type} implements {@link CloseableResource} or * {@link AutoCloseable} (unless the * {@code junit.jupiter.extensions.store.close.autocloseable.enabled} * configuration parameter is set to {@code false}), then the {@code close()} * method will be invoked on the stored object when the store is closed.

Done in d498200 !

...piter-engine/src/main/java/org/junit/jupiter/engine/descriptor/AbstractExtensionContext.java

marcphilipp · 2025-04-14T08:55:30Z

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/execution/NamespaceAwareStore.java

@@ -29,6 +31,8 @@
 @API(status = INTERNAL, since = "5.0")
 public class NamespaceAwareStore implements Store {

+	private static final Logger LOGGER = LoggerFactory.getLogger(NamespaceAwareStore.class);


Please remove this logger again.

marcphilipp · 2025-04-14T08:57:58Z

...form-tooling-support-tests/src/test/java/platform/tooling/support/tests/ManagedResource.java

@@ -100,6 +100,7 @@ private <T> Resource<T> getOrCreateResource(ExtensionContext extensionContext, C
 		}
 	}

+	@SuppressWarnings("deprecation")
 	class Resource<T> implements CloseableResource {


We should use AutoCloseable here

marcphilipp · 2025-04-14T08:58:02Z

...ing-support-tests/src/test/java/platform/tooling/support/tests/OutputAttachingExtension.java

@@ -76,6 +76,7 @@ private static boolean notEmpty(Path file) {
 		}
 	}

+	@SuppressWarnings("deprecation")
 	record OutputDir(Path root) implements ExtensionContext.Store.CloseableResource {


marcphilipp · 2025-04-14T08:59:11Z

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/ResourceAutoClosingTests.java

+		store.put("resource", resource);
+
+		((AutoCloseable) extensionContext).close();
+		assertThat(listener.stream(Level.WARNING)).map(LogRecord::getMessage).noneMatch(msg::equals);


Would be easier to assert that there are not log messages, WDYT?

That seems like a better way. 🙂

marcphilipp · 2025-04-14T08:59:58Z

documentation/src/test/java/example/extensions/HttpServerResource.java

 */
 // tag::user_guide[]
-class HttpServerResource implements CloseableResource {
+class HttpServerResource implements AutoCloseable {


We also need to update the Asciidoc title for this snippet.

IMHO, Since HttpServerResource is used as an example to explain CloseableResource, I think it would be better to revert it back to CloseableResource instead of AutoCloseable.

.`ExtensionContext.Store.CloseableResource` NOTE: An extension context store is bound to its extension context lifecycle. When an extension context lifecycle ends it closes its associated store. All stored values that are instances of `CloseableResource` are notified by an invocation of their `close()` method in the inverse order they were added in. An example implementation of `CloseableResource` is shown below, using an `HttpServer` resource. [source,java,indent=0] .`HttpServer` resource implementing `CloseableResource`

Using an example with AutoCloseable while explaining Closeable might confuse the reader.
@marcphilipp WDYT? 🤔

I think it should be used to explain AutoCloseable instead and we should revamp that section to mention that older releases used CloseableResource instead which is still supported but no longer recommended etc.

I think it should be used to explain AutoCloseable instead and we should revamp that section to mention that older releases used CloseableResource instead which is still supported but no longer recommended etc.

Done in 3a96c24 !

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

Issue junit-team#4434

…nfig/JupiterConfiguration.java Co-authored-by: Marc Philipp <[email protected]>

Issue junit-team#4434

YongGoose added 3 commits April 6, 2025 16:30

Deprecate CloseableResource

5cf0a08

Issue junit-team#4434

Introduce configuration parameter

a524c83

Issue junit-team#4434

Log warning for CloseableResource without AutoCloseable

5c7b99e

Issue junit-team#4434

YongGoose force-pushed the feature/4434 branch from 2a428f8 to 66a1772 Compare April 6, 2025 14:19

Add tests

44dfb8a

Issue junit-team#4434

YongGoose force-pushed the feature/4434 branch from 66a1772 to 44dfb8a Compare April 6, 2025 14:19

YongGoose commented Apr 6, 2025

View reviewed changes

jupiter-tests/src/test/java/org/junit/jupiter/engine/descriptor/ResourceAutoClosingTests.java Show resolved Hide resolved

marcphilipp reviewed Apr 8, 2025

View reviewed changes

YongGoose and others added 6 commits April 9, 2025 21:28

Update jupiter-tests/src/test/java/org/junit/jupiter/engine/descripto…

0e22d7b

…r/ResourceAutoClosingTests.java Co-authored-by: Marc Philipp <[email protected]>

Merge branch 'main' into feature/4434

f52d0e0

Apply comments

35f25db

Issue junit-team#4434

Merge branch 'main' into feature/4434

8c6dbb3

Add documents

708af7d

Issue junit-team#4434

Replace warning-suppressed code with AutoCloseable usage

849492d

Issue junit-team#4434

YongGoose marked this pull request as ready for review April 9, 2025 14:18

YongGoose requested a review from marcphilipp April 9, 2025 14:19

YongGoose added 2 commits April 11, 2025 16:40

Merge branch 'main' into feature/4434

f0af9bb

Merge branch 'main' into feature/4434

a2746b3

marcphilipp requested changes Apr 11, 2025

View reviewed changes

YongGoose added 2 commits April 12, 2025 17:13

Merge branch 'main' into feature/4434

c555910

Apply comment

c82c098

Issue junit-team#4434

YongGoose requested a review from marcphilipp April 12, 2025 10:15

YongGoose force-pushed the feature/4434 branch from d2c7606 to ea24f14 Compare April 12, 2025 10:22

Test warning logs for CloseableResource without AutoCloseable

8a5b7fe

Issue junit-team#4434

YongGoose force-pushed the feature/4434 branch from ea24f14 to 8a5b7fe Compare April 12, 2025 10:26

Merge branch 'main' into feature/4434

e8cb1d3

marcphilipp requested changes Apr 14, 2025

View reviewed changes

YongGoose and others added 3 commits April 15, 2025 18:57

Merge branch 'main' into feature/4434

46a8df0

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/de…

7fab7eb

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/de…

8057fe9

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

YongGoose and others added 7 commits April 15, 2025 19:08

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/de…

ed2df61

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/de…

a255a23

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/de…

46a606d

…scriptor/AbstractExtensionContext.java Co-authored-by: Marc Philipp <[email protected]>

Apply comment

d7a985e

Issue junit-team#4434

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/co…

c05235e

…nfig/JupiterConfiguration.java Co-authored-by: Marc Philipp <[email protected]>

Update junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/co…

49cb3f5

…nfig/JupiterConfiguration.java Co-authored-by: Marc Philipp <[email protected]>

Update javadoc

d498200

Issue junit-team#4434

YongGoose requested a review from marcphilipp April 15, 2025 10:46

polishing

056d060

Issue junit-team#4434

YongGoose force-pushed the feature/4434 branch from 49cc225 to 056d060 Compare April 15, 2025 10:49

Update document

d25f1b0

Issue junit-team#4434

YongGoose force-pushed the feature/4434 branch from 3a96c24 to d25f1b0 Compare April 18, 2025 00:06

Merge branch 'main' into feature/4434

30a4b01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce feature flag for auto-closing `AutoCloseable` in Jupiter's ExtensionContext.Store #4452

Introduce feature flag for auto-closing `AutoCloseable` in Jupiter's ExtensionContext.Store #4452

YongGoose commented Apr 6, 2025 •

edited

Loading

marcphilipp Apr 8, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

YongGoose Apr 12, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

YongGoose Apr 12, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 11, 2025

marcphilipp Apr 14, 2025

YongGoose Apr 15, 2025

marcphilipp Apr 14, 2025

marcphilipp Apr 14, 2025

marcphilipp Apr 14, 2025

marcphilipp Apr 14, 2025

YongGoose Apr 15, 2025

marcphilipp Apr 14, 2025

YongGoose Apr 15, 2025

marcphilipp Apr 16, 2025

YongGoose Apr 16, 2025

	String AUTOCLOSE_ENABLED_PROPERTY_NAME = "junit.jupiter.extensions.autoclose.enabled";
	String CLOSING_STORED_AUTO_CLOSEABLE_ENABLED_PROPERTY_NAME = "junit.jupiter.extensions.store.close.autocloseable.enabled";

		@@ -60,6 +61,8 @@ public interface JupiterConfiguration {

		boolean isParallelExecutionEnabled();

		boolean isAutoCloseEnabled();

	boolean isAutoCloseEnabled();
	boolean isClosingStoredAutoCloseablesEnabled();

Introduce feature flag for auto-closing AutoCloseable in Jupiter's ExtensionContext.Store #4452

Are you sure you want to change the base?

Introduce feature flag for auto-closing AutoCloseable in Jupiter's ExtensionContext.Store #4452

Conversation

YongGoose commented Apr 6, 2025 • edited Loading

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce feature flag for auto-closing `AutoCloseable` in Jupiter's ExtensionContext.Store #4452

Introduce feature flag for auto-closing `AutoCloseable` in Jupiter's ExtensionContext.Store #4452

YongGoose commented Apr 6, 2025 •

edited

Loading