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

Display JUnit Tags in Dev UI (Continuous Testing) #42988

Conversation

ueberfuhr
Copy link
Contributor

@ueberfuhr ueberfuhr commented Sep 3, 2024

I strongly use JUnits @Tag annotation to organize my tests, and I think this could be helpful to display them in the Dev UI. So I added this feature and a non-persistent checkbox to hide the tags, if needed.

See these screenshots:
Display Tags Feature (Dark Mode)
Display Tags Feature (Light Mode)

(Q is the shortcut for the io.quarkus.test.junit.QuarkusTest tag, so the user can see which test is a Quarkus test, and which one isn't.)

Hint:
The Vaadin Grid seems to recycle components, so the <qui-badge> too. However, this component is not implemented recyclable, which leads to inconsistent rendering. I have created a <qui-recyclable-badge>, that we can remove when the original badge is fixed. (see PR: qomponent/qui-badge#4)

What do you think - is this a helpful feature?

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 3, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@ueberfuhr ueberfuhr changed the title [DevUI / Continuous Testing] Display JUnit Tags Display JUnit Tags in Dev UI (Continuous Testing) Sep 3, 2024
@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch 2 times, most recently from 64161e8 to a49e41f Compare September 3, 2024 14:02
@gsmet
Copy link
Member

gsmet commented Sep 3, 2024

Sounds like an interesting addition. Can you filter on them?

/cc @phillip-kruger

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 @ueberfuhr - This is great. Let's wait for #42997 to be merged and then rebase this PR on top of that, so that you can use the badge that you fixed from qomponent.

@ueberfuhr
Copy link
Contributor Author

ueberfuhr commented Sep 4, 2024

Sounds like an interesting addition. Can you filter on them?

Currently - no. But I could give it a try, if you confirm.

Suggested Solution

I would not filter the results in the UI, but allow to restrict the test execution to a couple of selected tags, which would increase test performance.

The Quarkus Test Runtime allows to set application properties to include and exclude tags:

quarkus.test.include-tags=...
quarkus.test.exclude-tags=...

This leads to an initial configuration of the JUnit Test Runner that we could derive during runtime.

Further affected classes (first analyzation):

  • io.quarkus.devui.deployment.menu.ContinuousTestingProcessor
  • io.quarkus.devui.runtime.continuoustesting.ContinuousTestingJsonRPCService
  • io.quarkus.dev.testing.ContinuousTestingSharedStateManager.State

@gsmet
Copy link
Member

gsmet commented Sep 4, 2024

Oh my question was just about being able to filter on a tag in the UI if you have a lot of tests (i.e. you click on a tag and only the tests with this tag are displayed and then you can drop the filter). But I'm not sure it makes sense?

@ueberfuhr
Copy link
Contributor Author

IMHO: Such a client-side filter would be nice, but searching then should not only be limited to tag(s), but also to test classes and test names. Such a search form would be a new requirement.

Executing only tests with a given tag would filter the results in the UI too, but the filter then is server-side and more performant when repeating the tests. But I'm not sure if this is part of this PR, or if I should do this in another PR as a separate feature.

In summary, I would say:

  • this PR should only deal with the tag rendering in the DEV UI
  • another Issue/PR might allow to only execute tests with a given tag (or given tags)
  • another Issue/PR might introduce a filter form in Dev UI

Who can decide? 😉

@phillip-kruger
Copy link
Member

I agree. Let's get this PR in, and then we can look at more features. The client side filter should be easy to do.

@quarkus-bot

This comment has been minimized.

@ueberfuhr
Copy link
Contributor Author

#42997 is merged now, so I'll fix my code ASAP (tomorrow morning).

@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch from a49e41f to bdb91f4 Compare September 4, 2024 15:07
@phillip-kruger
Copy link
Member

Let me know when you are ready for a review

@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch from 1096aac to 481c94f Compare September 5, 2024 04:44
@quarkus-bot

This comment has been minimized.

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 !

@phillip-kruger phillip-kruger added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 5, 2024
@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch 2 times, most recently from a96b148 to 812c871 Compare September 5, 2024 09:12
@ueberfuhr ueberfuhr marked this pull request as draft September 5, 2024 09:14
@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch 2 times, most recently from 1096aac to 132ee7a Compare September 5, 2024 09:19
@ueberfuhr ueberfuhr marked this pull request as ready for review September 5, 2024 09:19
@phillip-kruger
Copy link
Member

@ueberfuhr can you squash your commits into one please. Thanks

@phillip-kruger
Copy link
Member

If there are no tags, can we disable the option to display tags and have the default to not display them ?

@quarkus-bot

This comment has been minimized.

@ueberfuhr
Copy link
Contributor Author

If there are no tags, can we disable the option to display tags and have the default to not display them ?

Yes, we can. 🙂 However, this needed a little refactoring of the QwcContinuousTesting class, because all the information about the results (incl. filtering by obj.test === true) are necessary when rendering the menu bar. Have tested this with 2 of my projects.

If I only have tests without any tag, it looks like this:

without tags

As soon as I introduce at least one tag:

with tag

@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch from a5155e1 to b302b93 Compare September 6, 2024 05:23
@ueberfuhr
Copy link
Contributor Author

@ueberfuhr can you squash your commits into one please. Thanks

Sure, done.

@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch from b302b93 to b73905e Compare September 6, 2024 09:58
@ueberfuhr
Copy link
Contributor Author

I did a small change based on the findings from #43067.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Sep 6, 2024

@brunobat not sure what's going on but OpenTelemetryInjectionsTest has started to become extremely flaky.

@brunobat
Copy link
Contributor

brunobat commented Sep 9, 2024

@brunobat not sure what's going on but OpenTelemetryInjectionsTest has started to become extremely flaky.

@gsmet created this: #43142

@ueberfuhr ueberfuhr force-pushed the feature/continuous-testing-display-tags branch from f5c4bdb to 9ee1db7 Compare September 9, 2024 22:27
@quarkus-bot

This comment has been minimized.

@ueberfuhr
Copy link
Contributor Author

@gsmet, is this PR now ready to merge, or is anything missing yet?

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.

Nice work!

There's just one last micro thing and then it's good to go.

@maxandersen
Copy link
Member

Oh nice one. Tags is underrated so nice to see it in Devui.

Should work nicely with https://quarkus.io/guides/getting-started-testing#running-specific-tests

Maybe a thing for the list of devui improvements for insights (or future one) @phillip-kruger

@phillip-kruger
Copy link
Member

Yes very cool. I'll show this on insights

- introduce `tags` column
- introduce toggle to hide the `tags`column
- automatically hide `tags` column when JUnit tags are not used
@ueberfuhr
Copy link
Contributor Author

@maxandersen @gsmet - for filtering tests for execution or for rendering, I've created #43208 for a discussion and a decision. 🙂

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 11, 2024

Status for workflow Quarkus CI

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

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


Flaky tests - Develocity

⚙️ Native Tests - HTTP

📦 integration-tests/rest-client-reactive

io.quarkus.it.rest.client.BasicTestIT.shouldCreateClientSpans - History

  • expected: <1> but was: <2> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <1> but was: <2>
	at io.quarkus.it.rest.client.BasicTest.shouldCreateClientSpans(BasicTest.java:216)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:810)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

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.

Awesome contribution, thanks!

@gsmet gsmet merged commit a9ee070 into quarkusio:main Sep 11, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 11, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 11, 2024
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.

6 participants