Skip to content
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.

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

Close all ApplicationContexts in the TestContext framework after all tests have been executed #26196

Open
StefanPenndorf opened this issue Dec 1, 2020 · 5 comments
Labels
in: test Issues in the test module type: enhancement A general enhancement

Comments

@StefanPenndorf
Copy link
Contributor

When using @SpringBootTest with JUnit Jupiter a pitest user had a problem when executing tests with pitest (see hcoles/pitest#827). As far as my analysis brought me the cause is Spring Frameworks TestContext caching.
Spring caches all application contexts loaded in a static variable and reused them among different tests and test classes (TestContextManager). Unfortunately Spring does not cleanup after all(!) tests have been run assuming the JVM will be terminated immediately after tests have been executed (which in turn triggers the automatic closing behavior for application contexts). This might hold for surefire or IDE starting those tests. But Pitest reuses the JVM instance for different test suite executions (multiple invocations of JUnit Jupiter's Launcher.execute() inside the same JVM) . Thus the cached application contexts stay there and interfere with future test runs.

On the other hand Testcontainers integrates with the JUnit Jupiter Engine and uses ClosableResource hook to cleanup containers afterwards.

I've created a JUnit extension cleaning up application contexts on shutdown as a proof of concept. See SpringBootCleanup in the sample project originally provided by @jaguado-arima .

@DirtiesContext will have the same effect here and I didn't notice any differences in performance regarding this simple example. But I think it's a bad idea to add @DirtiesContext to all test classes only to be able to use pitest... if you're relying on application context caching to reduce overall testing time for a huge test suite adding @DirtiesContext to all tests for the sake of mutation testing will slow down test execution (Springs context cache has been implemented for a reason...).

@sbrannen If you consider changing the behavior as suggested I can also provide a pull request implementing the necessary changes for SpringExtension.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 1, 2020
@sbrannen sbrannen added in: test Issues in the test module type: enhancement A general enhancement labels Dec 2, 2020
@sbrannen sbrannen changed the title Properly cleanup ApplicationContexts in TestContext when all tests have been executed Close all ApplicationContexts in TestContext after all tests have been executed Dec 2, 2020
@sbrannen sbrannen changed the title Close all ApplicationContexts in TestContext after all tests have been executed Close all ApplicationContexts in the TestContext framework after all tests have been executed Dec 2, 2020
@sbrannen
Copy link
Member

sbrannen commented Dec 2, 2020

But Pitest reuses the JVM instance for different test suite executions (multiple invocations of JUnit Jupiter's Launcher.execute() inside the same JVM) . Thus the cached application contexts stay there and interfere with future test runs.

That's an interesting use case, and we certainly don't have any features in place to support it.

I think it's a bad idea to add @DirtiesContext to all test classes only to be able to use pitest...

I agree: one should definitely not annotate all test classes with @DirtiesContext as a workaround for this use case.

@sbrannen If you consider changing the behavior as suggested I can also provide a pull request implementing the necessary changes for SpringExtension.

I took a look at the proposed SpringBootCleanup extension for JUnit Jupiter. It's an interesting approach for solving the problem, but I see a few issues with it.

  • It's specific to JUnit Jupiter; whereas, core features in the Spring TestContext Framework are agnostic of the underlying testing framework.
  • By invoking testContext.getApplicationContext() with the goal of closing it, the code may in fact restart an ApplicationContext that has already been closed -- either via @DirtiesContext or automatic eviction from the context cache.
  • It creates a CloseableResource for each class-level TestContext instance.

Ideally, there would be a general purpose mechanism for closing all contexts in the cache.

The org.springframework.test.context.cache.ContextCache.clear() API already exists, but the current implementation in DefaultContextCache does not actually close the contexts. So we may be able to improve that. If we do that, however, we'd still need a way to clear the ContextCache in an extension such as the proposed SpringBootCleanup extension.

@sbrannen sbrannen self-assigned this Dec 16, 2020
@markusheiden
Copy link
Contributor

Has there been any progress on this issue?

We are using RedisMessageListenerContainers that do not like Redis being closed prematurely and thus spam the log.

@heowc
Copy link
Contributor

heowc commented Mar 16, 2023

I also wonder if there is any progress on this issue. 🤔
I tried to recycle the ApplicationContext during @SpringBootTest, but because some @MockBean was used, multiple ApplicationContexts were created. So I adjusted the DefaultContextCache option (spring.test.context.cache.maxSize) to keep the ApplicationContext to a minimum, but it still exists in heap memory.

This also caused the following issues:

@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 29, 2023
@snicoll snicoll added this to the 6.2.x milestone Dec 29, 2023
@sbrannen sbrannen modified the milestones: 6.2.x, 6.2.0-M1, 6.2.0-M2 Mar 18, 2024
@sbrannen sbrannen modified the milestones: 6.2.0-M2, 6.2.x May 12, 2024
@sbrannen
Copy link
Member

For anyone relying on the aforementioned SpringBootCleanup extension, here is a simplified implementation of the CloseableResource, which will not accidentally load an ApplicationContext which has already been closed.

record ContextClosingResource(TestContextManager testContextManager) implements CloseableResource {

	@Override
	public void close() {
		TestContext testContext = this.testContextManager.getTestContext();
		if (testContext.hasApplicationContext()) {
			testContext.markApplicationContextDirty(HierarchyMode.EXHAUSTIVE);
		}
	}
}

Note that there is also no need to manually close the ApplicationContext, since markApplicationContextDirty(...) already does that.

@sbrannen sbrannen modified the milestones: 6.2.x, General Backlog Sep 8, 2024
@sbrannen sbrannen removed their assignment Dec 3, 2024
@Chasson1992
Copy link

Has there been any progress on this issue?

We are using RedisMessageListenerContainers that do not like Redis being closed prematurely and thus spam the log.

I wanted to bump this ticket. I'm also looking to solution to a similar problem with testcontainers, where the Spring context is not closed before the testcontainers are shutdown which spams our log output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants