Re-enable MutinyTest #11828
Re-enable MutinyTest #11828
Conversation
kenfinnigan
commented
Sep 2, 2020
kenfinnigan
commented
- Fixes MutinyTest#testSSE can be unstable #11687
- Moved JAX-RS ClientBuilder service file for native to RESTClientProcessor
- Use existing ResteasyProviderFactory to register client providers with if it exists, and don't replace the provider directly
| registerProviders(clientProviderFactory, contributedProviders, false); | ||
| registerProviders(clientProviderFactory, useBuiltIn, providersToRegister, contributedProviders); | ||
|
|
||
| if (ResteasyProviderFactory.peekInstance() != null) { |
There was a problem hiding this comment.
Does the execution order play a role here? @jaikiran reported that sometimes the server one was initialized first and sometimes the client one.
I think we should have a build item ensuring that the order of operations is fully determined.
There was a problem hiding this comment.
To my understanding, the only time there's a problem is when the server one is created first and the REST Client one clobbers it.
I don't think the reverse situation poses any problems.
In addition, there are many times where only the server or only REST Client is present, so I'm not sure how a BuildItem helps in that situation
There was a problem hiding this comment.
Another point to note is that the provider factory for server or client side are actually different instances of the same class, where the "server" instance has a no-op for all client operations, and the "client" instance the reverse.
There was a problem hiding this comment.
We could have a build item in one of the artifacts common to the two, the server would produce it and the client optionally consume it.
This way, if the server is present, we would be sure the client code would be executed after the server code.
Obviously, we can also do the opposite if it makes more sense.
There was a problem hiding this comment.
Another option, which will take some time, is to add the functionality we have for RestClientBuilderImpl to ResteasyClientBuilderImpl that can use a static method to set the provider directly into the instance.
Then ordering isn't an issue.
I've raised https://issues.redhat.com/browse/RESTEASY-2684 to discuss this option with the RESTEasy team
There was a problem hiding this comment.
Again, I want to be sure we properly control the execution order.
@kenfinnigan I can do the work and push an additional commit to your PR if you don't think it's useful.
There was a problem hiding this comment.
I don't see it as necessary or useful
There was a problem hiding this comment.
To reiterate, with this change it's not necessary to control the execution order. And I'm looking to properly fix it in RESTEasy so this wouldn't be needed long term either.
I don't see the need to further complicate the code for a change that isn't permanent
There was a problem hiding this comment.
And I'm looking to properly fix it in RESTEasy so this wouldn't be needed long term either.
Yeah but that won't be for 1.8 and we need a fix now.
And I really think having a deterministic execution order for things that interact with each other is important.
If just for the sake of not spending hours debugging weird issues.
I think adding the build item should be very easy. I'll take a look tomorrow. Either it's easy and don't add boilerplate or I will give up.
There was a problem hiding this comment.
I know it won't be done for 1.8, but why add more code that needs to be removed when it's there in 1.9?
When the change is done in RESTEasy, there is no longer a need for a build item
|
Sorry wrong button, please ignore the approval. |
| registerProviders(clientProviderFactory, contributedProviders, false); | ||
| registerProviders(clientProviderFactory, useBuiltIn, providersToRegister, contributedProviders); | ||
|
|
||
| if (ResteasyProviderFactory.peekInstance() != null) { |
There was a problem hiding this comment.
Yes, we need to be sure it fixes the random ordering.
cescoffier
dismissed
their stale review
Because I didn't get enough coffee this morning.
|
@kenfinnigan can you re-enable the tests from #11711 and see how it behaves? |
- Fixes quarkusio#11687 - Moved JAX-RS ClientBuilder service file for native to RESTClientProcessor - Use existing ResteasyProviderFactory to register client providers with if it exists, and don't replace the provider directly
kenfinnigan
force-pushed
the
mutiny-test
branch
from
892f0a8 to
389aea5
Compare
|
I've re-enabled that test as well, let's see what CI says |
| @@ -43,7 +42,6 @@ | |||
|
|
|||
| @QuarkusTest | |||
| @QuarkusTestResource(MongoTestResource.class) | |||
| @Disabled("See https://github.com/quarkusio/quarkus/issues/11711") | |||
There was a problem hiding this comment.
There's also NativeMongodbPanacheResourceIT in integration-tests/mongodb-panache which has been disabled for this same reason. That one needs to be re-enabled too, either in this PR or as a separate one.
There was a problem hiding this comment.
Sorry, missed that, thought it was just the one.
I'm loathed to re-trigger the full CI build, can it be in a followup if this is ok?
There was a problem hiding this comment.
I'm loathed to re-trigger the full CI build,
Same here.
can it be in a followup if this is ok?
I think that's OK since if this re-enabled regular test passes, it's almost guaranteed the native one too will, given the context of this issue. So we just need to see how this one goes.
kenfinnigan
dismissed
gsmet’s stale review
Switching to use BuildItems can be done later, if desired
