Add test coverage for issue 36402

[mocenas]

Create test coverage for issue: quarkusio/quarkus#36402.

Summary

Issue fails the Server Sent Events (SSE) while in native mode. New coverage adds app to simulate & test this behavior.
Original reproducer is here.

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

[michalvavrik]

Hey @jedla97 , I have more than one comment, but I have doubts we need a new module and so much code to test missing classes in the REST Client Classic. I'll verify you are right and get back to you when I have a time. Please wait till then, thank you.

[michalvavrik]

Hi @jedla97 ,

I have tested it a bit and all that I needed to reproduce the issue was following patch (I'm pretty sure you can simplify that as well, but I don't have more time)

Subject: [PATCH] sse-native
---
Index: http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java b/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java
new file mode 100644
--- /dev/null	(date 1699377305495)
+++ b/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java	(date 1699377305495)
@@ -0,0 +1,18 @@
+package io.quarkus.ts.http.advanced;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.event.Observes;
+import jakarta.enterprise.event.Startup;
+import jakarta.ws.rs.client.ClientBuilder;
+import jakarta.ws.rs.client.WebTarget;
+import jakarta.ws.rs.sse.SseEventSource;
+
+@ApplicationScoped
+public class SseClient {
+
+    void callSomeMethod(@Observes Startup startup) {
+        WebTarget target = ClientBuilder.newClient().target("http://localhost:8080/updates");
+        var src = SseEventSource.target(target).build();
+        System.out.println("I have a client " + src);
+    }
+}

with command mvn clean verify -Dnative -Dit.test=HttpAdvancedIT -Dquarkus.platform.version=3.2.7.Final

Please refactor it to your needs. Do you agree that it will cost us much less of execution time and it's easier to read? Please feel free to contradict. Thanks

[jedla97]

Hi, @michalvavrik did you mean me or it was mistake as the@ mocenas created this PR and the questions more target PR creator.

[michalvavrik]

Sorry @jedla97 for some reason I thought you opened issues. I thought it was weird considering to who is assigned to the JIRA ticket :-D

[michalvavrik]

@mocenas the comment was for you

[mocenas]

Hi @jedla97 ,

I have tested it a bit and all that I needed to reproduce the issue was following patch (I'm pretty sure you can simplify that as well, but I don't have more time)

Subject: [PATCH] sse-native
---
Index: http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java b/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java
new file mode 100644
--- /dev/null	(date 1699377305495)
+++ b/http/http-advanced/src/main/java/io/quarkus/ts/http/advanced/SseClient.java	(date 1699377305495)
@@ -0,0 +1,18 @@
+package io.quarkus.ts.http.advanced;
+
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.event.Observes;
+import jakarta.enterprise.event.Startup;
+import jakarta.ws.rs.client.ClientBuilder;
+import jakarta.ws.rs.client.WebTarget;
+import jakarta.ws.rs.sse.SseEventSource;
+
+@ApplicationScoped
+public class SseClient {
+
+    void callSomeMethod(@Observes Startup startup) {
+        WebTarget target = ClientBuilder.newClient().target("http://localhost:8080/updates");
+        var src = SseEventSource.target(target).build();
+        System.out.println("I have a client " + src);
+    }
+}

with command mvn clean verify -Dnative -Dit.test=HttpAdvancedIT -Dquarkus.platform.version=3.2.7.Final

Please refactor it to your needs. Do you agree that it will cost us much less of execution time and it's easier to read? Please feel free to contradict. Thanks

@michalvavrik I agree that your solution is simpler and faster, but I'm not sure I like it.

1. I though that goal of test coverage for backports is to have a test that will tell you if the issue is fixed or not (if this specific functionality works or not). your solution will just fail to start the tests and tell nothing - unless you know this exact issue, you'll have no idea why it failed (OK I can add a try-catch and print better exception message, but you get the point).
2. IMHO to verify that issue is fixed, there should be a test that has a pass run, proving that the features actually work. IMHO having just "module did not fail to start" is kind of bad proof that issue is indeed fixed.
3. IMHO new tests should not interfere with other ones. Your proposed solution, if failed, will cause entire http-advanced module to fail, because tested app will not start. And all other tests in it will just not run. That's why I've created a new module - to isolate the issue.
4. Due to bug description it requires picoli on client side. I don't know why it successfully fails in your example. But I wanted to move it to another module, so adding picoli will not interfere with other tests.

I'm new to issue-test-coverage this stuff so correct me if I'm wrong at my line of thinking.

[michalvavrik]

I though that goal of test coverage for backports is to have a test that will tell you if the issue is fixed or not (if this specific functionality works or not)

Agreed, that's is why I spent time understanding a root cause of issue and principle how it works and created simplified reproducer. Issue has literally nothing to do with picocli.

your solution will just fail to start the tests and tell nothing - unless you know this exact issue, you'll have no idea why it failed (OK I can add a try-catch and print better exception message, but you get the point).

My solution was POC for you, it's your job and I won't tell you how to do this. If I were to do this reproducer, I'd simply put what I added from startup observer to resource method and call it from a unit test, which tells you exactly what is tested

IMHO to verify that issue is fixed, there should be a test that has a pass run, proving that the features actually work. IMHO having just "module did not fail to start" is kind of bad proof that issue is indeed fixed.

+1, please see answer above

IMHO new tests should not interfere with other ones. Your proposed solution, if failed, will cause entire http-advanced module to fail, because tested app will not start. And all other tests in it will just not run. That's why I've created a new module - to isolate the issue.

no worry, if you put it to resource method, it interferes with nothing as you are bringing in no dependencies or application class changes, you can only add new resource class

Due to bug description it requires picoli on client side. I don't know why it successfully fails in your example. But I wanted to move it to another module, so adding picoli will not interfere with other tests.

no it does not, I already explained it

[michalvavrik]

@mocenas one else thing - there is nothing wrong about your reproducer (which is adjusted upstream reproducer, right?), my argument is that resources are tight, my suggestion will add couple of microseconds to execution time, new module build in native will add couple of minutes on my workstation

[mocenas]

@michalvavrik I've refactored the test to be in http-advanced module.

[michalvavrik]

overall looks good, I need to leave now for a while, I'll review & merge this later today. only thing that really concerns me is whether we need dedicated test class (we have comment thread on that)

[michalvavrik]

lgtm, let see what CI says (I'll re-run test locally before I merge it)