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

Refactor Continuous Testing JSON/RPC #43119

Conversation

ueberfuhr
Copy link
Contributor

@ueberfuhr ueberfuhr commented Sep 8, 2024

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 ueberfuhr marked this pull request as draft September 8, 2024 06:13
@ueberfuhr
Copy link
Contributor Author

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
Copy link
Contributor Author

ueberfuhr commented Sep 8, 2024

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):

Class diagram

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

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.

import org.junit.platform.engine.TestExecutionResult;
import org.junit.platform.engine.TestSource;
import org.junit.platform.engine.UniqueId;
import org.junit.platform.engine.*;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


import java.util.List;

public interface TestClassResultInterface extends Comparable<TestClassResultInterface> {
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify the purpose of all these new interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ueberfuhr ueberfuhr Sep 9, 2024

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

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 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
Copy link
Contributor Author

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
Copy link
Member

phillip-kruger commented Sep 18, 2024

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
Copy link
Contributor Author

ueberfuhr commented Sep 19, 2024

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

@ueberfuhr ueberfuhr force-pushed the feature/refactor-continuous-testing-rpc branch from 497eb45 to 093e274 Compare September 19, 2024 05:12
@ueberfuhr ueberfuhr marked this pull request as ready for review September 20, 2024 02:53
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

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:
stop_before

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

This comment has been minimized.

@phillip-kruger
Copy link
Member

@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 ueberfuhr force-pushed the feature/refactor-continuous-testing-rpc branch from 093e274 to c0a3be0 Compare October 3, 2024 05:13
@ueberfuhr
Copy link
Contributor Author

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

Copy link

quarkus-bot bot commented Oct 7, 2024

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
Copy link
Member

@gsmet - happy to merge ?

@gsmet
Copy link
Member

gsmet commented Oct 16, 2024

Sure, let's merge!

@gsmet gsmet merged commit 43fdb84 into quarkusio:main Oct 16, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 16, 2024
@gsmet
Copy link
Member

gsmet commented Oct 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continuous Testing RPC Service in Dev UI returns inconsistent JSON
3 participants