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

Limit MATCHING_RESOURCES TestResources to the test that declares them #44279

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

snazy
Copy link
Contributor

@snazy snazy commented Nov 4, 2024

This comment was marked as resolved.

@geoand geoand changed the title NOT A PR, Reproducer for #44235 Limit MATCHING_RESOURCES TestResources to the test that declares them Nov 4, 2024
@geoand geoand marked this pull request as ready for review November 4, 2024 11:17
@geoand geoand changed the title Limit MATCHING_RESOURCES TestResources to the test that declares them Limit MATCHING_RESOURCES TestResources to the test that declares them Nov 4, 2024

This comment has been minimized.

@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

@geoand your change LGTM! I've added some missing scope-checks, which otherwise eventually caused callbacks via AbstractTestWithCallbacksExtension not being properly registered, causing class-loader issues with NoSuchMethodExceptions due to "stale" callbacks still being registered.

Applied the change against the 3.16.1 tag and ran our integration tests, manually verifying that only the right test-resources (containers) are running.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

Thanks a lot!

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

@snazy something seems to have broke with the latest change

@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

@geoand yea, just making MATCHING_RESOURCES work like RESTRICTED_TO_CLASS isn't enough (actually, too much).

This comment has been minimized.

…to `@QuarkusIntegrationTest`

In case there are both test classes annotated with `@QuarkusTest` and `@QuarkusIntegrationTest`, JUnit callbacks (those in `AbstractTestWithCallbacksExtension`) are not properly cleaned up, causing the (later) run integration tests to try to call the "old" callbacks, which cannot work (class loader issue). This change adds a `Runnable` to clean up the memoized callbacks in `QuarkusTestExtensionState.close()`, which works for both `IntegrationTestExtensionState` and `QuarkusTestExtension.ExtensionState`.
@snazy
Copy link
Contributor Author

snazy commented Nov 4, 2024

Added two more fixes to this change.

  1. consider the resource-args when ordering tests, and also for restarting
  2. ensure that the JUnit callbacks are cleaned up, when having test classes having a mixed set of @QuarkusTest and @QuarkusIntegrationTest.

Re 2) I realized that the NoSuchMethodException was actually caused by JUnit callbacks that were registered for a @QuarkusTest (we call it an integration test in Nessie, because it uses test containers), but threw when running a @QuarkusIntegrationTest, because the callbacks were not cleared.

@geoand
Copy link
Contributor

geoand commented Nov 4, 2024

Thanks!

Copy link

quarkus-bot bot commented Nov 4, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 54f8235.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit 4a41afc into quarkusio:main Nov 5, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 5, 2024
@jedla97
Copy link
Contributor

jedla97 commented Nov 6, 2024

@geoand @snazy looking at this at this and think that this bug fix should be documented in release notes as it can possibly break someone tests.
Our one TS adopted this back in September and I probably use incorectly MATCHING_RESOURCES scope (Probably I interpret the java docs incorectly and it was working fine). There can be some early adopter of this which done same error as me (also in guides we still use @QuarkusTestResource so there isn't much example or info about this).

If I understand this change correctly. If you want use the MATCHING_RESOURCES you need to anotate every test class with it, to all the test classes with same resource are grouped together and use that resource without restart. Before this PR it behave same/similar to GLOBAL, that there was no need of anotation of every test classes. So it was enough to have one testResource class to start the resource to be used for all the test.

Simply:

  • RESTRICTED_TO_CLASS start for every test class
  • MATCHING_RESOURCES start for specific group of test classes
  • GLOBAL start for every test class

@geoand
Copy link
Contributor

geoand commented Nov 7, 2024

Before this PR it behave same/similar to GLOBAL, that there was no need of anotation of every test classes

Correct, but this was a bug honestly

@geoand
Copy link
Contributor

geoand commented Nov 7, 2024

That said, improvements to the docs and / or Javadoc are always welcome :)

benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this pull request 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.16: @WithTestResource starts all test resources (regression)
4 participants