The test org.springframework.boot.test.mock.mockito.SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget is not idempotent and fail if run twice in the same JVM, because it pollutes some states shared among tests. It may be good to clean this state pollution so that some other tests do not fail in the future due to the shared state polluted by this test.
Detail
Running SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget twice would result in the second run failing for the following error:
Wanted but not invoked:
org.springframework.boot.test.mock.mockito.SpyBeanWithAopProxyTests$DateService.getDate(
false
);
-> at org.springframework.boot.test.mock.mockito.SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget
Actually, there were zero interactions with this mock.
The root cause is that dateService.getDate() is invoked and the result is cached during the first testrun, which is not cleared when the test exits. Therefore, when during the second test run, dateService.getDate() is not called since it's available in the cache, resulting in the above error.
The suggested fix is to clear the cache when the test exits.
With the proposed fix, the test does not pollute the shared state (and passes when run twice in the same JVM).
Comment From: wilkinsona
Thanks for the PR. In what scenario do you expect the test to be run more than once in the same JVM? I'm not too keen on adding code to the test which doesn't serve an obvious purpose and I can't see any obvious reason why the test would run more than once. That makes it hard, just by looking at the code, to understand why the cache is being cleared.
Comment From: lzx404243
You're right that we'd not want to run the same test twice in one JVM, but more tests could be added in the future that would result in flaky order-dependent tests (either fail when run after SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget, or make the test SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget fail).
For instance, in SHA daa3d457b71896a758995c264977bdd1414ee4d4, running org.springframework.boot.logging.java.JavaLoggingSystemTests.withFile before org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests.loggingThatUsesJulIsCaptured results in an error:
git clone https://github.com/spring-projects/spring-boot
cd spring-boot
git checkout daa3d457b71896a758995c264977bdd1414ee4d4
mvn test -Dtest=org.springframework.boot.logging.java.JavaLoggingSystemTests#withFile,org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests#loggingThatUsesJulIsCaptured -pl spring-boot-project/spring-boot
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.springframework.boot.logging.java.JavaLoggingSystemTests
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.193 s - in org.springframework.boot.logging.java.JavaLoggingSystemTests
[INFO] Running org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.418 s <<< FAILURE! - in org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests
[ERROR] loggingThatUsesJulIsCaptured(org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests)
Time elapsed: 0.398 s <<< FAILURE! java.lang.AssertionError:
Expecting:
<"">
to contain:
<"Hello world">
Test org.springframework.boot.logging.java.JavaLoggingSystemTests.withFile fails because org.springframework.boot.logging.log4j2.Log4J2LoggingSystemTests.loggingThatUsesJulIsCaptured pollutes the state shared among tests.
This PR aims to resolve this kind of issue proactively by removing the state pollution in test org.springframework.boot.test.mock.mockito.SpyBeanWithAopProxyTests.verifyShouldUseProxyTarget, so that some other tests do not fail in the future due to the shared state polluted by this test.
Comment From: lzx404243
If you agree that these changes could be good, I have three other patches available which resolve similar issues: 1. https://github.com/lzx404243/spring-boot/pull/1 2. https://github.com/lzx404243/spring-boot/pull/2 3. https://github.com/lzx404243/spring-boot/pull/3
Please let me know your thoughts on these. If you want them, I can open PRs to this repo.
Comment From: wilkinsona
I don't think we should do anything here. There's only one test in SpyBeanWithAopProxyTests at the moment and its @Configuration class isn't shared by any other tests classes. As such, there's currently no chance that the test will be run twice in the same JVM. With that in mind, I find the code that clears the cache to be confusing as it doesn't need to be there and anyone looking at it in the future will wonder why it's there.
If we ever add another test to SpyBeanWithAopProxyTests the problem caused by the second test will quickly become apparent and we can solve it at that time. Thanks anyway.