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

ArC fixes for spec compatibility, round 2 #30666

Merged
merged 5 commits into from
Feb 2, 2023
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 27, 2023

Related to #28558

  • fix obtaining priority of a producer-based alternative from class-level @Priority
    • producers take priority from the declaring class
  • fix generating Bean.create() to wrap checked exceptions in CreationException
    • the path of least resistance: what was up to now generated as create() is now called doCreate() and create() just wraps that into try/catch
  • add support for static disposer methods
    • also fixes a NPE in case of static producer method declared on a @Dependent bean
  • improve runtime representation of array bean types
    • we always used GenericArrayType to represent array bean types at runtime, which is unexpected for people, and also for at least one TCK test that expects a java.lang.Class representation of String[]
  • add validation for array-typed producers

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jan 27, 2023
@quarkus-bot

This comment has been minimized.

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 30, 2023
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jan 30, 2023

@Ladicek I don't think I have seen this test fail before ^

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 30, 2023

Interesting. The failure is:

2023-01-30T14:38:49.7646913Z [INFO] Running io.quarkus.context.test.SimpleContextPropagationTest
2023-01-30T14:38:51.9399577Z 2023-01-30 14:38:51,927 INFO  [io.quarkus] (main) quarkus-integration-test-smallrye-context-propagation 999-SNAPSHOT on JVM (powered by Quarkus 999-SNAPSHOT) started in 1.155s. Listening on: http://localhost:8081
2023-01-30T14:38:51.9400595Z 2023-01-30 14:38:51,927 INFO  [io.quarkus] (main) Profile test activated. 
2023-01-30T14:38:51.9401730Z 2023-01-30 14:38:51,927 INFO  [io.quarkus] (main) Installed features: [agroal, cdi, hibernate-orm, hibernate-orm-panache, jdbc-h2, narayana-jta, resteasy-reactive, smallrye-context-propagation, vertx]
2023-01-30T14:38:56.0898796Z 2023-01-30 14:38:56,081 INFO  [io.quarkus] (main) quarkus-integration-test-smallrye-context-propagation stopped in 0.037s
2023-01-30T14:38:56.1016170Z [ERROR] Tests run: 11, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 6.325 s <<< FAILURE! - in io.quarkus.context.test.SimpleContextPropagationTest
2023-01-30T14:38:56.1017059Z [ERROR] io.quarkus.context.test.SimpleContextPropagationTest.testArcTCContextPropagationDisabled  Time elapsed: 0.012 s  <<< FAILURE!
2023-01-30T14:38:56.1018082Z org.opentest4j.AssertionFailedError: expected: <2> but was: <3>
2023-01-30T14:38:56.1018574Z 	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
2023-01-30T14:38:56.1019163Z 	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
2023-01-30T14:38:56.1019713Z 	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
2023-01-30T14:38:56.1020208Z 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
2023-01-30T14:38:56.1020674Z 	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:145)
2023-01-30T14:38:56.1021146Z 	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:528)
2023-01-30T14:38:56.1021813Z 	at io.quarkus.context.test.SimpleContextPropagationTest.testArcTCContextPropagationDisabled(SimpleContextPropagationTest.java:92)
2023-01-30T14:38:56.1022503Z 	at ...

@manovotn
Copy link
Contributor

Note that this test used to be flaky, but I am not aware of it being problematic since we swapped tests to use RR instead of classic RE (Oct last year).
See the history - https://github.com/quarkusio/quarkus/commits/main/integration-tests/smallrye-context-propagation/src/test/java/io/quarkus/context/test/SimpleContextPropagationTest.java

This commit also fixes a NPE in case of static producer method
declared on a `@Dependent` bean.
Previously, we always used `GenericArrayType` to represent array
bean types at runtime. This is unexpected for people, and also
for at least one TCK test that expects a `java.lang.Class`
representation of `String[]`. This commit makes sure that arrays
of primitive types and simple class types are represented using
an array-typed `java.lang.Class` object.
@manovotn
Copy link
Contributor

manovotn commented Feb 1, 2023

Frankly speaking, I doubt the CP test failure has anything to do with changes in this PR and it's not reproducible locally.
I'd say if the next CI run passes it, we should be fine merging it.
In case the test fails elsewhere, we should start treating it as flaky, probably temporarily excluding it, but I haven't seen it fail anywhere else yet.

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 1, 2023

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

@Ladicek Ladicek merged commit 2a5b547 into quarkusio:main Feb 2, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 2, 2023
@Ladicek Ladicek deleted the arc-fixes branch February 2, 2023 08:09
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants