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.

Sign up for GitHub

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

Jump to bottom

@RecordApplicationEvents should record events from spawned threads, too #29827

Closed
odrotbohm opened this issue Jan 16, 2023 · 4 comments · Fixed by #30020
Closed

@RecordApplicationEvents should record events from spawned threads, too #29827

odrotbohm opened this issue Jan 16, 2023 · 4 comments · Fixed by #30020
Assignees
@simonbasle
Labels
in: test Issues in the test module status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@odrotbohm
Copy link
Member

The infrastructure behind @RecordApplicationEvents currently uses a ThreadLocal to capture application events published by the current thread. Unfortunately, this rules out events published by asynchronous event listeners in turn. It would be nice if events would be captured that are published on threads spawned by the current execution thread.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 16, 2023
@sbrannen sbrannen added in: test Issues in the test module type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 16, 2023
@sbrannen sbrannen self-assigned this Jan 16, 2023
@sbrannen sbrannen added this to the 6.0.5 milestone Jan 16, 2023
@sbrannen
Copy link
Member

Related Issues

@sbrannen sbrannen mentioned this issue Jan 31, 2023
Closed
@sbrannen sbrannen assigned simonbasle and unassigned sbrannen Feb 10, 2023
@simonbasle simonbasle added for: team-attention status: pending-design-work Needs design work before any code can be developed labels Feb 13, 2023
@simonbasle simonbasle modified the milestones: 6.0.5, 6.1.x Feb 13, 2023
@simonbasle
Copy link
Contributor

The model falls apart for concurrent tests, and most notably for tests cases that instantiate the Test Class once. Typically, JUnit5 tests with @TestInstance(Lifecycle.PER_CLASS) (and TestNG as well if I understand correctly).

To be clear, turning the ApplicationEventsHolder ThreadLocal into an InheritableThreadLocal breaks the above case (as demonstrated by ParallelApplicationEventsIntegrationTests).

Unlike in Spring Modulith, we can't really assume that tests are not parallelized (or at least parallelized in a way that events collection won't get polluted), so we have to chose between:

  1. support concurrent tests / Lifecycle.PER_CLASS
  2. support asynchronously published events and events assertions performed in a separate thread (like with Awaitility)

The @Autowired support is also problematic, because for PER_CLASS the ApplicationEvents is injected once and cannot have a dedicated instance per test method.

This can't be solved in time for 6.0.5, and in fact I'm not sure this can be solved at all without some breaking change / behavior (and as such 6.1.0 would potentially be a better candidate). Removing from milestone and marking for further team discussion / design.

@simonbasle simonbasle removed their assignment Feb 13, 2023
@simonbasle simonbasle self-assigned this Feb 23, 2023
@simonbasle
Copy link
Contributor

I've made quite the number of local iterations on this one, and I think the minimum viable implementation is one that:

  1. have SpringExtension detect and reject test cases annotated with @RecordApplicationEvents and @TestInstance(Lifecycle.PER_CLASS) and @Execution(ExecutionMode.CONCURRENT)
  2. have ApplicationEventsHolder use an InheritableThreadLocal<ApplicationEvents>

I've tried various other configurations, none of which really alleviate the PER_CLASS caveat. These other ideas included: storing an ApplicationEvents in the TestContext, attempting to register one ApplicationEventApplicationListener per test and to trigger autowiring again, filtering captured events in the listener if a different ApplicationEvents could be found in the holder vs the one with which the event listener was constructed...

In the end, the InheritableThreadLocal seems to have the same impact with the minimum of code.

@simonbasle simonbasle mentioned this issue Feb 23, 2023
Merged
@simonbasle
Copy link
Contributor

Superseded by gh-30020

@simonbasle simonbasle added status: superseded An issue that has been superseded by another and removed status: pending-design-work Needs design work before any code can be developed labels May 5, 2023
@simonbasle simonbasle modified the milestones: 6.1.x, 6.1.0-M1 May 5, 2023
simonbasle added a commit to simonbasle/spring-framework that referenced this issue May 5, 2023
@simonbasle
Add test support to record async events, with Junit5 caveat
f269a80 
This commit modifies the way the `@RecordApplicationEvents` annotation
works in tests, allowing for capture of events from threads other than
the main test thread (async events) and for the assertion of captured
event from a separate thread (e.g. when using `Awaitility`).

This is done by switching the `ApplicationEventsHolder` to use an
`InheritedThreadLocal`.

There is a mutual exclusion between support of asynchronous events vs
support of JUnit5 parallel tests with the `@TestInstance(PER_CLASS)`
mode. As a result, we favor the former and now `SpringExtension` will
invalidate a test class that is annotated (or meta-annotated, or
enclosed-annotated) with `@RecordApplicationEvents` AND
`@TestInstance(PER_CLASS)` AND `@Execution(CONCURRENT)`.

See spring-projectsgh-29827
Closes spring-projectsgh-30020
simonbasle added a commit that referenced this issue May 5, 2023
@simonbasle
Add test support to record async events, with Junit5 caveat
b39e93d 
This commit modifies the way the `@RecordApplicationEvents` annotation
works in tests, allowing for capture of events from threads other than
the main test thread (async events) and for the assertion of captured
event from a separate thread (e.g. when using `Awaitility`).

This is done by switching the `ApplicationEventsHolder` to use an
`InheritedThreadLocal`.

There is a mutual exclusion between support of asynchronous events vs
support of JUnit5 parallel tests with the `@TestInstance(PER_CLASS)`
mode. As a result, we favor the former and now `SpringExtension` will
invalidate a test class that is annotated (or meta-annotated, or
enclosed-annotated) with `@RecordApplicationEvents` AND
`@TestInstance(PER_CLASS)` AND `@Execution(CONCURRENT)`.

See gh-29827
Closes gh-30020
@sbrannen sbrannen removed this from the 6.1.0-M1 milestone May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@simonbasle simonbasle

Labels
in: test Issues in the test module status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add test support to record async events, with JUnit Jupiter caveat simonbasle/spring-framework
4 participants
@sbrannen @odrotbohm @simonbasle @spring-projects-issues