Reset junit test's extension failed state for each test class #38127

zakkak · 2024-01-10T14:05:27Z

Fix Quarkus TestExtensions to avoid mistakenly transferring the failed
state of one test class to the other.

Tests with Mandrel 24.0-dev https://github.com/graalvm/mandrel/actions/runs/7476057769

geoand · 2024-01-10T15:11:54Z

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

        // we reload the test resources if we changed test class and if we had or will have per-test test resources
-        boolean reloadTestResources = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass)
+        boolean reloadTestResources = resetFailedState


This is kind of deceiving in appearances TBH because whether or not reloadTestResources is set to true, is not theorically equivalent to setting resetFailedState to true.

So I would just keep this line the way it was.

If you want to avoid code duplication what I would do is introduce 👍🏼

boolean isNewTestClass = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass);

and use that to set resetFailedState and reloadTestResources

Good point. Done.

geoand · 2024-01-10T15:16:03Z

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

+        // we reset the failed state if we changed test class and the new test class is not a nested class
+        boolean resetFailedState = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass)
+                && !isNested(currentJUnitTestClass, extensionContext.getRequiredTestClass());
+        if (resetFailedState && state != null) {
+            state.setTestFailed(null);
+            currentJUnitTestClass = extensionContext.getRequiredTestClass();
+        }


Same comments here as above

geoand

Thanks!

zakkak · 2024-01-11T12:12:23Z

~~I can reproduce the above issues with main locally,~~ but I don't see them appearing in the Quarkus CI in https://github.com/quarkusio/quarkus/actions/runs/7466758136 so I am not sure if the failures are related or not :/

The CI somehow skips to build locales and locales-app, the more I look at it the more it feels unrelated to me.

geoand · 2024-01-11T15:42:06Z

The fact that we don't get any passing JVM build is troubling

zakkak · 2024-01-12T08:55:59Z

The fact that we don't get any passing JVM build is troubling

And it's also persistent so it's clearly related to the patch.

gsmet · 2024-01-12T12:46:38Z

It looks a bit related to what I was struggling with here: #36356

zakkak · 2024-01-15T12:38:22Z

Interestingly, the tests fail even if I don't actually touch the state (zakkak@4977764)!

zakkak · 2024-01-16T18:54:00Z

So the tests are failing even with just the following patch:

diff --git a/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusIntegrationTestExtension.java b/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusIntegrationTestExtension.java
index 61924375753..0c1c1fdccad 100644
--- a/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusIntegrationTestExtension.java
+++ b/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusIntegrationTestExtension.java
@@ -146,8 +146,14 @@ private QuarkusTestExtensionState ensureStarted(ExtensionContext extensionContex
         QuarkusTestExtensionState state = getState(extensionContext);
         Class<? extends QuarkusTestProfile> selectedProfile = findProfile(testClass);
         boolean wrongProfile = !Objects.equals(selectedProfile, quarkusTestProfile);
+        // we reset the failed state if we changed test class
+        boolean isNewTestClass = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass);
+        //        if (isNewTestClass && state != null) {
+        //            state.setTestFailed(null);
+        //            currentJUnitTestClass = extensionContext.getRequiredTestClass();
+        //        }
         // we reload the test resources if we changed test class and if we had or will have per-test test resources
-        boolean reloadTestResources = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass)
+        boolean reloadTestResources = isNewTestClass
                 && (hasPerTestResources || QuarkusTestExtension.hasPerTestResources(extensionContext));
         if ((state == null && !failedBoot) || wrongProfile || reloadTestResources) {
             if (wrongProfile || reloadTestResources) {
diff --git a/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java b/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
index 4002b9139cf..22533be7a90 100644
--- a/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
+++ b/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java
@@ -586,10 +586,15 @@ private QuarkusTestExtensionState ensureStarted(ExtensionContext extensionContex
         QuarkusTestExtensionState state = getState(extensionContext);
         Class<? extends QuarkusTestProfile> selectedProfile = getQuarkusTestProfile(extensionContext);
         boolean wrongProfile = !Objects.equals(selectedProfile, quarkusTestProfile);
+        // we reset the failed state if we changed test class and the new test class is not a nested class
+        boolean isNewTestClass = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass)
+                && !isNested(currentJUnitTestClass, extensionContext.getRequiredTestClass());
+        //        if (isNewTestClass && state != null) {
+        //            state.setTestFailed(null);
+        //            currentJUnitTestClass = extensionContext.getRequiredTestClass();
+        //        }
         // we reload the test resources if we changed test class and the new test class is not a nested class, and if we had or will have per-test test resources
-        boolean reloadTestResources = !Objects.equals(extensionContext.getRequiredTestClass(), currentJUnitTestClass)
-                && !isNested(currentJUnitTestClass, extensionContext.getRequiredTestClass())
-                && (hasPerTestResources || hasPerTestResources(extensionContext));
+        boolean reloadTestResources = isNewTestClass && (hasPerTestResources || hasPerTestResources(extensionContext));
         if ((state == null && !failedBoot) || wrongProfile || reloadTestResources) {
             if (wrongProfile || reloadTestResources) {
                 if (state != null) {

See https://github.com/zakkak/quarkus/actions/runs/7538846221 and 4e7e9d8

So it feels like the issue is not related to the change itself but rather to how we run the tests.

@Karm does the error ring any bells to you? It looks like the locales test fails to properly build the app dependency.

Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact io.quarkus:quarkus-integration-test-locales-app:jar:999-SNAPSHOT

Interestingly, the app and the top level locales modules don't even show up in the list of modules:

[INFO] Quarkus - Integration Tests - Logging when minimum level is unset  [jar]
[INFO] Quarkus - Integration Tests - Logging when minimum level is set    [jar]
[INFO] Quarkus - Integration Tests - Locales - All                        [jar]
[INFO] Quarkus - Integration Tests - Locales - Some                       [jar]
[INFO] Quarkus - Integration Tests - Redis DevService                     [jar]
[INFO] Quarkus - Integration Tests - gRPC - Descriptor Sets - Parent      [pom]

while normally it looks like:

[INFO] Quarkus - Integration Tests - Logging when minimum level is unset  [jar]
[INFO] Quarkus - Integration Tests - Logging when minimum level is set    [jar]
[INFO] Quarkus - Integration Tests - Locales                              [jar]
[INFO] Quarkus - Integration Tests - Locales - App                        [jar]
[INFO] Quarkus - Integration Tests - Locales - All                        [jar]
[INFO] Quarkus - Integration Tests - Locales - Some                       [jar]
[INFO] Quarkus - Integration Tests - Redis DevService                     [jar]
[INFO] Quarkus - Integration Tests - gRPC - Descriptor Sets - Parent      [pom]

geoand · 2024-01-17T08:43:25Z

I will take a quick look

geoand · 2024-01-17T08:59:18Z

Hopefully #38238 should fix it

zakkak · 2024-01-17T13:54:24Z

Hopefully #38238 should fix it

Thanks @geoand, I am giving it a go in https://github.com/zakkak/quarkus/actions/runs/7556725913

geoand · 2024-01-17T13:59:00Z

🤞🏼

zakkak · 2024-01-17T18:00:57Z

@geoand it works!

geoand · 2024-01-17T18:01:35Z

🥂

zakkak · 2024-01-18T08:40:42Z

This is really puzzling. When I included b5a9b9d in my PR the CI was happy, after rebasing on main after b5a9b9d getting merged it doesn't work again...

geoand · 2024-01-18T08:41:08Z

😱

geoand · 2024-01-18T16:05:05Z

Try rebasing onto main please

Fix Quarkus TestExtensions to avoid mistakenly transferring the failed state of one test class to the other. Closes quarkusio#37809

quarkus-bot · 2024-01-18T22:27:22Z

✔️ 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.

zakkak requested a review from Sgitario January 10, 2024 14:05

zakkak added the area/testing label Jan 10, 2024

zakkak requested a review from geoand January 10, 2024 14:06

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 6213d96 to 86bb694 Compare January 10, 2024 14:07

geoand reviewed Jan 10, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 86bb694 to 9f8096e Compare January 11, 2024 09:11

geoand approved these changes Jan 11, 2024

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 11, 2024

This comment has been minimized.

Sign in to view

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 9f8096e to 82c2a1c Compare January 11, 2024 19:55

This comment has been minimized.

Sign in to view

zakkak marked this pull request as draft January 12, 2024 12:42

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 82c2a1c to 481e6af Compare January 12, 2024 12:44

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 481e6af to 4977764 Compare January 12, 2024 13:48

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch 2 times, most recently from f2b076c to 561ac5e Compare January 15, 2024 22:22

quarkus-bot bot added the area/native-image label Jan 15, 2024

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 561ac5e to 24de35e Compare January 16, 2024 06:45

geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 16, 2024

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 24de35e to 4e7e9d8 Compare January 16, 2024 08:24

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 4e7e9d8 to 929638a Compare January 17, 2024 13:53

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 929638a to db505dd Compare January 17, 2024 18:01

zakkak marked this pull request as ready for review January 17, 2024 18:01

zakkak added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 17, 2024

This comment has been minimized.

Sign in to view

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from db505dd to a05210a Compare January 17, 2024 22:36

This comment has been minimized.

Sign in to view

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from a05210a to 205ac74 Compare January 18, 2024 12:31

This comment has been minimized.

Sign in to view

Reset junit test's extension failed state for each test class

bc7f63b

Fix Quarkus TestExtensions to avoid mistakenly transferring the failed state of one test class to the other. Closes quarkusio#37809

zakkak force-pushed the 2024-01-10-reset-junit-state-for-each-test-class branch from 205ac74 to bc7f63b Compare January 18, 2024 20:20

geoand approved these changes Jan 19, 2024

View reviewed changes

geoand merged commit b56e918 into quarkusio:main Jan 19, 2024
49 checks passed

quarkus-bot bot added this to the 3.9 - main milestone Jan 19, 2024

quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jan 19, 2024

zakkak deleted the 2024-01-10-reset-junit-state-for-each-test-class branch January 19, 2024 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset junit test's extension failed state for each test class #38127

Reset junit test's extension failed state for each test class #38127

zakkak commented Jan 10, 2024

geoand Jan 10, 2024 •

edited

Loading

geoand Jan 10, 2024 •

edited

Loading

zakkak Jan 11, 2024

geoand Jan 10, 2024

zakkak Jan 11, 2024

This comment has been minimized.

geoand left a comment

This comment has been minimized.

zakkak commented Jan 11, 2024 •

edited

Loading

geoand commented Jan 11, 2024

This comment has been minimized.

This comment has been minimized.

zakkak commented Jan 12, 2024

gsmet commented Jan 12, 2024

zakkak commented Jan 15, 2024

zakkak commented Jan 16, 2024

geoand commented Jan 17, 2024

geoand commented Jan 17, 2024

zakkak commented Jan 17, 2024

geoand commented Jan 17, 2024

zakkak commented Jan 17, 2024

geoand commented Jan 17, 2024

This comment has been minimized.

This comment has been minimized.

zakkak commented Jan 18, 2024

geoand commented Jan 18, 2024

This comment has been minimized.

geoand commented Jan 18, 2024

quarkus-bot bot commented Jan 18, 2024 •

edited by github-actions bot

Loading

Reset junit test's extension failed state for each test class #38127

Reset junit test's extension failed state for each test class #38127

Conversation

zakkak commented Jan 10, 2024

geoand Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

geoand Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

zakkak Jan 11, 2024

Choose a reason for hiding this comment

geoand Jan 10, 2024

Choose a reason for hiding this comment

zakkak Jan 11, 2024

Choose a reason for hiding this comment

This comment has been minimized.

geoand left a comment

Choose a reason for hiding this comment

This comment has been minimized.

zakkak commented Jan 11, 2024 • edited Loading

geoand commented Jan 11, 2024

This comment has been minimized.

This comment has been minimized.

zakkak commented Jan 12, 2024

gsmet commented Jan 12, 2024

zakkak commented Jan 15, 2024

zakkak commented Jan 16, 2024

geoand commented Jan 17, 2024

geoand commented Jan 17, 2024

zakkak commented Jan 17, 2024

geoand commented Jan 17, 2024

zakkak commented Jan 17, 2024

geoand commented Jan 17, 2024

This comment has been minimized.

This comment has been minimized.

zakkak commented Jan 18, 2024

geoand commented Jan 18, 2024

This comment has been minimized.

geoand commented Jan 18, 2024

quarkus-bot bot commented Jan 18, 2024 • edited by github-actions bot Loading

geoand Jan 10, 2024 •

edited

Loading

geoand Jan 10, 2024 •

edited

Loading

zakkak commented Jan 11, 2024 •

edited

Loading

quarkus-bot bot commented Jan 18, 2024 •

edited by github-actions bot

Loading