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

3.16: @WithTestResource starts all test resources (regression) #44235

Closed
snazy opened this issue Oct 31, 2024 · 10 comments · Fixed by #44279
Closed

3.16: @WithTestResource starts all test resources (regression) #44235

snazy opened this issue Oct 31, 2024 · 10 comments · Fixed by #44279
Labels
area/testing kind/bug Something isn't working
Milestone

Comments

@snazy
Copy link
Contributor

snazy commented Oct 31, 2024

Describe the bug

Since Quarkus 3.16 (including 3.16.1, 3.15.1 is fine) our integration test classes start literally test-resources from all literally all @WithTestResource annotations, even for those that are not (yet) executed.

Before 3.16(.1), only the test-resource for the currently running integration-test-class were started.

The major change in behavior that I could identify so far is in io.quarkus.test.common.TestResourceManager#restrictToAnnotatedClass, which now delegates to the TestResourceClassEntryHandler implementations (for @QuarkusTestResource and @WithTestResource). (Admittedly, restrictToAnnotatedClass could never yield true before 3.16.)

Since restrictToAnnotatedClass() checks for == RESTRICTED_TO_CLASS on all annotations "everywhere", it initialized and starts all test resources. This can lead to e.g. 4 Keycloaks, 2 Minios and a bunch of other containers 🤷

Our integration-tests are annotated using @WithTestResource(value = xyz.class), leaving the default scope MATCHING_RESOURCES to leverage the ... Quarkus will not restart when running consecutive tests that use the same set of resources. behavior.

The current workaround is to set the scope to RESTRICTED_TO_CLASS.

/cc @geoand

Expected behavior

  • Only have the test-resources for the current test class running
  • Group tests by test-resources, considering "initArgs"
  • Don't start the "whole world" ;)

Actual behavior

No response

How to Reproduce?

I do have a reproducer, but it's still a (local) WIP to bump Quarkus to 3.16.1 in Nessie, including the workaround mentioned above.

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

@snazy snazy added the kind/bug Something isn't working label Oct 31, 2024
snazy added a commit to snazy/nessie that referenced this issue Oct 31, 2024
@geoand
Copy link
Contributor

geoand commented Oct 31, 2024

Thanks a lot for reporting.

Any chance you can add a reproducer so I can easily see the difference between 3.15 and 3.16 in action?

snazy added a commit to snazy/nessie that referenced this issue Oct 31, 2024
snazy added a commit to projectnessie/nessie that referenced this issue Oct 31, 2024
@hemanth-web-dev
Copy link

In Quarkus 3.16+, @WithTestResource annotations are unexpectedly starting all defined test resources for every test class, not just those needed for the current test, which worked as expected in previous versions. This change, due to TestResourceManager#restrictToAnnotatedClass now checking RESTRICTED_TO_CLASS by default, results in redundant containers starting up (e.g., multiple Keycloak and Minio instances) and increased resource consumption. While setting scope = RESTRICTED_TO_CLASS on each @WithTestResource annotation provides a temporary workaround, we need a fix that respects the original resource-scoping behavior, ensuring only necessary resources start per test class, as in Quarkus <= 3.15.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

Please provide us with a sample application were we cansee the difference in action thus making said fix much more likely

snazy added a commit to projectnessie/nessie that referenced this issue Nov 4, 2024
snazy added a commit to snazy/quarkus that referenced this issue Nov 4, 2024
@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

Here's a reproducer: #44279
The added tests verify that only the expected test-resources are started (uses TestInjector for this), but fails because all test resources are started.

There are three resources - all with scope MATCHING_RESOURCES.
One for "test one", one for "test two" and one that's shared but with different init-args. The shared test-resource is also started twice, but associates the wrong test-resource to the test (wrong value injected into the test class).

Sample output:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.301 s <<< FAILURE! -- in io.quarkus.it.extension.testresources.WithResourcesPoliciesFirstTest
[ERROR] io.quarkus.it.extension.testresources.WithResourcesPoliciesFirstTest.checkOnlyResource1started -- Time elapsed: 0.141 s <<< FAILURE!
org.opentest4j.AssertionFailedError: 

expected: ["resource1", null, "test-one"]
 but was: ["SomeResource1", "SomeResource2", "test-two"]
	at io.quarkus.it.extension.testresources.WithResourcesPoliciesFirstTest.checkOnlyResource1started(WithResourcesPoliciesFirstTest.java:28)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:971)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:821)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[INFO] Running io.quarkus.it.extension.testresources.WithResourcesPoliciesSecondTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.013 s <<< FAILURE! -- in io.quarkus.it.extension.testresources.WithResourcesPoliciesSecondTest
[ERROR] io.quarkus.it.extension.testresources.WithResourcesPoliciesSecondTest.checkOnlyResource1started -- Time elapsed: 0.001 s <<< FAILURE!
org.opentest4j.AssertionFailedError: 

expected: [null, "resource2", "test-two"]
 but was: ["SomeResource1", "SomeResource2", "test-two"]
	at io.quarkus.it.extension.testresources.WithResourcesPoliciesSecondTest.checkOnlyResource1started(WithResourcesPoliciesSecondTest.java:28)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:971)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:821)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

Thanks @snazy!

@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

Maybe a bit clearer, when just running one of the added tests:

./mvnw -pl integration-tests/test-extension/tests install -Dtest=WithResourcesPoliciesFirstTest



...
SomeResource1 start
SharedResource start with arg 'test-one'
SomeResource2 start
SharedResource start with arg 'test-two'
...
[ERROR] io.quarkus.it.extension.testresources.WithResourcesPoliciesFirstTest.checkOnlyResource1started -- Time elapsed: 0.143 s <<< FAILURE!
org.opentest4j.AssertionFailedError: 

expected: ["resource1", null, "test-one"]
 but was: ["SomeResource1", "SomeResource2", "test-two"]
	at io.quarkus.it.extension.testresources.WithResourcesPoliciesFirstTest.checkOnlyResource1started(WithResourcesPoliciesFirstTest.java:28)

...

SomeResource1 stop
SharedResource stop
SomeResource2 stop
SharedResource stop

Starting SomeResource2 and one of the SharedResource is wrong here.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

@snazy @hemanth-web-dev can you please describe to me what your Quarkus 3.15 test resource setup was like (the case of the reproducer however indeed quite clear)?

The reason I am asking here is that it is not clear at all to me what you expect the semantic of MATCHING_RESOURCES to be when used with resources that are not part of a class.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

In any case, can you give the updated version of #44279 a try? It should fix the problem

@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

@snazy @hemanth-web-dev can you please describe to me what your Quarkus 3.15 test resource setup was like (the case of the reproducer however indeed quite clear)?

What's in the reproducer (minus cosmetic things like omitting default annotation values).

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

Thanks @snazy

snazy added a commit to projectnessie/nessie that referenced this issue Nov 4, 2024
snazy added a commit to projectnessie/nessie that referenced this issue Nov 4, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 5, 2024
@gsmet gsmet modified the milestones: 3.17 - main, 3.16.2 Nov 5, 2024
snazy added a commit to projectnessie/nessie that referenced this issue Nov 11, 2024
benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this issue Nov 13, 2024
…us-googlecloud-jsonlogging!24)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-extension-processor](https://github.com/quarkusio/quarkus) |  | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-extension-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-bom](https://github.com/quarkusio/quarkus) | import | patch | `3.16.1` -> `3.16.3` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.16.1` -> `3.16.3` |

---

### Release Notes

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.16.3`](quarkusio/quarkus@3.16.2...3.16.3)

[Compare Source](quarkusio/quarkus@3.16.2...3.16.3)

### [`v3.16.2`](https://github.com/quarkusio/quarkus/releases/tag/3.16.2)

[Compare Source](quarkusio/quarkus@3.16.1...3.16.2)

##### Complete changelog

-   [#&#8203;34824](quarkusio/quarkus#34824) - AmazonLambdaRecorder Handler Discovery Erroneously Considers Decorators
-   [#&#8203;38086](quarkusio/quarkus#38086) - Documentation about `RecordCodecProvider` in MongoDB with Panache
-   [#&#8203;42149](quarkusio/quarkus#42149) - Upgrade Postgres 16
-   [#&#8203;44039](quarkusio/quarkus#44039) - WebSockets Next: create a new event loop context for each client
-   [#&#8203;44132](quarkusio/quarkus#44132) - Update getting-started-reactive.adoc
-   [#&#8203;44149](quarkusio/quarkus#44149) - Fix Config Error Screen
-   [#&#8203;44152](quarkusio/quarkus#44152) - Do not throw NPE in AfterAll interceptor if application didn't start
-   [#&#8203;44155](quarkusio/quarkus#44155) - Amazon Lambda - Support decorators
-   [#&#8203;44156](quarkusio/quarkus#44156) - Make OidcRequestContextProperties modifiable
-   [#&#8203;44178](quarkusio/quarkus#44178) - Bump com.fasterxml.jackson:jackson-bom from 2.18.0 to 2.18.1
-   [#&#8203;44183](quarkusio/quarkus#44183) - Properly apply the update recipes in version order
-   [#&#8203;44184](quarkusio/quarkus#44184) - Add Support for Trusted Proxy Detection on Forwarded Requests
-   [#&#8203;44185](quarkusio/quarkus#44185) - Repeating `@PermissionsAllowed` annotations totally disable method authentication
-   [#&#8203;44189](quarkusio/quarkus#44189) - Small improvements to the Deploying to Google Cloud guide
-   [#&#8203;44190](quarkusio/quarkus#44190) - ResteasyReactiveProcessor#setupEndpoints reports duplicate endpoints when a rest client matches a resource
-   [#&#8203;44191](quarkusio/quarkus#44191) - Update PostgreSQL image to 17
-   [#&#8203;44201](quarkusio/quarkus#44201) - Broken link
-   [#&#8203;44202](quarkusio/quarkus#44202) - Broken link?
-   [#&#8203;44203](quarkusio/quarkus#44203) - Make OidcRequestContextProperties modifiable
-   [#&#8203;44204](quarkusio/quarkus#44204) - Fix broken doc links
-   [#&#8203;44207](quarkusio/quarkus#44207) - Bump bouncycastle.version from 1.78.1 to 1.79
-   [#&#8203;44209](quarkusio/quarkus#44209) - Bump io.quarkus.develocity:quarkus-project-develocity-extension from 1.1.6 to 1.1.7
-   [#&#8203;44221](quarkusio/quarkus#44221) - Add extension description for websockets next
-   [#&#8203;44227](quarkusio/quarkus#44227) - Add stork-configuration-generator as an annotationProcessorPath
-   [#&#8203;44229](quarkusio/quarkus#44229) - ContainerResponseFilter with `@Priority(Integer.MIN_VALUE)` will be actually invoked with max priority
-   [#&#8203;44232](quarkusio/quarkus#44232) - Quartz: use a more reasonable default for quarkus.quartz.thread-count
-   [#&#8203;44235](quarkusio/quarkus#44235) - 3.16: `@WithTestResource` starts all test resources (regression)
-   [#&#8203;44237](quarkusio/quarkus#44237) - Properly implement priority of ContainerResponseFilter
-   [#&#8203;44238](quarkusio/quarkus#44238) - Refactor SecurityTransformerUtils to consider repeated annotations
-   [#&#8203;44239](quarkusio/quarkus#44239) - Replace oidc auth facebook screenshots with generic ones
-   [#&#8203;44244](quarkusio/quarkus#44244) - Bump `quarkiverse-parent` to 18
-   [#&#8203;44245](quarkusio/quarkus#44245) - Delete disabled job
-   [#&#8203;44248](quarkusio/quarkus#44248) - Ignore client interfaces when detecting duplicate endpoints
-   [#&#8203;44263](quarkusio/quarkus#44263) - Quarkus Dev UI - Clicking on gRPC - Services - service implementation class  Uncaught exception received by Vert.x
-   [#&#8203;44277](quarkusio/quarkus#44277) - Dev UI Open in IDE make sure lineNumber is in quotes
-   [#&#8203;44279](quarkusio/quarkus#44279) - Limit `MATCHING_RESOURCES` TestResources to the test that declares them
-   [#&#8203;44281](quarkusio/quarkus#44281) - Included pages within a fragment ignores rendered=false property.
-   [#&#8203;44298](quarkusio/quarkus#44298) - Qute: fix rendered=false if a fragment includes nested fragment
-   [#&#8203;44300](quarkusio/quarkus#44300) - When testing request payload is populated with string "null" if enable-reflection-free-serializers enabled
-   [#&#8203;44309](quarkusio/quarkus#44309) - Avoid deserializing null nodes in reflection free Jackson serialization
-   [#&#8203;44316](quarkusio/quarkus#44316) - Duplicated field serialization using the generated reflection free Jackson serializers
-   [#&#8203;44317](quarkusio/quarkus#44317) - Avoid duplicated field serialization in reflection free Jackson serializers
-   [#&#8203;44321](quarkusio/quarkus#44321) - Use Java 21 by default in the Deploying to Google Cloud guide
-   [#&#8203;44322](quarkusio/quarkus#44322) - Explain in MongoDB docs that records are supported
-   [#&#8203;44324](quarkusio/quarkus#44324) - Take config annotation when trying to match test resources

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
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.

4 participants