Test coverage for ensuring the MessageBodyWriter is used with null Accept header #1949

jcarranzan · 2024-08-28T09:34:53Z

Summary

Coverage for backports 3.8.6 --> quarkusio/quarkus#41411 - Ensure that MessageBodyWriter is passed the proper media type.
Verify that currently , it works with quarkus-rest

Notice this was failing with quarkus version:
mvn clean verify -Dquarkus.platform.version=3.11.2 -Dit.test=HeadersIT#testWithNoAcceptHeader -Dreruns=0

Also, I noticed that the fix is not working with quarkus-resteasy,I'll open a bug related to.

Please select the relevant options.

Bug fix (non-breaking change which fixes an issue)
Dependency update
Refactoring
Backport
New scenario (non-breaking change which adds functionality)
This change requires a documentation update
This change requires execution against OCP (use run tests phrase in comment)

Checklist:

Methods and classes used in PR scenarios are meaningful
Commits are well encapsulated and follow the best practices

jedla97

Thanks for coverage. I have some comments and suggestion.

.../http-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/YamlProvider.java

...tp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/HeadersResource.java

http/http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/HeadersIT.java

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/HeadersMessageBodyWriter.java

...ed-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/HeadersMessageBodyWriter.java

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

michalvavrik

What is the point of coverage like this?

Upstream has completely same Quarkus REST tests, don't you trust upstream CI that has to pass or do you expect something specific for a product? I don't think we should mirror every test in upstream. @fedinskiy do we really need to test this? Cannot you use upstream test coverage label?

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

michalvavrik · 2024-08-29T06:04:02Z

Test coverage for backport 3.8.6-41411

Please improve PR title, it will take just a few seconds to make clear what 41411 is. Thank you

jcarranzan · 2024-08-29T06:24:38Z

What is the point of coverage like this?

Upstream has completely same Quarkus REST tests, don't you trust upstream CI that has to pass or do you expect something specific for a product? I don't think we should mirror every test in upstream. @fedinskiy do we really need to test this? Cannot you use upstream test coverage label?

@michalvavrik the point is to verify the upstream backports for 3.8.6, and when I do that I try to create some test coverage in our TS, verify some corner cases(RestEasy), etc, but you are right if these tests are already in upstream tests, I don't want duplicity.
Maybe, I am missing something in the backports testing process, What is your way of proceeding in these cases?
Thanks

michalvavrik · 2024-08-29T06:32:00Z

Maybe, I am missing something in the backports testing process, What is your way of proceeding in these cases?
Thanks

Yeah @jcarranzan , your work is alright. The reason why I am asking @fedinskiy (responsible for this release) is that I would simply replace this with JIRA label. Because there are some backports that is alright not to test. So I cannot let what's next, all my comments are rather opinions, you don't need to follow them :-)

michalvavrik · 2024-08-29T06:59:59Z

Anyway @jcarranzan @jedla97 feel free to proceed with this PR, I don't mean to obstruct.

fedinskiy · 2024-08-29T07:05:50Z

@michalvavrik just in case: this task is related to 3.8.6, and I am responsible for 3.15 :)

michalvavrik · 2024-08-29T07:07:05Z

@michalvavrik just in case: this task is related to 3.8.6, and I am responsible for 3.15 :)

damn!

michalvavrik · 2024-08-29T07:07:56Z

In that case I'll leave this to @jcarranzan and @jedla97 and move to my tasks.

jcarranzan · 2024-08-29T09:08:25Z

Issue created: quarkusio/quarkus#42854

jedla97

@jcarranzan Thanks for update. In overall it's look good to me. Just have 2 suggestion and also unresolve the thread about that logging with question.

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

...tp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/HeadersResource.java

jedla97 · 2024-08-29T10:15:03Z

Also looking at the test and it seems that HeadersMessageBodyWriter changing all other tests so they are failing.

jcarranzan · 2024-08-29T11:06:46Z

Also looking at the test and it seems that HeadersMessageBodyWriter changing all other tests so they are failing.

Fixed!, thanks @jedla97 .

jedla97

Hi @jcarranzan I'm looking at this and don't like that this message header writer affecting other test and resources.

I looked at it and proposing solution.

That MessageBodyWriter<String> won't be catching a modify the all String responses but you will create new class (your own datatype) and it will be modified only the response types of this class. So the implementation will be something like MessageBodyWriter<MySimpleClass>.

What I have looked it's valid usecase + you will have different test then is covered in upstream as bonus. I'll leave the implementation details on you. Just FYI I tried it with simple class and it worked.

Also when we use this implementation we don't need separate test class just for this one test (save few minutes when executing native 😄)

jedla97

Thanks for looking at proposed solution! I have few small comments .

Also I see that you checked the Backport in Pr description. This point is used when you backorting something to different branch.

Looking at HeadersIT the it's run on openshift OpenShiftHeadersIT so the This change requires execution against OCP (use run tests phrase in comment) point should be checked to not merge this before we run ocp tests

...tp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/HeadersResource.java

...-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/BaseHttpAdvancedReactiveIT.java

...ive/src/test/java/io/quarkus/ts/http/advanced/reactive/DevModeGrpcIntegrationReactiveIT.java

http/http-advanced-reactive/src/test/java/io/quarkus/ts/http/advanced/reactive/HeadersIT.java

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/BaseHttpAdvancedIT.java

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/DevModeGrpcIntegrationIT.java

http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/HeadersResource.java

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

jedla97 · 2024-09-02T21:08:00Z

run tests

jedla97 · 2024-09-03T06:46:41Z

@michalvavrik can you do a review as I was finished this PR.

michalvavrik · 2024-09-03T07:36:52Z

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

@@ -93,6 +96,19 @@ void testPathSpecificHeaderRulesOrder() {
        cacheControlMatches(response, "max-age=1");
    }

+    @Test
+    @Tag("https://github.com/quarkusio/quarkus/pull/41411")


I refuse to believe that PR only changing independent RESTEasy Reactive library has to be tested in the RESTEasy Classic. They are completely independent implementations.

If you mean to test that same issue is not present in the RESTEasy Classic, I can respect that (though I would not), but I would not link the PR. Maybe link the issue and add comment that this is not reproducing, just verifying it is not there?

I'm for having the same test in RESTEasy Classic to ensure the same behavior in this case.

+1 to have just comment as @michalvavrik suggested, that coverage is inspired by the issue, but it's not reproducing it on classic side.

TBH I was thinking yesterday if I should remove this classic coverage. Your reasoning make sense so I remove it.

@jcarranzan if you still think that we should test even the rest classic please create PR

it's nice you be agreed with @jedla97 , but @rsvoboda doesn't :-) maybe just tweak the comment.

my thinking was that users shouldn't use RESTEasy Classic anyway, so I care way less about it.

Oh now I read @rsvoboda comment so I will update it back

Reverted back + changed to use original issue in comment

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java

michalvavrik

Job well done @jedla97 .

addressed

jcarranzan requested a review from gtroitsk August 28, 2024 09:34

jcarranzan marked this pull request as ready for review August 28, 2024 09:35

jcarranzan marked this pull request as draft August 28, 2024 10:30

jcarranzan requested review from jedla97 and removed request for gtroitsk August 28, 2024 13:00

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch from fa67b16 to 58624c3 Compare August 28, 2024 14:14

jcarranzan marked this pull request as ready for review August 28, 2024 14:16

jedla97 requested changes Aug 28, 2024

View reviewed changes

michalvavrik requested changes Aug 29, 2024

View reviewed changes

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java Outdated Show resolved Hide resolved

jcarranzan changed the title ~~Test coverage for backport 3.8.6-41411~~ Test coverage for ensuring the MessageBodyWriter is used with null Accept header - backport 3.8.6#41411 Aug 29, 2024

fedinskiy added the triage/backport-3.8 label Aug 29, 2024

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch from 1adbf8c to c069169 Compare August 29, 2024 07:23

jcarranzan requested a review from jedla97 August 29, 2024 07:23

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch from f0fde39 to a1ed08a Compare August 29, 2024 08:51

jedla97 requested changes Aug 29, 2024

View reviewed changes

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java Outdated Show resolved Hide resolved

...tp-advanced-reactive/src/main/java/io/quarkus/ts/http/advanced/reactive/HeadersResource.java Outdated Show resolved Hide resolved

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch from 471377b to d3e7a96 Compare August 29, 2024 11:06

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch 4 times, most recently from c31c109 to 26de7b5 Compare August 29, 2024 14:38

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch 3 times, most recently from 0079f9e to 91db874 Compare August 30, 2024 06:52

jedla97 requested changes Aug 30, 2024

View reviewed changes

jcarranzan force-pushed the backports/3.8.6-backports-41411 branch from 57b83d5 to 9731967 Compare August 30, 2024 14:50

jcarranzan requested a review from jedla97 August 30, 2024 14:57

jedla97 added the triage/backport-3.15 label Aug 30, 2024

jedla97 previously requested changes Aug 30, 2024

View reviewed changes

jedla97 force-pushed the backports/3.8.6-backports-41411 branch 2 times, most recently from 9bc2d17 to c23d6a9 Compare September 2, 2024 21:06

jedla97 requested a review from michalvavrik September 3, 2024 06:46

michalvavrik reviewed Sep 3, 2024

View reviewed changes

Test coverage for backport 3.8.6-41411

8e24f71

rsvoboda changed the title ~~Test coverage for ensuring the MessageBodyWriter is used with null Accept header - backport 3.8.6#41411~~ Test coverage for ensuring the MessageBodyWriter is used with null Accept header Sep 3, 2024

jedla97 force-pushed the backports/3.8.6-backports-41411 branch 2 times, most recently from 8d08dd1 to 3a3e9cb Compare September 3, 2024 08:27

michalvavrik reviewed Sep 3, 2024

View reviewed changes

http/http-advanced/src/test/java/io/quarkus/ts/http/advanced/HeadersIT.java Outdated Show resolved Hide resolved

Finish test coverage for quarkusio/quarkus#41411

5973827

michalvavrik approved these changes Sep 3, 2024

View reviewed changes

jedla97 force-pushed the backports/3.8.6-backports-41411 branch from 3a3e9cb to 5973827 Compare September 3, 2024 08:33

michalvavrik approved these changes Sep 3, 2024

View reviewed changes

michalvavrik merged commit 8a130aa into quarkus-qe:main Sep 4, 2024
8 checks passed

michalvavrik deleted the backports/3.8.6-backports-41411 branch September 4, 2024 08:39

jedla97 added this to the 3.8 milestone Sep 5, 2024

jedla97 removed the triage/backport-3.8 label Sep 5, 2024

michalvavrik removed the triage/backport-3.15 label Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test coverage for ensuring the MessageBodyWriter is used with null Accept header #1949

Test coverage for ensuring the MessageBodyWriter is used with null Accept header #1949

jcarranzan commented Aug 28, 2024 •

edited by jedla97

Loading

jedla97 left a comment

michalvavrik left a comment

michalvavrik commented Aug 29, 2024 •

edited

Loading

jcarranzan commented Aug 29, 2024 •

edited

Loading

michalvavrik commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

fedinskiy commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

jcarranzan commented Aug 29, 2024

jedla97 left a comment

jedla97 commented Aug 29, 2024

jcarranzan commented Aug 29, 2024

jedla97 left a comment

jedla97 left a comment

jedla97 commented Sep 2, 2024

jedla97 commented Sep 3, 2024

michalvavrik Sep 3, 2024

rsvoboda Sep 3, 2024

jedla97 Sep 3, 2024

michalvavrik Sep 3, 2024 •

edited

Loading

jedla97 Sep 3, 2024

jedla97 Sep 3, 2024

michalvavrik left a comment

Test coverage for ensuring the MessageBodyWriter is used with null Accept header #1949

Test coverage for ensuring the MessageBodyWriter is used with null Accept header #1949

Conversation

jcarranzan commented Aug 28, 2024 • edited by jedla97 Loading

Summary

Checklist:

jedla97 left a comment

Choose a reason for hiding this comment

michalvavrik left a comment

Choose a reason for hiding this comment

michalvavrik commented Aug 29, 2024 • edited Loading

jcarranzan commented Aug 29, 2024 • edited Loading

michalvavrik commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

fedinskiy commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

michalvavrik commented Aug 29, 2024

jcarranzan commented Aug 29, 2024

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 commented Aug 29, 2024

jcarranzan commented Aug 29, 2024

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 left a comment

Choose a reason for hiding this comment

jedla97 commented Sep 2, 2024

jedla97 commented Sep 3, 2024

michalvavrik Sep 3, 2024

Choose a reason for hiding this comment

rsvoboda Sep 3, 2024

Choose a reason for hiding this comment

jedla97 Sep 3, 2024

Choose a reason for hiding this comment

michalvavrik Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

jedla97 Sep 3, 2024

Choose a reason for hiding this comment

jedla97 Sep 3, 2024

Choose a reason for hiding this comment

michalvavrik left a comment

Choose a reason for hiding this comment

jcarranzan commented Aug 28, 2024 •

edited by jedla97

Loading

michalvavrik commented Aug 29, 2024 •

edited

Loading

jcarranzan commented Aug 29, 2024 •

edited

Loading

michalvavrik Sep 3, 2024 •

edited

Loading