Add tests coverage for REST client reactive using HTTP2 #1637

jedla97 · 2024-02-01T01:36:05Z

Summary

Adding test coverage for QUARKUS-658.

This cover basic scenarios which Quarkus users can use.

The http2ClientAsync and http2ClientSync which are removed is covered by new tests. In addition the class these test were using were also removed. The ones from http-advanced were removed completely as Quarkus doesn't plan to support the non-reactive client.

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-02-01T11:51:12Z

The HttpVersionAlpnWithConfigKeyIT.testAsyncResponseWithSingleClientOnSingleClient and HttpVersionAlpnWithConfigKeyIT.testSyncResponseWithSingleClientOnSingleClient are expected to fail because the quarkusio/quarkus#38514 but I PR for it is created and passing so it will be merged pretty soon.

Cookie test failing also on daily so it's not caused by this PR

jcarranzan · 2024-02-01T15:31:07Z

The HttpVersionAlpnWithConfigKeyIT.testAsyncResponseWithSingleClientOnSingleClient and HttpVersionAlpnWithConfigKeyIT.testSyncResponseWithSingleClientOnSingleClient are expected to fail because the quarkusio/quarkus#38514 but I PR for it is created and passing so it will be merged pretty soon.

Cookie test failing also on daily so it's not caused by this PR

The issue is already closed so we can run again the tests

jedla97 · 2024-02-02T00:26:27Z

The failing test are just cookies which will be fixed by #1639

jcarranzan

Hi @jedla97, I've left some minor comments, but overall, the code looks good to me. Additionally, I was thinking that you might consider including a couple of tests that intentionally induce failures or check for the possibility of receiving a status code other than 200.
Thank you

...rc/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionClientResource.java

...ive/src/test/java/io/quarkus/ts/http/restclient/reactive/HttpVersionAlpnWithConfigKeyIT.java

http/rest-client-reactive/src/main/resources/httpVersion.properties

jedla97 · 2024-02-02T13:09:18Z

Thanks for review @jcarranzan I update things which you have pointed out.

Additionally, I was thinking that you might consider including a couple of tests that intentionally induce failures or check for the possibility of receiving a status code other than 200.

In this I will try thing about negative scenario and if I have some idea I'll create standalone PR for it.
In case of checking for 200 status in tests.

It's there yous to check but not testing the client itself. The test calling endpoint where is the client created/injected and that endpoint calling another endpoint as rest client. So status in tests is there just to check that's response is as expected (client use/not use http2). If there will be some problem with client it should return 500.

jedla97 · 2024-02-02T13:15:07Z

Just note why we can't use the rest client directly in test:

quarkusio/quarkus#25041

Tl;DR : At this moment quarkus rest client reactive is not meant to be used standalone.

jcarranzan

Thank you for the tweaks, just only a comment about the MalformedURLException signature and I'll approve it.

michalvavrik

I don't have time to review this right now (not that I was asked to :-D), but once this is approved please do not merge. There is a lot of changes (nothing against them!) and I'd like to check when I find a time. Thanks

http/rest-client-reactive/src/main/resources/httpVersion.properties

...ive/src/main/java/io/quarkus/ts/http/restclient/reactive/HttpVersionClientWithConfigKey.java

michalvavrik

This looks fine. @jcarranzan please have a look next week, review and merge. If you need more time, can you maybe create dedicated ticket for yourself in JIRA, please? Let's move on. Thx

...rc/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionClientResource.java

michalvavrik · 2024-02-10T10:34:05Z

.../src/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionBaseResource.java

+import io.smallrye.mutiny.Uni;
+
+@ApplicationScoped
+@Path("/httpVersion")


Suggested change

@Path("/httpVersion")

@Path("/http-version")

That's usual convention, or are you testing whether it works with case sensitive path? If so, let's document.

michalvavrik · 2024-02-10T10:35:23Z

...ive/src/test/java/io/quarkus/ts/http/restclient/reactive/HttpVersionAlpnWithConfigKeyIT.java

+@QuarkusScenario
+public class HttpVersionAlpnWithConfigKeyIT {
+
+    String SUCCESSFUL_RESPONSE_STRING = "Hello " + NAME;


Suggested change

String SUCCESSFUL_RESPONSE_STRING = "Hello " + NAME;

private static final String SUCCESSFUL_RESPONSE_STRING = "Hello " + NAME;

michalvavrik · 2024-02-10T10:37:45Z

...rc/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionClientResource.java

+                .build(HttpVersionClient.class);
+
+        Response response = httpVersionClient.getClientHttpVersionAsync(NAME).await()
+                .atMost(Duration.ofSeconds(TIMEOUT_SECONDS));


I think blocking here is just laziness, there is no point to it. Asynchronous test should be processed in a reactive manner and return stuff asynchronously from resource as well. Please feel free to prove me wrong. Thanks

michalvavrik · 2024-02-10T10:38:45Z

...rc/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionClientResource.java

+
+    /**
+     * Create HttpVersionClient and make REST client asynchronous request to `httpVersion/asynchronous endpoint`
+     * Response from the client is checked if http 2 was used


Suggested change

* Response from the client is checked if http 2 was used

* Response from the client is checked if HTTP/2 was used

please check other comments as well. As you have already decided to write nice Java doc.

michalvavrik · 2024-02-10T10:51:30Z

.../src/main/java/io/quarkus/ts/http/restclient/reactive/resources/HttpVersionBaseResource.java

+    @GET
+    @Path("synchronous")
+    public Response sayHello(@QueryParam("name") String name) {
+        return Response.ok("Hello " + name).build();


Hello is fine, you can keep it if you want as it tests query param. It would be nice to have incoming request protocol in response. This way, you can check that REST client is telling you a true about used HTTP version. I propose to inject RoutingContext, then io.vertx.ext.web.RoutingContext#request -> io.vertx.core.http.HttpServerRequest#version should always return HTTP_2.

michalvavrik · 2024-02-10T10:55:34Z

@jedla97 let's add info about HTTP/2 for REST client into root project README

…ADME

…ctive

jedla97 · 2024-02-13T16:29:39Z

@jcarranzan I update all the comments mentioned by @michalvavrik so can you have quick look. Also add mention of these test to the README.

jcarranzan · 2024-02-13T19:05:43Z

Changes LGTM, and tests are correct as well. However I noticed the test classes seem to contain repetitive tests, and perhaps using inheritance could consolidate tests into a single class. Nonetheless, I respect the current approach, it's simply a different way of approaching the tests.
On the other hand, try to resolve the comments from Michal, still I can see some unresolved threads, thank you.

jcarranzan · 2024-02-13T19:05:55Z

Changes LGTM, and tests are correct as well. However I noticed the test classes seem to contain repetitive tests, and perhaps using inheritance could consolidate tests into a single class. Nonetheless, I respect the current approach, it's simply a different way of approaching the tests.
On the other hand, try to resolve the comments from Michal, still I can see some unresolved threads, thank you.

jedla97 · 2024-02-13T19:43:19Z

Thanks for review

However I noticed the test classes seem to contain repetitive tests, and perhaps using inheritance could consolidate tests into a single class

When writing the test I was thinking about it, but if we want one class as base for multiple tests there would be subset of 2 tests for all 4 tests classes.
When we would have 2 base classes the subset of test would be 4 (all https tests). So in the result we shouldn't have 4 classes but 6.

In my POV it's not that useful in this scenario.

On the other hand, try to resolve the comments from Michal, still I can see some unresolved threads, thank you.

I update the solution but sum it in one comment as I was resolve all the comment on code side (not GitHub side)

jedla97 force-pushed the QUARKUS-658 branch 2 times, most recently from 37cc3d7 to ce004ca Compare February 1, 2024 11:02

jedla97 marked this pull request as ready for review February 1, 2024 11:03

jedla97 requested a review from jcarranzan February 1, 2024 11:04

jedla97 force-pushed the QUARKUS-658 branch from ce004ca to 6f4dbd3 Compare February 1, 2024 21:08

jcarranzan reviewed Feb 2, 2024

View reviewed changes

http/rest-client-reactive/src/main/resources/httpVersion.properties Outdated Show resolved Hide resolved

jedla97 force-pushed the QUARKUS-658 branch 2 times, most recently from f13e12e to f84d112 Compare February 2, 2024 13:01

jedla97 requested a review from jcarranzan February 5, 2024 13:15

jcarranzan reviewed Feb 5, 2024

View reviewed changes

jedla97 force-pushed the QUARKUS-658 branch from f84d112 to 965ad83 Compare February 5, 2024 16:16

jedla97 requested a review from jcarranzan February 5, 2024 16:17

michalvavrik reviewed Feb 5, 2024

View reviewed changes

http/rest-client-reactive/src/main/resources/httpVersion.properties Outdated Show resolved Hide resolved

jedla97 force-pushed the QUARKUS-658 branch from 965ad83 to 49c8de8 Compare February 5, 2024 21:16

jcarranzan reviewed Feb 6, 2024

View reviewed changes

...ive/src/main/java/io/quarkus/ts/http/restclient/reactive/HttpVersionClientWithConfigKey.java Outdated Show resolved Hide resolved

jedla97 force-pushed the QUARKUS-658 branch from 49c8de8 to 5905342 Compare February 6, 2024 12:20

michalvavrik approved these changes Feb 10, 2024

View reviewed changes

jedla97 force-pushed the QUARKUS-658 branch from 5905342 to 544227e Compare February 13, 2024 16:01

jedla97 added 2 commits February 13, 2024 17:23

Add tests coverage for REST client reactive using HTTP2 and update RE…

fa479aa

…ADME

Remove REST client tests as they adapted and moved to rest-client-rea…

de3936d

…ctive

jedla97 force-pushed the QUARKUS-658 branch from 544227e to de3936d Compare February 13, 2024 16:25

jcarranzan merged commit 8abc157 into quarkus-qe:main Feb 13, 2024
7 checks passed

jedla97 added the triage/backport-3.8 label Feb 13, 2024

fedinskiy mentioned this pull request Feb 16, 2024

Backports/3.8 16.02 #1668

Merged

9 tasks

michalvavrik removed the triage/backport-3.8 label Feb 19, 2024

jedla97 deleted the QUARKUS-658 branch December 10, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests coverage for REST client reactive using HTTP2 #1637

Add tests coverage for REST client reactive using HTTP2 #1637

jedla97 commented Feb 1, 2024 •

edited

Loading

jedla97 commented Feb 1, 2024

jcarranzan commented Feb 1, 2024

jedla97 commented Feb 2, 2024

jcarranzan left a comment

jedla97 commented Feb 2, 2024

jedla97 commented Feb 2, 2024

jcarranzan left a comment

michalvavrik left a comment

michalvavrik left a comment •

edited

Loading

michalvavrik Feb 10, 2024

michalvavrik Feb 10, 2024

michalvavrik Feb 10, 2024

michalvavrik Feb 10, 2024

michalvavrik Feb 10, 2024

michalvavrik commented Feb 10, 2024

jedla97 commented Feb 13, 2024

jcarranzan commented Feb 13, 2024

jcarranzan commented Feb 13, 2024

jedla97 commented Feb 13, 2024

	String SUCCESSFUL_RESPONSE_STRING = "Hello " + NAME;
	private static final String SUCCESSFUL_RESPONSE_STRING = "Hello " + NAME;

	* Response from the client is checked if http 2 was used
	* Response from the client is checked if HTTP/2 was used

Add tests coverage for REST client reactive using HTTP2 #1637

Add tests coverage for REST client reactive using HTTP2 #1637

Conversation

jedla97 commented Feb 1, 2024 • edited Loading

Summary

Checklist:

jedla97 commented Feb 1, 2024

jcarranzan commented Feb 1, 2024

jedla97 commented Feb 2, 2024

jcarranzan left a comment

Choose a reason for hiding this comment

jedla97 commented Feb 2, 2024

jedla97 commented Feb 2, 2024

jcarranzan left a comment

Choose a reason for hiding this comment

michalvavrik left a comment

Choose a reason for hiding this comment

michalvavrik left a comment • edited Loading

Choose a reason for hiding this comment

michalvavrik Feb 10, 2024

Choose a reason for hiding this comment

michalvavrik Feb 10, 2024

Choose a reason for hiding this comment

michalvavrik Feb 10, 2024

Choose a reason for hiding this comment

michalvavrik Feb 10, 2024

Choose a reason for hiding this comment

michalvavrik Feb 10, 2024

Choose a reason for hiding this comment

michalvavrik commented Feb 10, 2024

jedla97 commented Feb 13, 2024

jcarranzan commented Feb 13, 2024

jcarranzan commented Feb 13, 2024

jedla97 commented Feb 13, 2024

jedla97 commented Feb 1, 2024 •

edited

Loading

michalvavrik left a comment •

edited

Loading