Change default behavior of `@WithTestResource` #42852

geoand · 2024-08-29T08:39:21Z

The new default @WithTestResource behavior is
for Quarkus to NOT restart when consecutive tests
are using the same @WithTestResource annotations.

Despite the first first commit being a refactor and not a behavior change,
it is where the heavy lifting is done (while hopefully making the code clearer to read).

The second commit which brings the change just takes the structure put in place
by the previous one and changes the default.

~~P.S. For now, I only have the refactoring code in this PR, which I want to test against CI as it should not include any behavior change~~

geoand · 2024-09-02T13:50:29Z

The failure in Quickstart compilation is expected

emmanuelbernard · 2024-09-02T15:22:08Z

@geoand you mention consecutive tests. Is there an effort to group tests with the same annotation together ?

geoand · 2024-09-02T15:30:09Z

Quarkus already does that by implementing the appropriate JUnit API.
If it doesn't work as expected, that's a bug

holly-cummins · 2024-09-02T15:30:40Z

@geoand you mention consecutive tests. Is there an effort to group tests with the same annotation together ?

It's done here:

quarkus/test-framework/junit5/src/main/java/io/quarkus/test/junit/util/QuarkusTestProfileAwareClassOrderer.java

Lines 141 to 145 in dcd9928

    
           private boolean hasRestrictedResource(ClassDescriptor classDescriptor) { 
        
               return classDescriptor.findRepeatableAnnotations(WithTestResource.class).stream() 
        
                       .anyMatch(res -> res.restrictToAnnotatedClass() || isMetaTestResource(res, classDescriptor)) || 
        
                       classDescriptor.findRepeatableAnnotations(QuarkusTestResource.class).stream() 
        
                               .anyMatch(res -> res.restrictToAnnotatedClass() || isMetaTestResource(res, classDescriptor));

. There's a bit of documentation for it https://quarkus.io/guides/getting-started-testing#testing_different_profiles but it doesn't explain that it takes into account the other, non-profile, annotations.

FroMage

LGTM

test-framework/common/src/main/java/io/quarkus/test/common/WithTestResource.java

test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusIntegrationTestExtension.java

test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java

gsmet · 2024-09-02T16:18:57Z

test-framework/common/src/main/java/io/quarkus/test/common/WithTestResource.java

@@ -13,11 +13,20 @@
 /**
 * Used to define a test resource, which can affect various aspects of the application lifecycle.
 * <p>
+ * As of Quarkus 3.16, the default behavior of the annotation (meaning that {@code scope} has not been set)
+ * is that test classes annotated with same {@code WithTestResource} will <strong>not</strong> force a restart
+ * of Quarkus.


Maybe I'm blind but while I see the code to order the restricted to class first, I'm not exactly sure how the tests with the same resources are actually grouped?

Are you talking about QuarkusTestProfileAwareClassOrderer? I haven't changed that

Yes I'm talking about this specific class. I think someone said it was ordering classes depending on resources and I don't see that in the code. I just see some grouping by if they have per-class test resources.

Don't we want to group classes which have similar test resources?

I can look into this in a follow up (or maybe @famod can since he wrote the ordering code).
For the time being, I would like to get this if we agree it's the proper functionality as the change has a very high chance of conflicting with any changes made in test handling code.

I don't see that in the code

At a first glimpse I don't see that either. 🤔 It is has been a while, so I'd need some time to look into it.
Until when do we need that adjustment (if needed after all)?

It is has been a while, so I'd need some time to look into it. Until when do we need that adjustment (if needed after all)?

As this PR is breaking and meant to go into 3.16, we still have a weeks

Btw, @QuarkusComponentTest isn't covered either but I haven't looked into whether grouping such tests would make sense at all.

Yeah, it probably makes sense to have those executed at the beginning or the end of the run, but that's definitely something for after this PR is in.

Btw, @QuarkusComponentTest isn't covered either but I haven't looked into whether grouping such tests would make sense at all.

Given they are supposed to be faster and lower level, let's execute them first.
But yes, definitely out of the scope for this PR.

FYI, I haven't had the time to take a stab at the QuarkusTestProfileAwareClassOrderer change. I might have some time for that by the end of next week.

This is a prerequisite of fixing the behavior of `@WithTestResource`

The new default @WithTestResource behavior is for Quarkus to NOT restart when consecutive tests are using the same @WithTestResource annotations.

quarkus-bot · 2024-09-03T10:57:31Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 12fe9e0.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	Quickstarts Compilation - JDK 17	`Compile Quickstarts`	Failures	Logs	Raw logs	🚧

You can consult the Develocity build scans.

Failures

⚙️ Quickstarts Compilation - JDK 17 #

- Failing: optaplanner-quickstart

📦 optaplanner-quickstart

✖ Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:testCompile (default-testCompile) on project optaplanner-quickstart: Compilation failure /home/runner/work/quarkus/quarkus/quarkus-quickstarts/optaplanner-quickstart/src/test/java/org/acme/optaplanner/TestResources.java:[6,57] cannot find symbol symbol: method restrictToAnnotatedClass() location: @interface io.quarkus.test.common.WithTestResource

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/resteasy-reactive/rest-client/deployment

✖ io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService - History

expected: "hello, Alice" but was: "hello, I'm a slow server" - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: 

expected: "hello, Alice"
 but was: "hello, I'm a slow server"
	at io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService(StorkResponseTimeLoadBalancerTest.java:58)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:499)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:413)

emmanuelbernard · 2024-09-03T15:03:37Z

Assuming this is the right track, what would be your doc describing the isolation and how to isolate / not isolate and why? For mere humans :D

With this "doc as spec", we will move the user experience up a notch.
Etiher as a set of paragrahns here for now or already embedded in the right doc as change to the PR.

Thoughts?

geoand · 2024-09-03T15:06:37Z

That's a good question... I am pretty sure it would need a few iterations to be made clear for all people

geoand · 2024-09-03T15:11:36Z

The way I envision it:

Global: applies to all tests and thus doesn't make a difference when it comes to restarting Quarkus
Restricted to class: applies only to the annotated class and thus always forces a restart
Matching: classes with the same such annotations use the same test resources and Quarkus is not restarted between their execution

emmanuelbernard · 2024-09-03T16:00:26Z

test-framework/common/src/main/java/io/quarkus/test/common/TestResourceScope.java

+     */
+    RESTRICTED_TO_CLASS,
+    /**
+     * Means that Quarkus will not restart when running consecutive tests that use the same resource


The same resource or the same set of resources? If the former how do you group to restart the resources less often?

Users can't really influence the order of test execution, but if Quarkus does its job correctly, restarting will be minimized

How? (for my benefit).

https://github.com/quarkusio/quarkus/blob/dcd9928aed836f1b2b4d4a2274bef1b767a97311/test-framework%2Fjunit5%2Fsrc%2Fmain%2Fjava%2Fio%2Fquarkus%2Ftest%2Fjunit%2Futil%2FQuarkusTestProfileAwareClassOrderer.java

We might need to update it a little, but the idea is that we can tell JUnit the order with which tests can be executed

Also my question was
@WithTestResource(value={Resource1.class, Resource2.class, Resource3.class})
will it run in the same batch as @WithTestResource(value={Resource3.class})
what about @WithTestResource(value={Resource3.class, Resource4.class})

Even if those are executed one after the other, they still force a restart because they don't match

ok, thanks. So i'ts not the same resource that matches, it's the exact resource set (or list of resoruces) that matches.

gsmet

Let's merge this as it's going to be very prone to conflicts and let's iterate during the 3.16 development cycle.

geoand · 2024-09-04T08:16:01Z

Thanks!

holly-cummins · 2024-09-04T13:33:13Z

Watching with sadness, as I see the non-trivial conflicts mounting up with #34681. 🫣

Serves me right for going slow. 😆
... but I might hold off on doing a rebase for a while. 😬

geoand · 2024-09-04T13:38:05Z

@holly-cummins do you need help with that one?

FWIW, the code should be simpler to follow now, but a conflict is a conflict no doubt :(

emmanuelbernard · 2024-09-05T13:31:35Z

@gsmet @geoand I was wondering whether we could have the people that experienced OOME and slowdown test main to see if they new see an improvement. Would be sueful not uncover sifts between our expectation and that damned reality

geoand · 2024-09-05T13:33:09Z

Yeah, that's a very good idea!

geoand · 2024-09-06T08:57:10Z

#42471 (comment) is a good indication things are much better now :)

famod · 2024-10-25T12:07:59Z

FWIW, this breaking change is not mentioned in https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.16

geoand · 2024-10-25T12:12:48Z

Hopefully no one was using the old one :)

quarkus-bot bot added the area/testing label Aug 29, 2024

geoand force-pushed the @WithTestResource-fixed branch 8 times, most recently from 07c1585 to b5ff28f Compare August 30, 2024 10:47

geoand changed the title ~~Refactor TestResource handling code~~ Change default behavior of @WithTestResource Aug 30, 2024

geoand changed the title ~~Change default behavior of @WithTestResource~~ Change default behavior of @WithTestResource Aug 30, 2024

geoand added the release/breaking-change label Aug 30, 2024

geoand marked this pull request as ready for review August 30, 2024 20:05

geoand requested review from gsmet and FroMage August 30, 2024 20:05

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Aug 31, 2024

FroMage approved these changes Sep 2, 2024

View reviewed changes

gsmet reviewed Sep 2, 2024

View reviewed changes

Refactor TestResource handling code

e7b2153

This is a prerequisite of fixing the behavior of `@WithTestResource`

geoand force-pushed the @WithTestResource-fixed branch from 8bcffd7 to 91c009e Compare September 3, 2024 06:00

Change default behavior of @WithTestResource

12fe9e0

The new default @WithTestResource behavior is for Quarkus to NOT restart when consecutive tests are using the same @WithTestResource annotations.

geoand force-pushed the @WithTestResource-fixed branch from 91c009e to 12fe9e0 Compare September 3, 2024 06:01

emmanuelbernard reviewed Sep 3, 2024

View reviewed changes

gsmet approved these changes Sep 4, 2024

View reviewed changes

gsmet merged commit 116c8a3 into quarkusio:main Sep 4, 2024
51 of 52 checks passed

quarkus-bot bot added this to the 3.16 - main milestone Sep 4, 2024

geoand deleted the @WithTestResource-fixed branch September 4, 2024 08:16

This was referenced Sep 5, 2024

Injection of BrowserContext stopped working on main/999-SNAPSHOT #43034

Closed

[CI] - Quarkus Super Heroes + Quarkus main #23612

Closed

jedla97 mentioned this pull request Sep 5, 2024

Change parameters for WithTestResource annotation quarkus-qe/beefy-scenarios#505

Merged

geoand mentioned this pull request Sep 5, 2024

OOM in tests (Part 3) #42471

Closed

holly-cummins mentioned this pull request Sep 6, 2024

Improve sharing between different classloaders in a multi-profile test run #43093

Open

geoand mentioned this pull request Sep 20, 2024

QuarkusTestResourceLifecycleManager: Unable to reuse testcontainers instances #43395

Closed

yrodiere mentioned this pull request Oct 28, 2024

TestResourceScope.MATCHING_RESOURCES ignores configuration annotations from QuarkusTestResourceConfigurableLifecycleManager #44129

Closed

Change default behavior of @WithTestResource #42852

Change default behavior of @WithTestResource #42852

Conversation

geoand commented Aug 29, 2024 • edited Loading

This comment has been minimized.

geoand commented Sep 2, 2024

emmanuelbernard commented Sep 2, 2024

geoand commented Sep 2, 2024

holly-cummins commented Sep 2, 2024 • edited Loading

FroMage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quarkus-bot bot commented Sep 3, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Failing Jobs

Failures

⚙️ Quickstarts Compilation - JDK 17 #

📦 optaplanner-quickstart

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/resteasy-reactive/rest-client/deployment

emmanuelbernard commented Sep 3, 2024

geoand commented Sep 3, 2024

geoand commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmanuelbernard Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

geoand commented Sep 4, 2024

holly-cummins commented Sep 4, 2024

geoand commented Sep 4, 2024 • edited Loading

emmanuelbernard commented Sep 5, 2024

geoand commented Sep 5, 2024

geoand commented Sep 6, 2024

famod commented Oct 25, 2024

geoand commented Oct 25, 2024

Change default behavior of `@WithTestResource` #42852

Change default behavior of `@WithTestResource` #42852

geoand commented Aug 29, 2024 •

edited

Loading

holly-cummins commented Sep 2, 2024 •

edited

Loading

quarkus-bot bot commented Sep 3, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

emmanuelbernard Sep 3, 2024 •

edited

Loading

geoand commented Sep 4, 2024 •

edited

Loading