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

Add coverage for Quarkus #43422 #2186

Merged
merged 1 commit into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package io.quarkus.ts.http.advanced.reactive;

import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;

public interface GreetingInterface {

@GET
@Path("/interface-greeting")
String interfaceGreeting();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package io.quarkus.ts.http.advanced.reactive;

import jakarta.enterprise.inject.spi.CDI;
import jakarta.ws.rs.Path;

@Path("/greeting")
public class GreetingOptionAndHeadResource implements GreetingInterface {

@Path("/cdi-sub-resource")
public GreetingSubResources helloFromSubResource() {
return CDI.current()
.select(GreetingSubResources.class)
.get();
}

@Override
public String interfaceGreeting() {
return "Greeting from interface";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package io.quarkus.ts.http.advanced.reactive;

import jakarta.enterprise.context.RequestScoped;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.POST;

@RequestScoped
public class GreetingSubResources {

@GET
public String get() {
return "Greeting from sub-resource using GET";
}

@POST
public String post() {
return "Greeting from sub-resource using POST";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import static org.apache.http.HttpStatus.SC_NOT_FOUND;
import static org.apache.http.HttpStatus.SC_OK;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.in;
Expand Down Expand Up @@ -102,6 +104,64 @@ public void abstractResourceWithPath() {
.body(is("Hello from Quarkus REST"));
}

@Test
@DisplayName("Test Quarkus REST using CDI getting the sub-resource")
void cdiSubResourceGetRequest() {
getApp().given()
.when()
.get(GREETING_ENDPOINT + "/cdi-sub-resource")
.then()
.statusCode(SC_OK)
.body(is("Greeting from sub-resource using GET"));
}

@Test
@DisplayName("Test Quarkus REST using CDI getting the sub-resource")
void cdiSubResourcePostRequest() {
getApp().given()
.when()
.post(GREETING_ENDPOINT + "/cdi-sub-resource")
.then()
.statusCode(SC_OK)
.body(is("Greeting from sub-resource using POST"));
}

@Test
@DisplayName("Test Quarkus REST interface resource with @Path")
void interfaceResourceWithPath() {
getApp().given()
.when()
.get(GREETING_ENDPOINT + "/interface-greeting")
.then()
.statusCode(SC_OK)
.body(is("Greeting from interface"));
}

@Test
@Tag("https://github.com/quarkusio/quarkus/issues/43422")
@DisplayName("Test Quarkus REST abstract resource with @Path using OPTION and HEAD request")
@DisabledOnNative(reason = "https://github.com/quarkusio/quarkus/issues/42976")
void optionAndHeadRequestUsingAbstractResourceWithPath() {
testOptionRequest(GREETING_ENDPOINT);
testHeadRequest(GREETING_ENDPOINT);
}

@Test
@Tag("https://github.com/quarkusio/quarkus/issues/43422")
@DisplayName("Test Quarkus REST interface resource with @Path using OPTION and HEAD request")
void optionAndHeadRequestUsingInterfaceResourceWithPath() {
testOptionRequest(GREETING_ENDPOINT + "/interface-greeting");
testHeadRequest(GREETING_ENDPOINT + "/interface-greeting");
}

@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() {
Copy link
Member

@michalvavrik michalvavrik Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thanks

testOptionRequest(GREETING_ENDPOINT + "/cdi-sub-resource");
testHeadRequest(GREETING_ENDPOINT + "/cdi-sub-resource");
}

@Test
@DisplayName("GRPC Server test")
public void testGrpc() {
Expand Down Expand Up @@ -354,6 +414,24 @@ private void testResponseContentType(String acceptedContentType, String expected
.then().header(CONTENT_TYPE, expectedContentType);
}

private void testOptionRequest(String path) {
getApp().given()
.when()
.options(path)
.then()
.statusCode(SC_OK)
.header(HttpHeaders.ALLOW, allOf(containsString("HEAD"),
containsString("OPTIONS"), containsString("GET")));
}

private void testHeadRequest(String path) {
getApp().given()
.when()
.head(path)
.then()
.statusCode(SC_OK);
}

private ValidatableResponse req99BottlesOfBeer(int bottleNumber, int httpStatusCode) {
return getApp().given()
.get(ROOT_PATH + NinetyNineBottlesOfBeerResource.PATH + "/" + bottleNumber)
Expand Down
Loading