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

Publish test execution events to the test ApplicationContext [SPR-13916] #18490

Closed
5 tasks done
spring-projects-issues opened this issue Feb 4, 2016 · 21 comments
Closed
5 tasks done
Assignees
Labels
in: test Issues in the test module type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Feb 4, 2016

Frank Scheffler opened SPR-13916 and commented

We are using Mockito mocks quite often throughout our Spring-based integration tests, which leaves us with the problem of resetting them before/after each test method. While we can use some globally defined custom TestExecutionListener to reset all the mocks found within the ApplicationContext, it would be more practical to be able to define specific event-listeners within the application context itself. This would allow for more fine-grained control over which beans (i.e. mocks) should be reset.

Deliverables

  • Implement EventPublishingTestExecutionListener.
  • Implement abstract TestContextEvent and all concrete subclasses.
  • Implement composed @EventListener annotations for all events.
  • Document exception handling and async support in Javadoc.
  • Document in the Testing chapter of the reference manual.

Issue Links:

2 votes, 6 watchers

@spring-projects-issues
Copy link
Collaborator Author

Frank Scheffler commented

In fact, one solution would be to provide an additional EventPublishingTestExecutionListener, so users could plug-in the event publishing behavior, if they are in need for it.

TestExecutionListener could look like this, backed by an event class hierarchy reflecting the testcontext lifecycle:

public class EventPublishingTestExecutionListener extends AbstractTestExecutionListener {

  @Override
  public void afterTestMethod(TestContext testContext) throws Exception {
    testContext.getApplicationContext().publishEvent(new AfterMethodTestExecutionEvent(testContext));
  }
  
}

Within the test configuration code you could then, for instance, reset the mocks in question, e.g. by:

@Configuration
public class MyMockConfiguration implements ApplicationListener<AfterMethodTestExecutionEvent> {

   @Bean
   public MyService myService() {
      return Mockito.mock(MyService.class);
   }

   @Override
    public void onApplicationEvent(AfterMethodTestExecutionEvent event) {
      Mockito.reset(myService());
    }
}

@spring-projects-issues
Copy link
Collaborator Author

Frank Scheffler commented

I have created an implementation draft for further discussions at: maverick1601@f07c34c

The tests work fine and I added EventListener-based meta-annotation to simplify integration into configuration classes and beans, e.g. simply annotating methods with @BeforeTestMethod, which is compliant with JUnit life-cycle annotations.

I am, however, currently unsure about the following:

  1. What order should the ApplicationEventTestExecutionListener have?
  2. Should it be added to the list of default test execution listeners, as i did?
  3. Is the eager initialization of the ApplicationContext as part of the listener a problem or not?

Finally I would like to summarize the benefits of this approach and why we need it:

  1. Using ApplicationEvents for signaling the test life-cycle enables us to interact with the test framework from inside our Spring configuration classes.
  2. We can therefore more concisely prepare or clean up mocks that are beans within the application context. This is inherited behavior for all test cases using such configurations, as compared to copying standard @Before methods around.
  3. Event consumption is much more flexible with respect to profiles etc., while TestExecutionListeners have to detect, what needs to be done based on the TestContext only.
  4. Moreover, we will be able to reduce the amount of TestExecutionListeners in our case, because the logic can be refactored into event-consuming methods.
  5. Being framework agnostic enables us to integrate this with FitNesse and other testing frameworks more easily.

@spring-projects-issues
Copy link
Collaborator Author

Frank Scheffler commented

Any feedback on this or the related question on Google groups (https://groups.google.com/forum/#!topic/spring-framework-contrib/crdJWZk3Wcs). I would like to proceed on finishing the code, if this is considered a useful enhancement to the TCF.

@spring-projects-issues
Copy link
Collaborator Author

Sam Brannen commented

Frank Scheffler,

Thanks for the proposal and draft implementation.

At a quick glance, it looks reasonably sound, though I haven't yet had time to look at it in great detail.

With regard to the automatic registration of ApplicationEventTestExecutionListener, I'm not so sure that's a good idea, especially if the ApplicationContext is always eagerly initialized. Most listeners (except DITEL) tend not to eagerly load a context (or do anything for that matter) if unnecessary. In other words, we wouldn't want to attempt to fire events in a context that will never exist for a given test class.

I'll move this issue to the 4.3 backlog with the hope of finding more time to devote to it as we move toward 4.3 GA.

Regards,

Sam

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Feb 17, 2016

Sam Brannen commented

Please note that this issue is very closely related to and potentially supersedes #13352.

@spring-projects-issues
Copy link
Collaborator Author

Phil Webb commented

You might be interested in spring-projects/spring-boot#5042 which is looking to add better Mockito support to Spring Boot.

@spring-projects-issues
Copy link
Collaborator Author

Frank Scheffler commented

We have implemented this at a customer and enhanced the listener to publish events to the fixture, i.e. the test class, itself as well, if implemented as ApplicationListener or via @EventListener (meta)-annotations. This improves test framework agnosticism, making it easier for us to switch test code from JUnit to TestNG or FitNesse (we have implemented our own Spring-Test integration for that one), later on, because we rely solely on our own meta-annotations instead of JUnit-specific ones.

@sbrannen
Copy link
Member

This issue supersedes gh-13352.

@sbrannen sbrannen changed the title Publish TestExecutionListener events to the ApplicationContext under test [SPR-13916] Publish TestExecutionListener events to the test ApplicationContext [SPR-13916] Feb 23, 2019
@sbrannen sbrannen modified the milestones: General Backlog, 5.1.6, 5.2 M1 Feb 23, 2019
@sbrannen
Copy link
Member

Related discussion on StackOverflow

@sbrannen
Copy link
Member

Hi @maverick1601,

I would like to proceed on finishing the code, if this is considered a useful enhancement to the TCF.

I took a glance at your draft, and it looks very similar to solutions I've implemented for clients.

Are you still interested in submitting a PR?

If so, feel free to simply submit your draft, and I'll take it from there. Otherwise, I'll likely just implement it from scratch.

I'm hoping to get this into Spring 5.2 M1, so I'd be grateful for prompt feedback.

Thanks,

Sam

@sbrannen sbrannen changed the title Publish TestExecutionListener events to the test ApplicationContext [SPR-13916] Publish test execution events to the test ApplicationContext [SPR-13916] Feb 23, 2019
@maverick1601
Copy link
Contributor

I would do so. Since the issue is about 3 yrs old, I don't know how much I would have to rewrite. Back then we could not fully agree on the fact, that such a TestExecutionListener would be included in the list of default listeners, especially with respect to the fact that such an event publishing TestExecutionListener would trigger context initialization. What is your opinion on that?

@sbrannen
Copy link
Member

I would do so.

I like to give credit where credit is due, so that would be great!

Can you provide an estimate of when you think you'll able to do that?

Since the issue is about 3 yrs old, I don't know how much I would have to rewrite.

I don't think you'd have to rewrite much of anything. There are two new methods in the TestExecutionListener API, but I assume you'll make quick work of that. If you don't have time, don't worry: I can add the support for the new methods.

Back then we could not fully agree on the fact, that such a TestExecutionListener would be included in the list of default listeners, especially with respect to the fact that such an event publishing TestExecutionListener would trigger context initialization. What is your opinion on that?

I'm currently inclined not to register this new listener by default. I think it's perhaps best to keep it as an opt-in feature.

In any case, that can be decided later or changed before 5.2 GA is released.

My main objective right now is to get the feature in place in 5.2 M1 so that people can experiment with it and provide feedback.

Cheers,

Sam

p.s. don't worry so much about the documentation at this point unless you have time. If you don't have time, I'll pick up the documentation task after merging in your work.

@maverick1601
Copy link
Contributor

Okay, I took a look at it and it seems the main work is to rebase my previous proposal against master branch and to cover those two new life-cycle methods. I'd say this is done by the end of this week.

@sbrannen
Copy link
Member

Sounds good.

Looking forward to your PR!

@maverick1601
Copy link
Contributor

@sbrannen See: #22477

@sbrannen
Copy link
Member

@maverick1601, thanks for the PR.

I'll take a look.

maverick1601 added a commit to maverick1601/spring-framework that referenced this issue Feb 28, 2019
Adds new TestExecutionListener for publishing events to the test application context.
These may be consumed by @eventlistener annotated methods to react to the test
life-cycle. The test execution listener isn't enabled by default.

Implements spring-projects#18490
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 1, 2019
@sbrannen
Copy link
Member

sbrannen commented Mar 1, 2019

The work for this issue has been merged into master in 34fee86 and refined in 2d6624d.

Reopening in order to add documentation to the Testing chapter of the reference manual. See new Deliverables section of this issue.

@sbrannen sbrannen reopened this Mar 1, 2019
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Mar 1, 2019
sbrannen added a commit that referenced this issue Mar 2, 2019
This commit introduces support for SpEL expressions for conditional
event processing in annotations such as @BeforeTestClass,
@BeforeTestMethod, etc.

This commit also introduces a new getTestContext() method in
TestContextEvent in order to improve the readability of such SpEL
expressions.

See gh-18490
@sbrannen
Copy link
Member

sbrannen commented Mar 2, 2019

Update: commit e272e2e introduced support for conditional event processing in the built-in event listener annotations.

sbrannen added a commit that referenced this issue Mar 3, 2019
This commit introduces tests for both synchronous and asynchronous
exception handling for TestContext event annotations.

See gh-18490
sbrannen added a commit that referenced this issue Apr 2, 2019
This commit updates the class-level Javadoc for
EventPublishingTestExecutionListener in order to provide explicit
documentation for exception handling and async support.

See gh-18490
@sbrannen
Copy link
Member

sbrannen commented Apr 3, 2019

Update:

This new feature set is now documented in the Reference Manual.

In addition, the EventPublishingTestExecutionListener is now registered by default for 5.2 M1.

bclozel added a commit that referenced this issue Apr 4, 2019
sbrannen added a commit that referenced this issue Apr 4, 2019
@sbrannen
Copy link
Member

sbrannen commented Apr 4, 2019

Reopening to sort out issues with having the new EventPublishingTestExecutionListener registered by default and having an order of HIGHEST_PRECEDENCE.

The current state of affairs leads to undesirable side effects, such as:

  • @DirtiesContext no longer works as expected: a context that is dirtied (and properly closed) after a test method or test class will be magically recreated by the EventPublishingTestExecutionListener, thus effectively breaking @DirtiesContext support.
  • The test's ApplicationContext will be eagerly loaded before any other TestExecutionListener and before any before all lifecycle methods in test classes (e.g., JUnit Jupiter's @BeforeAll and JUnit 4's @BeforeClass).

@sbrannen
Copy link
Member

sbrannen commented Apr 6, 2019

Update: the implementation of this feature has been revised. See 353e092 for details.

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

3 participants