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

DirtiesContextTestExecutionListener has unexpected execution order leading to errors #34225

Closed
tfactor2 opened this issue Jan 9, 2025 · 7 comments
Labels
in: test Issues in the test module status: invalid An issue that we don't feel is valid

Comments

@tfactor2
Copy link

tfactor2 commented Jan 9, 2025

We encountered an issue where our custom test execution listener, responsible for cleaning the database after-test, was executed after the ApplicationContext was cleaned up. We had set the listener order to 3000 to prevent conflicts with other internal listeners.

However, we didn't know that DirtiesContextTestExecutionListener had the same order level (3000), leading our cleanup listener to run after the context was marked dirty.
This behavior led to unexpected results - Spring initiated a new context since the previous one was removed, resulting in the database being cleaned for another test.

Additionally, it would be great to understand the rationale behind 3000 for DirtiesContextTestExecutionListener.
Looks like the logic should be more complex:
after the method marking should be executed last (having listener with the lowest order value),
before the method marking should be executed first (listener with the highest order value).

Spring Version: 6.1.13

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 9, 2025
@snicoll snicoll added the in: test Issues in the test module label Jan 9, 2025
remeio added a commit to remeio/spring-framework that referenced this issue Jan 15, 2025
@remeio
Copy link
Contributor

remeio commented Jan 15, 2025

Hi, You can find the TestExecutionListener subclasses at file spring-test/src/main/resources/META-INF/spring.factories:

# Default TestExecutionListeners for the Spring TestContext Framework
#
org.springframework.test.context.TestExecutionListener = ...

and find this orders:

  • 1000: org.springframework.test.context.web.ServletTestExecutionListener
  • 1500: org.springframework.test.context.support.DirtiesContextBeforeModesTestExecutionListener
  • 1800: org.springframework.test.context.event.ApplicationEventsTestExecutionListener
  • 1950: org.springframework.test.context.bean.override.BeanOverrideTestExecutionListener
  • 2000: org.springframework.test.context.support.DependencyInjectionTestExecutionListener
  • 2500: org.springframework.test.context.observation.MicrometerObservationRegistryTestExecutionListener
  • 3000: org.springframework.test.context.support.DirtiesContextTestExecutionListener
  • 3005: org.springframework.test.context.support.CommonCachesTestExecutionListener
  • 4000: org.springframework.test.context.transaction.TransactionalTestExecutionListener
  • 5000: org.springframework.test.context.jdbc.SqlScriptsTestExecutionListener
  • 10000: org.springframework.test.context.event.EventPublishingTestExecutionListener
  • Ordered.LOWEST_PRECEDENCE - 100: org.springframework.test.context.bean.override.mockito.MockitoResetTestExecutionListener

At design, I thinks this order magic values use to determine the relative order of TestExecutionListener subclasses, but spring isn't support the relative order(See more at #33595 ), it just have the order value (a int value).

At use, If you want add a new TestExecutionListener subclass which:

  • before CommonCachesTestExecutionListener
  • and after DirtiesContextTestExecutionListener

You can set the order like 3001 (3000 < order < 3005), it will work like the relative order.

Finally, I think It's necessary to add the comment for the method XXXTestExecutionListener#getOrder, otherwise we will face too much magic value. Actually, BeanOverrideTestExecutionListener already has a nice comment:

/**
 * Returns {@code 1950}, which ensures that the {@code BeanOverrideTestExecutionListener}
 * is ordered after the
 * {@link org.springframework.test.context.support.DirtiesContextBeforeModesTestExecutionListener
 * DirtiesContextBeforeModesTestExecutionListener} and just before the
 * {@link DependencyInjectionTestExecutionListener}.
 */
@Override
public int getOrder() {
  return 1950;
}

@remeio

This comment has been minimized.

@sbrannen sbrannen added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 18, 2025
@sbrannen sbrannen changed the title DirtiesContextTestExecutionListener has unexpected execution order leading to errors DirtiesContextTestExecutionListener has unexpected execution order leading to errors Jan 18, 2025
@sbrannen
Copy link
Member

Hi @tfactor2,

I don't understand the basis for your claim that "DirtiesContextTestExecutionListener has an unexpected execution order."

All TestExecutionListener implementations have a documented order since the ordering support was introduced in Spring Framework 4.1. See #16092 and commit e6d1614.

The Ordering TestExecutionListener Implementations section of the reference manual also states the following.

AbstractTestExecutionListener and all default TestExecutionListener implementations provided by Spring implement Ordered with appropriate values. Third-party frameworks and developers should therefore make sure that their default TestExecutionListener implementations are registered in the proper order by implementing Ordered or declaring @Order. See the javadoc for the getOrder() methods of the core default TestExecutionListener implementations for details on what values are assigned to each core listener.

In light of the above, I am closing this issue as "works as designed".

Regards,

Sam

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Jan 18, 2025
@sbrannen
Copy link
Member

sbrannen commented Jan 18, 2025

Hi @remeio,

Finally, I think It's necessary to add the comment for the method XXXTestExecutionListener#getOrder, otherwise we will face too much magic value.

Indeed, that would be useful, and I will process PR #34265 that you submitted. 👍

Actually, BeanOverrideTestExecutionListener already has a nice comment:

Glad you like it! That was the last TestExecutionListener I documented, and you're right: it sets a good precedent regarding explaining the order value.


but spring isn't support the relative order (See more at #33595), it just have the order value (a int value).

FYI: That issue is related to aspect ordering (AOP) and is unrelated to TestExecutionListener ordering.

@nizametdinovcrx
Copy link

@sbrannen, thanks for pointing that out.
A few things can be improved though, in my opinion:

  1. Add a few statements about order value interpretation for after methods:
    org.springframework.test.context.TestContextManager.afterTestExecution:
public void afterTestExecution(Object testInstance, Method testMethod, @Nullable Throwable exception)
....
    // Traverse the TestExecutionListeners in reverse order to ensure proper
   // "wrapper"-style execution of listeners.
  for (TestExecutionListener testExecutionListener : getReversedTestExecutionListeners()) {
  1. Expose the order of system listeners as constants - then application developers can refer to these values in their getOrder implementations and calculate order dynamically (DirtiesContextBeforeModesTestExecutionListener.ORDER + 10, for example).

  2. "Reserve" some ranges for system listeners (the documentation should be enough, I think). For example, "system listeners should reside in the range 0-10000 and Ordered.LOWEST_PRECEDENCE - 10000. If possible, use the rest of the range to avoid breaking system functionality".

sbrannen added a commit that referenced this issue Feb 11, 2025
…ants

Prior to this commit, the order values of TestExecutionListener
implementations were hard-coded in their getOrder() methods.

To benefit users and integrators, this commit exposes those order values
as an ORDER constant in each TestExecutionListener.

See gh-34225
Closes gh-34404
@sbrannen
Copy link
Member

Thanks for the proposals, @nizametdinovcrx.

Feedback

  1. Add a few statements about order value interpretation for after methods:

I partially addressed that in commit 9d3374b, and I have raised #34422 to further improve the documentation.

  1. Expose the order of system listeners as constants - then application developers can refer to these values in their getOrder implementations and calculate order dynamically (DirtiesContextBeforeModesTestExecutionListener.ORDER + 10, for example).

I addressed that in #34404.

  1. "Reserve" some ranges for system listeners (the documentation should be enough, I think). For example, "system listeners should reside in the range 0-10000 and Ordered.LOWEST_PRECEDENCE - 10000. If possible, use the rest of the range to avoid breaking system functionality".

MockitoResetTestExecutionListener already has a published order value of Ordered.LOWEST_PRECEDENCE - 100. So, Ordered.LOWEST_PRECEDENCE - 10000 wouldn't be an option at this point.

In any case, in the future we may have our own valid reasons to introduce a core "system" listener that does not have an order value in one of those ranges. In light of that, I don't think it warrants attempting to define a reserved range. Furthermore, those ranges are not actually reserved for core listeners, and third parties are actually encouraged to introduce listeners with order values within those ranges so that they can be positioned correctly.

Related Issues

@nizametdinovcrx
Copy link

@sbrannen, great, thx!

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 status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

6 participants