Add coverage for Quarkus #43422 #2186

jedla97 · 2024-11-12T22:08:22Z

Summary

Adding coverage for quarkusio/quarkus#43422 issue.

The PR to fix the original issue is quarkusio/quarkus#43440

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 · 2024-11-12T22:08:39Z

run tests

jedla97 · 2024-11-12T23:32:47Z

Native is related will look at it

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

jcarranzan

code LGTM thanks, but I'll wait for the native fixes updated and changes requested.

rsvoboda

OPTIONS is not covered
what about other HTTP methods and sub-resource use case?
test failures need investigation

jedla97 · 2024-11-13T15:29:11Z

Native issue was caused by this quarkusio/quarkus#42976 so I create different resource class where I tested it + testing it with interface.

Added OPTION request, what I tried it worked same even before the upstream PR.
Added POST method to sub-resource to check if it works (Personaly think that using it like this is more like edgecase)

jedla97 · 2024-11-13T15:29:44Z

run tests

jcarranzan

Thanks @jedla97 the tests look good to me and now are covering all topics mentioned.
I have two small suggestions:

Could you consider creating a reusable helper method since most of tests have similar structure body?
On the other hand, I am wondering if it would be good having some negative test that returns for instance a 405 status code where be appropiated.

This is cover age for quarkusio/quarkus#43440

jedla97 · 2024-11-14T11:30:27Z

Could you consider creating a reusable helper method since most of tests have similar structure body?

@jcarranzan I created the methods for HEAD and OPTION requests. With that I combined them into one test.

On the other hand, I am wondering if it would be good having some negative test that returns for instance a 405 status code where be appropiated.

Tbh I don't see much added value in these negative tests. Also what should be testing? Not able to send DELETE request as it's not defined on endpoint? If you insist on adding something like this I can but as I said I don't see any adding value as I believe that this is tested by unit test in Quarkus/Vert.x/RestEasy

jedla97 · 2024-11-14T11:30:40Z

run tests

jcarranzan · 2024-11-14T11:33:18Z

Ok thanks, let's wait for the checks.

michalvavrik · 2024-11-14T17:56:35Z

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

+    @Test
+    @Tag("https://github.com/quarkusio/quarkus/issues/43422")
+    @DisplayName("Test Quarkus REST using CDI getting the sub-resource for OPTION and HEAD request")
+    void cdiSubResourceOptionAndHeadRequest() {


This is only method that fails with 3.15.1 in JVM mode. Why are other tests optionAndHeadRequestUsingInterfaceResourceWithPath and optionAndHeadRequestUsingAbstractResourceWithPath tagged with https://github.com/quarkusio/quarkus/issues/43422 if they are passing with 3.15.1 in JVM mode?

I added the tag there as this issue was main motivation to create that coverage. If you things tha tag shouldn't be there. I can create PR for its removal.

Clear answer :-D :

I don't really care, keep it as it. I just thought that if you tag something with an issue, it means that issue is tested by this. It can still be true that changed code is not failing but testing changes because it can test code that we didn't have covered previously. What I do when I verify bug tickets is that I look for tests tagged with that issue. Now I'll keep in mind that failing test tagged with an issue may not mean that issue is not fixed. It probably won't happen anyway.

Also, why are other 3 tests added in this PR without this tag, is that because they were not added because of this issue or you just forgot?

Goodcatch @michalvavrik. @jedla97 can you prepare a followup PR with backport label?

Also, why are other 3 tests added in this PR without this tag, is that because they were not added because of this issue or you just forgot?

There are more like to ensure that endpoint work. The quarkus fix was to add HEAD and OPTION. At least this is my logic behind adding the tag here.

@jedla97 can you prepare a followup PR with backport label?

@rsvoboda with adding or removing the tags?

I think this kinda depends on how other QE members use @Tag, I always thought that it signals that some test tests the issue/ticket. But maybe that was just me.

@Tag usage ... there is no rule really, some members started to use it, but we didn't have any discussion and conclusion on its usage.

I would personally use @Tag only on tests testing the bug. I wouldn't tag the other related tests.

Alright, thanks

michalvavrik · 2024-11-15T11:41:37Z

@jedla97 I think we should try to have PR titled with a text that describes what PR does, you checked https://cbea.ms/git-commit/, coverage for Quarkus #43422 is less informative then Test support for @HEAD and @OPTIONS in sub-resources or something like that. I realize sometimes it is really hard to write correct title (and I often fail).

jedla97 added the triage/backport-3.15 label Nov 12, 2024

jedla97 requested a review from jcarranzan November 12, 2024 22:08

jcarranzan reviewed Nov 13, 2024

View reviewed changes

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

jcarranzan approved these changes Nov 13, 2024

View reviewed changes

rsvoboda requested changes Nov 13, 2024

View reviewed changes

jedla97 force-pushed the 43440-coverage branch from 2e9e1de to 1fb23ed Compare November 13, 2024 15:23

jedla97 requested a review from jcarranzan November 13, 2024 15:29

jcarranzan reviewed Nov 14, 2024

View reviewed changes

Add coverage for HEAD request

a5bef9d

This is cover age for quarkusio/quarkus#43440

jedla97 force-pushed the 43440-coverage branch from 1fb23ed to a5bef9d Compare November 14, 2024 11:20

rsvoboda approved these changes Nov 14, 2024

View reviewed changes

rsvoboda merged commit 2ebfe00 into quarkus-qe:main Nov 14, 2024
9 checks passed

michalvavrik reviewed Nov 14, 2024

View reviewed changes

jedla97 deleted the 43440-coverage branch November 15, 2024 08:52

jedla97 mentioned this pull request Nov 27, 2024

[3.15] 3.15 backports 27.11.2024 #2222

Merged

9 tasks

jedla97 removed the triage/backport-3.15 label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add coverage for Quarkus #43422 #2186

Add coverage for Quarkus #43422 #2186

jedla97 commented Nov 12, 2024

jedla97 commented Nov 12, 2024

jedla97 commented Nov 12, 2024

jcarranzan left a comment •

edited

Loading

rsvoboda left a comment

jedla97 commented Nov 13, 2024

jedla97 commented Nov 13, 2024

jcarranzan left a comment

jedla97 commented Nov 14, 2024

jedla97 commented Nov 14, 2024

jcarranzan commented Nov 14, 2024

michalvavrik Nov 14, 2024 •

edited

Loading

jedla97 Nov 15, 2024

michalvavrik Nov 15, 2024

michalvavrik Nov 15, 2024

rsvoboda Nov 15, 2024

jedla97 Nov 15, 2024

michalvavrik Nov 15, 2024

rsvoboda Nov 15, 2024

michalvavrik Nov 15, 2024

michalvavrik commented Nov 15, 2024

Add coverage for Quarkus #43422 #2186

Add coverage for Quarkus #43422 #2186

Conversation

jedla97 commented Nov 12, 2024

Summary

Checklist:

jedla97 commented Nov 12, 2024

jedla97 commented Nov 12, 2024

jcarranzan left a comment • edited Loading

Choose a reason for hiding this comment

rsvoboda left a comment

Choose a reason for hiding this comment

jedla97 commented Nov 13, 2024

jedla97 commented Nov 13, 2024

jcarranzan left a comment

Choose a reason for hiding this comment

jedla97 commented Nov 14, 2024

jedla97 commented Nov 14, 2024

jcarranzan commented Nov 14, 2024

michalvavrik Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalvavrik commented Nov 15, 2024

jcarranzan left a comment •

edited

Loading

michalvavrik Nov 14, 2024 •

edited

Loading