-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add test for Sse method indexing on native #1613
Add test for Sse method indexing on native #1613
Conversation
run tests |
...ient-reactive/src/test/java/io/quarkus/ts/http/restclient/reactive/ReactiveRestClientIT.java
Outdated
Show resolved
Hide resolved
...ent-reactive/src/main/java/io/quarkus/ts/http/restclient/reactive/resources/SseResource.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you probably want to squash these two commits together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mocenas I looking at it and I thing it is covered by #1517 as I disable it here https://github.com/quarkus-qe/quarkus-test-suite/blob/main/http/http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/BaseHttpAdvancedReactiveIT.java#L453
So I don't know if we want to merge almost same code except you seem to have little different updates
method. As this is both in http reactive part of TS. Can you look at it if it's test different thing or it's the same?
Btw in that PR it was updated to not use only String
as StringBuilder
is not thread safe and LockSupport
was replaced by CountDownLatch
I'm unable to reproduce the issue quarkusio/quarkus#36986 with this test. I was not able to reproduce the issue in the module "http-advanced-reactive" at all. That's why I put it in rest-client-reactive module. I assume it is because it uses "jackson" version of rest client, but I'm not sure. I tried to reproduce it with quarkus-bom v 3.5.1. If you have better luck reproducing it, I agree it would be better to not duplicate this, otherwise we will have to. I agree that using thread safe things is better. |
@jedla97 Please review ^^ if it is possible to reproduce this issue with current code, or should we merge this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mocenas thanks for update and change it to thread safe. Just one small thing can you squash the commits to one.
Tested it and I was unable to reproduce the error (still don't know why). So yes we need to merge this. Thanks for looking at it and point it to me.
I will squash the commits on github merge. |
OK. I know it exist but wasn't sure how the result will look. Now I know 😄 |
* Add test for Sse method indexing on native
Summary
Add tests for quarkusio/quarkus#36986
Please select the relevant options.
run tests
phrase in comment)Checklist: