spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.21k stars 37.97k forks source link

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

Open StefanPenndorf opened 3 years ago

StefanPenndorf commented 3 years ago

When using @SpringBootTest with JUnit Jupiter a pitest user had a problem when executing tests with pitest (see https://github.com/hcoles/pitest/issues/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.

sbrannen commented 3 years ago

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.

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.

markusheiden commented 2 years ago

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 commented 1 year ago

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:

sbrannen commented 3 months ago

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.