Ensure TestWatchers can access data in the ExtensionContext.Store

Changes made in conjunction with junit-team#3614 resulted in an exception being
thrown if a NamespacedHierarchicalStore was queried after it had been
closed. Consequently, TestWatcher callbacks were no longer able to
access data in the Store.

To fix that regression, this commit revises NamespacedHierarchicalStore
so that it no longer throws an exception after it has been closed if
the store is queried via one of the get(...) or
getOrComputeIfAbsent(...) methods; however, if a
getOrComputeIfAbsent(...) invocation results in the computation of a
new value, an exception will still be thrown. In other words, when
a NamespacedHierarchicalStore is closed, it now effectively switches to
ready-only mode.

See: junit-team#3614
Fixes: junit-team#3944

Author: sbrannen

documentation/src/docs/asciidoc/release-notes/release-notes-5.11.1.adoc

@@ -18,6 +18,10 @@ on GitHub.
 
 * Fixed potential locking issue with `ExclusiveResource` in the
   `HierarchicalTestExecutorService`, which could lead to deadlocks in certain scenarios.
+* `NamespacedHierarchicalStore` no longer throws an exception after it has been closed if
+  the store is queried via one of the `get(...)` or `getOrComputeIfAbsent(...)` methods;
+  however, if a `getOrComputeIfAbsent(...)` invocation results in the computation of a new
+  value, an exception will still be thrown.
 
 [[release-notes-5.11.1-junit-platform-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes
@@ -37,7 +41,8 @@ on GitHub.
 [[release-notes-5.11.1-junit-jupiter-bug-fixes]]
 ==== Bug Fixes
 
-* ❓
+* `TestWatcher` callback methods can once again access data in the
+  `ExtensionContext.Store`.
 
 [[release-notes-5.11.1-junit-jupiter-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java

@@ -139,7 +139,6 @@ public void close() {
 	 * closed
 	 */
 	public Object get(N namespace, Object key) {
-		rejectIfClosed();
 		StoredValue storedValue = getStoredValue(new CompositeKey<>(namespace, key));
 		return StoredValue.evaluateIfNotNull(storedValue);
 	}
@@ -156,7 +155,6 @@ public Object get(N namespace, Object key) {
 	 * be cast to the required type, or if this store has already been closed
 	 */
 	public <T> T get(N namespace, Object key, Class<T> requiredType) throws NamespacedHierarchicalStoreException {
-		rejectIfClosed();
 		Object value = get(namespace, key);
 		return castToRequiredType(key, value, requiredType);
 	}
@@ -174,13 +172,15 @@ public <T> T get(N namespace, Object key, Class<T> requiredType) throws Namespac
 	 * closed
 	 */
 	public <K, V> Object getOrComputeIfAbsent(N namespace, K key, Function<K, V> defaultCreator) {
-		rejectIfClosed();
 		Preconditions.notNull(defaultCreator, "defaultCreator must not be null");
 		CompositeKey<N> compositeKey = new CompositeKey<>(namespace, key);
 		StoredValue storedValue = getStoredValue(compositeKey);
 		if (storedValue == null) {
 			storedValue = this.storedValues.computeIfAbsent(compositeKey,
-				__ -> storedValue(new MemoizingSupplier(() -> defaultCreator.apply(key))));
+				__ -> storedValue(new MemoizingSupplier(() -> {
+					rejectIfClosed();
+					return defaultCreator.apply(key);
+				})));
 		}
 		return storedValue.evaluate();
 	}
@@ -202,7 +202,6 @@ public <K, V> Object getOrComputeIfAbsent(N namespace, K key, Function<K, V> def
 	public <K, V> V getOrComputeIfAbsent(N namespace, K key, Function<K, V> defaultCreator, Class<V> requiredType)
 			throws NamespacedHierarchicalStoreException {
 
-		rejectIfClosed();
 		Object value = getOrComputeIfAbsent(namespace, key, defaultCreator);
 		return castToRequiredType(key, value, requiredType);
 	}

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

@@ -12,6 +12,7 @@
 
 import static java.util.function.Predicate.not;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
@@ -41,8 +42,11 @@
 import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.junit.jupiter.api.TestMethodOrder;
+import org.junit.jupiter.api.extension.BeforeTestExecutionCallback;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
+import org.junit.jupiter.api.extension.ExtensionContext.Store;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.extension.TestWatcher;
 import org.junit.jupiter.api.fixtures.TrackLogRecords;
@@ -67,6 +71,7 @@ class TestWatcherTests extends AbstractJupiterTestEngineTests {
 	@BeforeEach
 	void clearResults() {
 		TrackingTestWatcher.results.clear();
+		DataRetrievingTestWatcher.results.clear();
 	}
 
 	@Test
@@ -167,6 +172,16 @@ void testWatcherSemanticsWhenRegisteredAtMethodLevel() {
 		assertThat(TrackingTestWatcher.results.get("testDisabled")).containsExactly("test", "repeatedTest");
 	}
 
+	@Test
+	void testWatcherCanRetrieveDataFromTheExtensionContextStore() {
+		Class<?> testClass = DataRetrievingTestWatcherTestCase.class;
+		EngineExecutionResults results = executeTestsForClass(testClass);
+
+		results.testEvents().assertStatistics(stats -> stats.started(1).succeeded(1).failed(0));
+
+		assertThat(DataRetrievingTestWatcher.results).containsExactly(entry("key", "enigma"));
+	}
+
 	private void assertCommonStatistics(EngineExecutionResults results) {
 		results.containerEvents().assertStatistics(stats -> stats.started(3).succeeded(3).failed(0));
 		results.testEvents().assertStatistics(stats -> stats.skipped(2).started(6).succeeded(2).aborted(2).failed(2));
@@ -414,4 +429,40 @@ public void testFailed(ExtensionContext context, Throwable cause) {
 
 	}
 
+	/**
+	 * {@link TestWatcher} that retrieves data from the {@link ExtensionContext.Store}.
+	 * @see <a href="https://github.com/junit-team/junit5/issues/3944">#3944</a>
+	 */
+	static class DataRetrievingTestWatcher implements BeforeTestExecutionCallback, TestWatcher {
+
+		private static final Namespace NAMESPACE = Namespace.create(DataRetrievingTestWatcher.class);
+
+		private static final String KEY = "key";
+
+		private static final Map<String, String> results = new HashMap<>();
+
+		@Override
+		public void beforeTestExecution(ExtensionContext context) throws Exception {
+			getStore(context).put(KEY, "enigma");
+		}
+
+		@Override
+		public void testSuccessful(ExtensionContext context) {
+			results.put(KEY, getStore(context).get(KEY, String.class));
+		}
+
+		private static Store getStore(ExtensionContext context) {
+			return context.getStore(NAMESPACE);
+		}
+	}
+
+	@ExtendWith(DataRetrievingTestWatcher.class)
+	static class DataRetrievingTestWatcherTestCase {
+
+		@Test
+		public void test() {
+			//no-op
+		}
+	}
+
 }

platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java

@@ -519,28 +519,35 @@ void closeIsIdempotent() throws Throwable {
 			verifyNoMoreInteractions(closeAction);
 		}
 
+		/**
+		 * @see <a href="https://github.com/junit-team/junit5/issues/3944">#3944</a>
+		 */
 		@Test
-		void rejectsModificationAfterClose() {
+		void acceptsQueryAfterClose() {
+			store.put(namespace, key, value);
 			store.close();
 			assertClosed();
 
-			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.put(namespace, "key1", "value1"));
-			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.remove(namespace, "key1"));
-			assertThrows(NamespacedHierarchicalStoreException.class,
-				() -> store.remove(namespace, "key1", Number.class));
+			assertThat(store.get(namespace, key)).isEqualTo(value);
+			assertThat(store.get(namespace, key, String.class)).isEqualTo(value);
+			assertThat(store.getOrComputeIfAbsent(namespace, key, k -> "new")).isEqualTo(value);
+			assertThat(store.getOrComputeIfAbsent(namespace, key, k -> "new", String.class)).isEqualTo(value);
 		}
 
 		@Test
-		void rejectsQueryAfterClose() {
+		void rejectsModificationAfterClose() {
 			store.close();
 			assertClosed();
 
-			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.get(namespace, "key1"));
-			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.get(namespace, "key1", Integer.class));
+			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.put(namespace, key, value));
+			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.remove(namespace, key));
+			assertThrows(NamespacedHierarchicalStoreException.class, () -> store.remove(namespace, key, int.class));
+
+			// Since key does not exist, an invocation of getOrComputeIfAbsent(...) will attempt to compute a new value.
 			assertThrows(NamespacedHierarchicalStoreException.class,
-				() -> store.getOrComputeIfAbsent(namespace, "key1", k -> "value"));
+				() -> store.getOrComputeIfAbsent(namespace, key, k -> "new"));
 			assertThrows(NamespacedHierarchicalStoreException.class,
-				() -> store.getOrComputeIfAbsent(namespace, "key1", k -> 1337, Integer.class));
+				() -> store.getOrComputeIfAbsent(namespace, key, k -> "new", String.class));
 		}
 
 		private void assertNotClosed() {