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

Continuous Testing RPC Service in Dev UI returns inconsistent JSON #43067

Closed
ueberfuhr opened this issue Sep 6, 2024 · 5 comments · Fixed by #43119
Closed

Continuous Testing RPC Service in Dev UI returns inconsistent JSON #43067

ueberfuhr opened this issue Sep 6, 2024 · 5 comments · Fixed by #43119

Comments

@ueberfuhr
Copy link
Contributor

Describe the bug

In case of one skipped JUnit test, the JSON returned from the backend to the Dev UI does not contain the skipped test in the top-level skipped property. However, it is contained within the results.<test-class>.skipped property. Is this a bug? Sure, it's a minor one, but Dev UI has to be aware of this.

Expected behavior

I would expect the skipped test to be part of both properties.

Actual behavior

The skipped test does not occur in the top-level skipped array.

How to Reproduce?

  1. Mark a test to be @Disabled and run the tests
  2. In Dev UI, debug the QwcContinuousTesting._results property

Output of uname -a or ver

Darwin head 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6030 arm64

Output of java -version

openjdk version "21.0.4" 2024-07-16 OpenJDK Runtime Environment Homebrew (build 21.0.4) OpenJDK 64-Bit Server VM Homebrew (build 21.0.4, mixed mode, sharing)

Quarkus version or git rev

HEAD

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)

Additional information

No response

Copy link

quarkus-bot bot commented Sep 6, 2024

/cc @cescoffier (devui), @phillip-kruger (devui), @stuartwdouglas (continuous-testing)

@ueberfuhr
Copy link
Contributor Author

This affects #42988 (not a blocker, but I need to be careful there)

@ueberfuhr
Copy link
Contributor Author

Screenshot

@ueberfuhr
Copy link
Contributor Author

ueberfuhr commented Sep 6, 2024

Seems to be this code in io.quarkus.deployment.dev.testing.TestRunResults#<init>:

            if (current) {
                if (!i.getValue().getFailing().isEmpty()) {
                    currentFailing.put(i.getKey(), i.getValue());
                    failing.add(i.getValue());
                } else if (!i.getValue().getPassing().isEmpty()) {
                    currentPassing.put(i.getKey(), i.getValue());
                    passing.add(i.getValue());
                } else {
                    skipped.add(i.getValue());
                }
            } else {
                if (!i.getValue().getFailing().isEmpty()) {
                    historicFailing.put(i.getKey(), i.getValue());
                    failing.add(i.getValue());
                } else if (!i.getValue().getPassing().isEmpty()) {
                    historicPassing.put(i.getKey(), i.getValue());
                    passing.add(i.getValue());
                } else {
                    skipped.add(i.getValue());
                }
            }

I don't understand the requirement here, but this code does the following:

  1. If there is at least one failing test in the test class, all test class results are placed into the failing array.
  2. If there isn't any failing, but at least one passing test in the test class, all test class results are placed into the passing array.
  3. Otherwise, all test class results are placed into the skipped array.

So we can assume that

  • The skipped array only contains skipped tests.
  • The passing array containts both passing and skipped tests.
  • The failing array contains all failing, passing and skipped tests
  • There isn't any duplicate all over the arrays.

This leads to skipped tests not to be placed in the skipped array unless there are passing or failing tests in the same test class. 😵‍💫

I find this confusing, and depending from where this is used, I guess we should refactor this.

  • If those three arrays are not mandatory, just remove them. We have a results array where all results are included.
  • If needed, we should split the test class entries, so that the three arrays only contain the results they stand for.

Here's a sample JSON of the _results object:
_results.json

@phillip-kruger
Copy link
Member

I am not sure, this is code from before the new Dev UI.

ueberfuhr added a commit to ueberfuhr-opensource-forks/quarkus that referenced this issue Sep 9, 2024
 - 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 added a commit to ueberfuhr-opensource-forks/quarkus that referenced this issue Sep 12, 2024
 - 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 added a commit to ueberfuhr-opensource-forks/quarkus that referenced this issue Sep 19, 2024
 - 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
@gsmet gsmet closed this as completed in c0a3be0 Oct 16, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 16, 2024
bschuhmann pushed a commit to bschuhmann/quarkus that referenced this issue Nov 16, 2024
 - 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants