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

QuarkusTest TestProfile pollutes environment of QuarkusComponentTest #46035

Closed
ryandens opened this issue Feb 2, 2025 · 8 comments · Fixed by #46046
Closed

QuarkusTest TestProfile pollutes environment of QuarkusComponentTest #46035

ryandens opened this issue Feb 2, 2025 · 8 comments · Fixed by #46046
Labels
area/testing kind/bug Something isn't working
Milestone

Comments

@ryandens
Copy link
Contributor

ryandens commented Feb 2, 2025

Describe the bug

Having a QuarkusTest provide config overrides via the TestProfile annotation pollutes the environment of other tests in the suite.

I was able to debug the QuarkusComponentTestExtension and inspect the resolved Config object. Using the org.eclipse.microprofile.config.Config.getConfigValue API, I was able to see that the incorrect config value was coming from system properties (I assume set by the quarkus test extension).

Expected behavior

I understand the limitation with Quarkus tests and how the Test Profiles work by requiring a test ordering and a restarting of the Quarkus server (which is part of my motivation for wanting to move more of my tests to be QuarkusComponentTests rather than QuarkusTest).

However, it appears the orderer is unaware of QuarkusComponentTests. It would seem the server needs to either restart after the last QuarkusTest with a TestProfile, or QuarkusComponentTests needed to be ordered before QuarkusTests. I encountered this issue when my test order changed on a quarkus upgrade. However, this doesn't appear to be associated with any change to the QuarkusTestProfileAwareClassOrderer.

It's possible to work around this by extending QuarkusTestProfileAwareClassOrderer and overriding getCustomOrderKey to detect if the class is annotated with QuarkusComponentTest. However, if it is a goal to increase the adoption of QuarkusComponentTests then I think they should be included in the default ordering

Actual behavior

No response

How to Reproduce?

Reproducer: https://github.com/ryandens/quarkus-component-test-reproducer

  1. Clone the repository
  2. Run the build, see the failing QuarkusComponentTest (resolving the non-default config value specified in the QuarkusTest)
  3. Remove the QuarkusTest and see QuarkusComponentTest now passes.
  4. Optionally, enable the quarkus.test.class-orderer commented out. in src/test/resources/application.properties to see that changing the order of the test classes fixes this issue

Output of uname -a or ver

Darwin Mac 24.2.0 Darwin Kernel Version 24.2.0: Fri Dec 6 18:56:34 PST 2024; root:xnu-11215.61.5~2/RELEASE_ARM64_T6020 arm64

Output of java -version

OpenJDK 64-Bit Server VM Temurin-21.0.6+7 (build 21.0.6+7-LTS, mixed mode, sharing)

Quarkus version or git rev

3.18.1

Build tool (ie. output of mvnw --version or gradlew --version)

------------------------------------------------------------ Gradle 8.12.1 ------------------------------------------------------------ Build time: 2025-01-24 12:55:12 UTC Revision: 0b1ee1ff81d1f4a26574ff4a362ac9180852b140 Kotlin: 2.0.21 Groovy: 3.0.22 Ant: Apache Ant(TM) version 1.10.15 compiled on August 25 2024 Launcher JVM: 21.0.6 (Eclipse Adoptium 21.0.6+7-LTS) Daemon JVM: /Users/ryandens/.sdkman/candidates/java/21.0.6-tem (no JDK specified, using current Java home) OS: Mac OS X 15.2 aarch64

Additional information

No response

@ryandens ryandens added the kind/bug Something isn't working label Feb 2, 2025
Copy link

quarkus-bot bot commented Feb 2, 2025

/cc @geoand (kotlin)

@ryandens
Copy link
Contributor Author

ryandens commented Feb 2, 2025

I believe this issue got incorrectly labeled with area/kotlin it likely should be area/testing

@gsmet
Copy link
Member

gsmet commented Feb 3, 2025

@ryandens couldn't agree more with your analysis.

I think we should run the @QuarkusComponentTest first given they are supposed to be a lot more lightweight.

That being said RestorableSystemProperties#close() should have been called as it's a shutdown task and it should restore the values of the system properties.

Could you check why it wasn't called? Or why it didn't do its job?

@ryandens
Copy link
Contributor Author

ryandens commented Feb 3, 2025

👍 interesting, I was able to debug this with ./gradlew test --debug-jvm and step through my test classes + RestorableSystemProperties. The order in which they were called on my machine were:

  1. QuarkusTest
  2. QuarkusComponentTest
  3. RestorableSystemProperties#close()

It's been a while since I've been in the weeds of JUnit Extension state management. I can't remember the guarantees with the extension state getting closed by JUnit - anecdotally, I recall managing state that must be closed after a test class is complete by AfterAllCallback, but I'm not confident that is correct. I quickly searched the JUnit user guide and couldn't find anything definitive.

@gsmet
Copy link
Member

gsmet commented Feb 3, 2025

Yeah so my guess is that the application is not closed when Quarkus switches to component testing. Thus causing the issue.

IIRC, the previous app is stopped when the next test starts.

I wonder if we should just change QuarkusTestProfileAwareClassOrderer#DEFAULT_ORDER_PREFIX_NON_QUARKUS_TEST to 10_ so that non Quarkus tests are executed first:

  • I would expect simple JUnit tests to execute a lot faster and be something you want to check early
  • I would expect @QuarkusComponentTest to be in the exact same situation.

@geoand WDYT?

@geoand
Copy link
Contributor

geoand commented Feb 3, 2025

I think that's a good idea

@gsmet
Copy link
Member

gsmet commented Feb 3, 2025

I'll prepare something along these lines.

gsmet added a commit to gsmet/quarkus that referenced this issue Feb 3, 2025
I was hoping to constrain JUnit tests and @QuarkusComponentTests but
@QuarkusComponentTest is in another artifact so this will have to do.

Related to quarkusio#46035.

Note that you can still break things if you specify another priority but
that's how it is :).
@gsmet
Copy link
Member

gsmet commented Feb 3, 2025

#46046 might help, we will see what CI has to say.

Feel free to test it if you can :).

geoand pushed a commit to gsmet/quarkus that referenced this issue Feb 6, 2025
I was hoping to constrain JUnit tests and @QuarkusComponentTests but
@QuarkusComponentTest is in another artifact so this will have to do.

Related to quarkusio#46035.

Note that you can still break things if you specify another priority but
that's how it is :).
gsmet added a commit to gsmet/quarkus that referenced this issue Feb 10, 2025
I was hoping to constrain JUnit tests and @QuarkusComponentTests but
@QuarkusComponentTest is in another artifact so this will have to do.

Related to quarkusio#46035.

Note that you can still break things if you specify another priority but
that's how it is :).
gsmet added a commit to gsmet/quarkus that referenced this issue Feb 10, 2025
I was hoping to constrain JUnit tests and @QuarkusComponentTests but
@QuarkusComponentTest is in another artifact so this will have to do.

Related to quarkusio#46035.

Note that you can still break things if you specify another priority but
that's how it is :).
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Feb 11, 2025
@gsmet gsmet modified the milestones: 3.19 - main, 3.18.3 Feb 11, 2025
gsmet added a commit to gsmet/quarkus that referenced this issue Feb 11, 2025
I was hoping to constrain JUnit tests and @QuarkusComponentTests but
@QuarkusComponentTest is in another artifact so this will have to do.

Related to quarkusio#46035.

Note that you can still break things if you specify another priority but
that's how it is :).

(cherry picked from commit 3b89af2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants