From 3e2cc6cac1151ced3286cf0039123dd577e57f12 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 13 Sep 2024 16:11:28 +0200 Subject: [PATCH] Ensure TestWatchers can access data in the ExtensionContext.Store Changes made in conjunction with #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: #3614 Fixes: #3944 --- .../release-notes/release-notes-5.11.1.adoc | 7 ++- .../store/NamespacedHierarchicalStore.java | 9 ++-- .../engine/extension/TestWatcherTests.java | 51 +++++++++++++++++++ .../NamespacedHierarchicalStoreTests.java | 27 ++++++---- 4 files changed, 78 insertions(+), 16 deletions(-) diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.1.adoc index 80f58d56d747..41114a7b0230 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.11.1.adoc +++ b/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 diff --git a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java b/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java index 965eb9b24439..79836ac6f501 100644 --- a/junit-platform-engine/src/main/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStore.java +++ b/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 get(N namespace, Object key, Class requiredType) throws NamespacedHierarchicalStoreException { - rejectIfClosed(); Object value = get(namespace, key); return castToRequiredType(key, value, requiredType); } @@ -174,13 +172,15 @@ public T get(N namespace, Object key, Class requiredType) throws Namespac * closed */ public Object getOrComputeIfAbsent(N namespace, K key, Function defaultCreator) { - rejectIfClosed(); Preconditions.notNull(defaultCreator, "defaultCreator must not be null"); CompositeKey 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 Object getOrComputeIfAbsent(N namespace, K key, Function def public V getOrComputeIfAbsent(N namespace, K key, Function defaultCreator, Class requiredType) throws NamespacedHierarchicalStoreException { - rejectIfClosed(); Object value = getOrComputeIfAbsent(namespace, key, defaultCreator); return castToRequiredType(key, value, requiredType); } diff --git a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TestWatcherTests.java b/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TestWatcherTests.java index 27ca3d2d77c9..a8400aa63ba3 100644 --- a/jupiter-tests/src/test/java/org/junit/jupiter/engine/extension/TestWatcherTests.java +++ b/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 #3944 + */ + 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 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 + } + } + } diff --git a/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java b/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java index 9f18083a924c..f234ff11dbe3 100644 --- a/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java +++ b/platform-tests/src/test/java/org/junit/platform/engine/support/store/NamespacedHierarchicalStoreTests.java @@ -519,28 +519,35 @@ void closeIsIdempotent() throws Throwable { verifyNoMoreInteractions(closeAction); } + /** + * @see #3944 + */ @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() {