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

Print pending groups in-line and include their reason, also add progress bar to the runner #9796

Merged
merged 7 commits into from
Apr 26, 2024

Conversation

radeusgd
Copy link
Member

Pull Request Description

  • Closes Bring back printing pending test groups #9534 by printing the pending groups with pending reason
  • Re-introduces original ordering of tests
  • Adds a progress bar to the test suite runner in 'interactive' mode (if ANSI colors are enabled, progress bar will also be)

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 26, 2024
@radeusgd radeusgd self-assigned this Apr 26, 2024
@radeusgd
Copy link
Member Author

  • Re-introduces original ordering of tests

I can revert this if anyone objects.

The rationale for me is that it is easier to find tests in my code if they have the same ordering in the report - there is a 'spatial' analogy between both. I was finding it much harder to find tests after the ordering was changed.

We could add an option to randomize the ordering on CI if requested.

@radeusgd
Copy link
Member Author

  • Adds a progress bar to the test suite runner in 'interactive' mode (if ANSI colors are enabled, progress bar will also be)

I really wanted this for a long time. Now it is clearer how the execution is progressing and if execution is stuck on a specific test case.

progress-bar.mp4

Comment on lines 80 to 81
# If ANSI colors are enabled, we assume an interactive terminal and will print progress
progress_reporter = case config.use_ansi_colors of
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit smelly: I'm overriding the meaning of use_ansi_colors to generally mean that the application is run in an 'interactive' terminal session.

Shall we rename this setting to interactive_terminal? It also does not have the same meaning.

We could introduce a separate setting for this but I think that is adding unnecessary complication - in general both colors and progress bars are going to be used in the same setting - on a dev machine, but not on CI.

Copy link
Member

Choose a reason for hiding this comment

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

interactive_terminal definitely sounds more appropriate.

in general both colors and progress bars are going to be used in the same setting - on a dev machine, but not on CI.

Colors are used on the CI. And I think that in helps with readability. At least it helps to me. I would like to retain the colors on the CI, and not print the progress bar on the CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I was somehow sure that colors are not used on CI. My mistake. Of course the progress bar should not run on CI. I will try a different approach then.

@radeusgd radeusgd requested a review from Akirathan April 26, 2024 11:12
@radeusgd
Copy link
Member Author

image

Pending groups are displayed like they used to be, now also with a grey color.

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Progress reporting looks cool and useful. I had no idea one can implement that with such ease. Printing reasons for pending tests is also useful. Bringing back the original ordering by assembling the filtered tests into a nested list, rather than via Map is also OK. I would only like the coloring to be retained on the CI, but not print any progress bar, obivously.

Comment on lines 80 to 81
# If ANSI colors are enabled, we assume an interactive terminal and will print progress
progress_reporter = case config.use_ansi_colors of
Copy link
Member

Choose a reason for hiding this comment

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

interactive_terminal definitely sounds more appropriate.

in general both colors and progress bars are going to be used in the same setting - on a dev machine, but not on CI.

Colors are used on the CI. And I think that in helps with readability. At least it helps to me. I would like to retain the colors on the CI, and not print the progress bar on the CI.

Java_System.out.print '\r'

## PRIVATE
type Ignore_Progress_Reporter
Copy link
Member

Choose a reason for hiding this comment

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

Please ensure that Ignore_Progress_Reporter is used on the CI, but the coloring is still there.

Copy link
Member Author

Choose a reason for hiding this comment

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

In 7aeecf2 I have changed it so that the progress bar is displayed if System.console() != null. This should be enough to avoid it running on CI or piped and better than adding yet another env var.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have inspected the runs.

Indeed before the progress was displayed (badly) on CI:
image

But after the above commit all seems fine again:
image

I think the automatic detection of console is even better than an env var.

@radeusgd radeusgd requested a review from Akirathan April 26, 2024 15:58
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Apr 26, 2024
@mergify mergify bot merged commit 8e03e3b into develop Apr 26, 2024
34 of 36 checks passed
@mergify mergify bot deleted the wip/radeusgd/9534-test-pending branch April 26, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back printing pending test groups
4 participants