Skip to content

Close contexts when clearing test context cache #36825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Will-thom wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −1
Open

Close contexts when clearing test context cache #36825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion spring-test/src/main/java/org/springframework/test/context/cache/ContextCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ default int getContextUsageCount() {
void reset();

/**
* Clear all contexts from the cache, clearing context hierarchy information as well.
* Clear all contexts from the cache, explicitly
* {@linkplain org.springframework.context.ConfigurableApplicationContext#close() closing}
* each context that is an instance of {@code ConfigurableApplicationContext}
* and clearing context hierarchy information as well.
*/
void clear();

Expand Down
3 changes: 3 additions & 0 deletions spring-test/src/main/java/org/springframework/test/context/cache/DefaultContextCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,9 @@ public void reset() {
@Override
public void clear() {
synchronized (this.contextMap) {
for (MergedContextConfiguration key : new ArrayList<>(this.contextMap.keySet())) {
remove(key, HierarchyMode.CURRENT_LEVEL);
}
this.contextMap.clear();

@sbrannen sbrannen Aug 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this new loop, the four subsequent calls to this.contextMap.clear(), this.hierarchyMap.clear(), this.contextUsageMap.clear(), and this.unusedContexts.clear() are now redundant: the loop's calls to remove() should already empty all four as an invariant, since every key in the pre-loop snapshot ends up removed either directly or as a descendant.

Could you please remove those clear() calls and ensure the tests cover those invariants (see other comment regarding tests)?

this.hierarchyMap.clear();
this.contextUsageMap.clear();
Expand Down
36 changes: 36 additions & 0 deletions spring-test/src/test/java/org/springframework/test/context/cache/LruContextCacheTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,42 @@ void maxCacheSizeZero() {
assertThatIllegalArgumentException().isThrownBy(() -> new DefaultContextCache(0));
}

@Test
void clearClosesContexts() {
DefaultContextCache cache = new DefaultContextCache(4);

cache.put(fooConfig, key -> fooContext);
cache.put(barConfig, key -> barContext);
cache.put(bazConfig, key -> bazContext);
assertCacheContents(cache, "Foo", "Bar", "Baz");

cache.clear();
assertCacheContents(cache);

verify(fooContext, times(1)).close();
verify(barContext, times(1)).close();
verify(bazContext, times(1)).close();
verify(abcContext, never()).close();
}

@Test
void resetClosesContexts() {
DefaultContextCache cache = new DefaultContextCache(4);

cache.put(fooConfig, key -> fooContext);
cache.put(barConfig, key -> barContext);
cache.get(fooConfig);
assertThat(cache.getHitCount()).isEqualTo(1);
assertCacheContents(cache, "Bar", "Foo");

cache.reset();
assertCacheContents(cache);
assertThat(cache.getHitCount()).isZero();

verify(fooContext, times(1)).close();
verify(barContext, times(1)).close();
}

@sbrannen sbrannen Aug 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of things I'd like addressed on the test side before merging:

  1. Hierarchy coverage. These two new tests only cover flat, non-hierarchical configs (config() always passes a null parent). Since this change goes through the hierarchy-aware remove() path, could you please add a test that builds an actual multi-level @ContextHierarchy (similar to the removeContextHierarchyCacheLevel*() tests in ContextCacheTests) and asserts that clear()/reset() closes every level correctly?

  2. Invariant coverage. Neither of the new tests verifies that hierarchyMap and contextUsageMap actually end up empty — only contextMap is checked, via assertCacheContents(). Since those two structures are exactly what this change touches inside remove(), please also assert getParentContextCount() and getContextUsageCount() return to 0 after clear()/reset() – for example, using the existing assertContextCacheStatistics() / assertParentContextCount() helpers already used in ContextCacheTests. For resetClosesContexts(), it'd also be good to assert getMissCount() is 0 for symmetry with the existing getHitCount() check.

This would give us a real regression guard on these new code paths, rather than only checking that the mock contexts were closed.

As side note, I think using Mockito.inOrder() for the verification would also serve us well to ensure the contexts are closed in the expected order within a context hierarchy.

However, inOrder should only assert order along a single ancestor → descendant chain (e.g. child → parent). It shouldn't assert anything about the relative order of sibling contexts, since children of the same parent are stored in a HashSet and closed in unspecified order. In other words, asserting sibling order would result in a flaky test.



@Nested
@SuppressWarnings("deprecation")
Expand Down