Refactor Continuous Testing JSON/RPC #43119

ueberfuhr · 2024-09-08T06:13:24Z

This PR resolves #43067 by refactoring the JSON that is exchanged with the RPC service for continuous testing.

more intuitive structure (see Sample JSON)
less payload (50%)
specialized on the UI requirements, so the UI is not required to do further logic
merged both stream observers (_state and _results) to a single one (they were always published at the same time in the RPC service, so they triggered two events that led to rendering in the UI at the same time)

ueberfuhr · 2024-09-08T06:18:28Z

Important

This PR is built on top of the changed of #42988. So the diffs here currently include the diffs from the other PR too.
I would rebase and squash the commits when #42988 is merged.

ueberfuhr · 2024-09-08T14:35:56Z

Here some more details about the changes.

Improvement Details

Before	After	Comments
instance of `io.quarkus.deployment.dev.testing.TestRunResults` (`quarkus-core-deployment`) was serialized directly and returned as JSON (in `quarkus-vertx-http`)	`io.quarkus.deployment.dev.testing.TestRunResults` now implements an interface (in `quarkus-development-mode-spi`), which is then used in the RPC Service (in `quarkus-vertx-http`) and transformed into an instance of `io.quarkus.devui.runtime.continuoustesting.ContinuousTestingJsonRPCState` (which is esp. written to be serialized and used in the UI)	more API-awareness - refactorings in `io.quarkus.deployment.dev.testing.TestRunResults` will not break the compatibility in the Continuous Testing RPC/UI (compiler-checked)
style class for the test results (green, red, grey) was derived from the JUnit enum ´org.junit.platform.engine.TestExecutionResult.Status´	style class is appended directly in the UI	looser coupling
test results were placed into the JSON twice one in `passing`/`failing`/`skipped`, and once in `results`	only `failing`/`skipped` exist	50% size

Class Diagram

I`m not sure whether the dependencies between the modules are correct - I didn't change them. Here's a class diagram visualizing the old (black) and the new code (red):

gsmet

Thanks. I added a couple of questions. But I will let @phillip-kruger review it in depth.

In any case, when rebasing, please make sure the commit comment is meaningful.

gsmet · 2024-09-08T18:15:53Z

core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java

-import org.junit.platform.engine.TestExecutionResult;
-import org.junit.platform.engine.TestSource;
-import org.junit.platform.engine.UniqueId;
+import org.junit.platform.engine.*;


You will need to disable the star imports in your IDE.

Done so, thanks for the hint. What about a .editorconfig file in the project?

If you can provide one that makes sense, that would be awesome.

Let's not be too strict though but this in particular could be added for sure.

gsmet · 2024-09-08T18:19:15Z

core/devmode-spi/src/main/java/io/quarkus/dev/testing/results/TestClassResultInterface.java

+
+import java.util.List;
+
+public interface TestClassResultInterface extends Comparable<TestClassResultInterface> {


Could you clarify the purpose of all these new interfaces?

The test results are stored into objects of type TestRunResults, TestClassResult and TestResult. They are returned to the ContinuousTestingJsonRPCService as a generic java.lang.Object and there transformed to JSON - then sent to the browser for UI rendering. Because my intention was to avoid passing the objects through the RPC service without any API awareness, validation..., the RPC service needs the objects typified.
Unfortunately, the module, where the JSON RPC service is placed, does not have a compile dependency to the module where the Test result types are declared. (And I did not want to change this, because this might lead to a cyclic dependency.) So I declared the interfaces (whose names might sound uncreative, because I did not want to change the implementation classes' names to avoid too much diffs) in one module where both have access to. (I'm not sure if this is the correct module, but I found the ContinuousTestingSharedStateManager there, so I think it is intended so)

Furthermore, the interfaces have the advantage that we can be aware of which informationen in required for the Continuous Testing UI, and which information is not. (that is not part of the interface) This makes further refactoring easier, because we now only have compile-time dependencies. (The test results are also used for printing out in the console for example.)

What is unclear for me: Why are the classes TestRunResults, TestClassResult and TestResultplaced into a deployment module (core-deployment)? They could also be placed into development-mode-spi, where the interfaces are now? This might avoid the need of the interfaces. (see the class diagram)

phillip-kruger · 2024-09-08T23:42:08Z

Cool ! I'll have a look once the other PR is merged. One thing to check is if this is used only by Dev UI, as this might also be used in the cli / log ? We just need to make sure that still works.

ueberfuhr · 2024-09-09T05:47:19Z

Cool ! I'll have a look once the other PR is merged. One thing to check is if this is used only by Dev UI, as this might also be used in the cli / log ? We just need to make sure that still works.

I'm aware of. For them, I only introduced the interfaces. That is the only change I made. The interfaces are more generic, e.g. their method's return types only use the interface type:

Map<String, ? extends TestClassResultInterface> getResults();

Whereas the original implementation classes implement those interface methods by returning their original return types:

@Override
public Map<String, TestClassResult> getResults() {
    return Collections.unmodifiableMap(results);
}

So this all should be compiler checked. But of course, we need to check the correct functionality.

ueberfuhr · 2024-09-12T11:46:09Z

Cool ! I'll have a look once the other PR is merged. One thing to check is if this is used only by Dev UI, as this might also be used in the cli / log ? We just need to make sure that still works.

@phillip-kruger, the other PR is merged and this branch rebased. This PR is a draft, because I'm not sure if the classes shown in the class diagram are positioned at the correct position.

Why is TestRunResults part of the deployments module?
Why is Continuous Testing in general part of the VertX extension?

phillip-kruger · 2024-09-18T10:33:55Z

I must still look at this but until then:

Why is TestRunResults part of the deployments module?

Because it's not needed at runtime. If we can do it in deployment, we should.

Why is Continuous Testing in general part of the VertX extension?

Not too sure, but because Dev UI sits in Vert.x, some things (especially core / always available) was added there.

ueberfuhr · 2024-09-19T05:03:50Z

@phillip-kruger - thanks for your notes.

I must still look at this but until then:
Why is TestRunResults part of the deployments module?
Because it's not needed at runtime. If we can do it in deployment, we should.

It's only 3 or 4 classes, not doing any logic. The logic is done at runtime (only in dev mode, where deployment modules are handled differently anyway). It is triggered by the classes in development-mode-spi, and the results are JSON-serialized in the vertx-http runtime module.

I currently would refrain from moving the classes, unless there's a value, because there are too much dependencies that would lead to much more refactoring. So the current state would be fine for me.

phillip-kruger

Thanks for this contribution. I think this is good, just one small fix then we can merge. The start/stop button should change icons depending on the state.

Before:

After (this PR) . The stop icon never display:

phillip-kruger · 2024-09-26T01:14:12Z

@ueberfuhr ^^^

- merge observers for test state and results - replace direct rendering of results and state by custom API object (DTO) - optimize size and structure of JSON message resolves quarkusio#43067

ueberfuhr · 2024-10-03T05:16:04Z

@phillip-kruger , you're right - fixed it.

quarkus-bot · 2024-10-07T04:08:15Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit c0a3be0.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

phillip-kruger · 2024-10-07T04:52:16Z

@gsmet - happy to merge ?

gsmet · 2024-10-16T13:43:23Z

Sure, let's merge!

gsmet · 2024-10-16T13:44:05Z

@ueberfuhr thanks for this awesome work and sorry it took so long for us to merge it!

ueberfuhr marked this pull request as draft September 8, 2024 06:13

quarkus-bot bot added area/core area/vertx labels Sep 8, 2024

gsmet reviewed Sep 8, 2024

View reviewed changes

ueberfuhr force-pushed the feature/refactor-continuous-testing-rpc branch 2 times, most recently from 7f23428 to 497eb45 Compare September 12, 2024 08:06

ueberfuhr force-pushed the feature/refactor-continuous-testing-rpc branch from 497eb45 to 093e274 Compare September 19, 2024 05:12

ueberfuhr marked this pull request as ready for review September 20, 2024 02:53

phillip-kruger requested changes Sep 24, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Sep 24, 2024

Refactor Continuous Testing RPC service and UI

c0a3be0

- merge observers for test state and results - replace direct rendering of results and state by custom API object (DTO) - optimize size and structure of JSON message resolves quarkusio#43067

ueberfuhr force-pushed the feature/refactor-continuous-testing-rpc branch from 093e274 to c0a3be0 Compare October 3, 2024 05:13

ueberfuhr requested a review from phillip-kruger October 3, 2024 05:15

phillip-kruger approved these changes Oct 7, 2024

View reviewed changes

gsmet merged commit 43fdb84 into quarkusio:main Oct 16, 2024
52 checks passed

quarkus-bot bot added the kind/bugfix label Oct 16, 2024

quarkus-bot bot added this to the 3.17 - main milestone Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Continuous Testing JSON/RPC #43119

Refactor Continuous Testing JSON/RPC #43119

ueberfuhr commented Sep 8, 2024 •

edited

Loading

ueberfuhr commented Sep 8, 2024

ueberfuhr commented Sep 8, 2024 •

edited

Loading

gsmet left a comment

gsmet Sep 8, 2024

ueberfuhr Sep 9, 2024

gsmet Sep 25, 2024

gsmet Sep 8, 2024

ueberfuhr Sep 9, 2024

ueberfuhr Sep 9, 2024

ueberfuhr Sep 9, 2024 •

edited

Loading

phillip-kruger commented Sep 8, 2024

ueberfuhr commented Sep 9, 2024

ueberfuhr commented Sep 12, 2024

phillip-kruger commented Sep 18, 2024 •

edited

Loading

ueberfuhr commented Sep 19, 2024 •

edited

Loading

phillip-kruger left a comment •

edited

Loading

This comment has been minimized.

phillip-kruger commented Sep 26, 2024

ueberfuhr commented Oct 3, 2024

quarkus-bot bot commented Oct 7, 2024 •

edited by github-actions bot

Loading

phillip-kruger commented Oct 7, 2024

gsmet commented Oct 16, 2024

gsmet commented Oct 16, 2024


		import java.util.List;

		public interface TestClassResultInterface extends Comparable<TestClassResultInterface> {

Refactor Continuous Testing JSON/RPC #43119

Refactor Continuous Testing JSON/RPC #43119

Conversation

ueberfuhr commented Sep 8, 2024 • edited Loading

ueberfuhr commented Sep 8, 2024

ueberfuhr commented Sep 8, 2024 • edited Loading

Improvement Details

Class Diagram

gsmet left a comment

Choose a reason for hiding this comment

gsmet Sep 8, 2024

Choose a reason for hiding this comment

ueberfuhr Sep 9, 2024

Choose a reason for hiding this comment

gsmet Sep 25, 2024

Choose a reason for hiding this comment

gsmet Sep 8, 2024

Choose a reason for hiding this comment

ueberfuhr Sep 9, 2024

Choose a reason for hiding this comment

ueberfuhr Sep 9, 2024

Choose a reason for hiding this comment

ueberfuhr Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

phillip-kruger commented Sep 8, 2024

ueberfuhr commented Sep 9, 2024

ueberfuhr commented Sep 12, 2024

phillip-kruger commented Sep 18, 2024 • edited Loading

ueberfuhr commented Sep 19, 2024 • edited Loading

phillip-kruger left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

phillip-kruger commented Sep 26, 2024

ueberfuhr commented Oct 3, 2024

quarkus-bot bot commented Oct 7, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

phillip-kruger commented Oct 7, 2024

gsmet commented Oct 16, 2024

gsmet commented Oct 16, 2024

ueberfuhr commented Sep 8, 2024 •

edited

Loading

ueberfuhr commented Sep 8, 2024 •

edited

Loading

ueberfuhr Sep 9, 2024 •

edited

Loading

phillip-kruger commented Sep 18, 2024 •

edited

Loading

ueberfuhr commented Sep 19, 2024 •

edited

Loading

phillip-kruger left a comment •

edited

Loading

quarkus-bot bot commented Oct 7, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`