spring-projects / spring-framework

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

Automatic registration of EventPublishingTestExecutionListener breaks certain test setups #24116

Closed patrick-peer closed 4 years ago

patrick-peer commented 4 years ago

I found that the changes in issue #23748 break a special test setup of mine (well, I could not find a way to work around this, anyway). As requested I repost my concerns as a new issue here.

The scenario goes like this: I have this Jetty/CXF/Spring server process, which is required to be self contained and start from the command line as a plain old Java application. The setup and tear down of the Spring Context is handled manually within that process, of course. I have an accompanying integration test, which asserts that the startup and shutdown of the server work as intended. I use TestNG and AbstractTestNGSpringContextTests for that. We recently updated Spring, and this particular test now fails, due to the EventPublishingTestExecutionListener accessing the Spring Context in org.springframework.test.context.support.DefaultTestContext.getApplicationContext() after it was closed by the server process.

So, while this enhancement improves a lot of cases I guess, I suggest it also requires a possibility to modify the list of listeners.

sbrannen commented 4 years ago

The scenario goes like this: I have this Jetty/CXF/Spring server process, which is required to be self contained and start from the command line as a plain old Java application. The setup and tear down of the Spring Context is handled manually within that process, of course.

How does that external process gain access to the ApplicationContext managed by the Spring TestContext Framework?

Have you registered a custom TestExecutionListener, and if so, how?

So, while this enhancement improves a lot of cases I guess, I suggest it also requires a possibility to modify the list of listeners.

You can modify the listeners in use via @TestExecutionListeners. Have you tried that?

patrick-peer commented 4 years ago

First of all: Thanks for your (very) fast responses!

How does that external process gain access to the ApplicationContext managed by the Spring TestContext Framework?

Right, I guess you need more context to fully understand my dilemma :). I described an integration test for my highest level bean. In production code I get it from the manually created Spring Context, in the test code, I just autowire it in the test class. This bean starts and stops the jetty server, and unfortunately jetty closes the Spring Context, when it is shut down. However, the correct shutdown behaviour (of the bean) is the very thing I want to test. Sorry for leaving that part out before. I kind of forgot about it and overlooked it myself.

Have you registered a custom TestExecutionListener, and if so, how?

As a matter of fact, I have registered a TestExecutionListener via @TestExecutionListeners, but it has nothing to do with the matter at hand.

You can modify the listeners in use via @TestExecutionListeners. Have you tried that?

I browsed the code for handling @TestExecutionListeners and to me it read like I can only add more listeners, not remove them, once they are set. Am I missing something?

sbrannen commented 4 years ago

I browsed the code for handling @TestExecutionListeners and to me it read like I can only add more listeners, not remove them, once they are set. Am I missing something?

You cannot remove them, but you can override any inherited configuration with a new set of listeners (by setting inheritListeners=false), thereby effectively removing ones that you don't want or need.

That of course may require you to register several default listeners.

Does that meet your needs?

patrick-peer commented 4 years ago

I missed inheritListeners. Yes, this is indeed a workaround for my particular case. Thanks!

Frankly, I would still prefer a more refined approach though, i.e. a possibility to rule out individual listeners. Now, I need to traverse the type hierarchy of my test(s) (including Springs test classes), compile a list of all listeners along the way and declare them again without the one that I want to rule out, just as you mentioned. Also, by doing so, I effectively ignore future changes of the list of default listeners. I could imagine a new method @TestExecutionListeners.blacklist(), which I feel would improve at least my particular setup. This just to further reflect on the matter ;).

The actual implementation of the workaround with inheritListeners was still easy enough, and the problem I described also is contained rather well. So, form my point of view we could close this issue. I leave it up to you though, in case you want to discuss this further.

sbrannen commented 4 years ago

I missed inheritListeners. Yes, this is indeed a workaround for my particular case. Thanks!

👍

Frankly, I would still prefer a more refined approach though, i.e. a possibility to rule out individual listeners.

It would be possible to introduce such a feature, but I feel that might be a different topic.

For your particular use case, your code is effectively violating the contract of the Spring TestContext Framework (TCF). Specifically, an ApplicationContext created by the TCF should only be closed by the TCF itself. This is supported via org.springframework.test.context.TestContext.markApplicationContextDirty(HierarchyMode), which can be invoked from a TestExecutionListener.

Even if your code closes the ApplicationContext manually, it should still be possible to invoke markApplicationContextDirty() to have it properly removed from the TCF internals.

If you manage to do that before the EventPublishingTestExecutionListener kicks in, that would avoid the exception you encountered, since org.springframework.test.context.TestContext.hasApplicationContext() would then return false resulting in no event being fired by the EventPublishingTestExecutionListener.

Unfortunately, I do not know enough about your actual setup, but perhaps you can figure out how to make that happen in your test suite.

Let me know how it goes...

patrick-peer commented 4 years ago

For your particular use case, your code is effectively violating the contract of the Spring TestContext Framework (TCF). Specifically, an ApplicationContext created by the TCF should only be closed by the TCF itself.

I completely agree. Unfortunately I do not have control over Jetty's behavior, regarding the handling of the WebApplicationContext I provide it with.

This is supported via org.springframework.test.context.TestContext.markApplicationContextDirty(HierarchyMode), which can be invoked from a TestExecutionListener.

Even if your code closes the ApplicationContext manually, it should still be possible to invoke markApplicationContextDirty() to have it properly removed from the TCF internals.

If you manage to do that before the EventPublishingTestExecutionListener kicks in, that would avoid the exception you encountered, since org.springframework.test.context.TestContext.hasApplicationContext() would then return false resulting in no event being fired by the EventPublishingTestExecutionListener.

Unfortunately, I do not know enough about your actual setup, but perhaps you can figure out how to make that happen in your test suite.

Let me know how it goes...

public class ContextCloseWorkaround implements TestExecutionListener {
    @Override
    public void afterTestExecution(TestContext testContext) throws Exception {
        testContext.markApplicationContextDirty(HierarchyMode.CURRENT_LEVEL);
    }
}

Thanks a lot for the suggestion! The listener above turns out to make my tests work as intended, without the redundant declaration of the default listeners. I prefer this iteration. :+1:

Cheers!

sbrannen commented 4 years ago

Thanks a lot for the suggestion! The listener above turns out to make my tests work as intended, without the redundant declaration of the default listeners. I prefer this iteration. 👍

Awesome. Glad to hear that solves the problem for you.

In light of that, I am closing this issue.