--show-progress=running enhancements #2729
Conversation
With fast running tests, it avoids showing a partially filled list of tests that can lead the user to think not all the possible tests are running in parallel.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2729 +/- ##
==========================================
+ Coverage 77.37% 77.39% +0.02%
==========================================
Files 112 112
Lines 25893 25886 -7
==========================================
Hits 20034 20034
+ Misses 5859 5852 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let multi_progress = MultiProgress::new(); | ||
| multi_progress.set_draw_target(Self::stderr_target()); | ||
| multi_progress.set_move_cursor(true); | ||
| // multi_progress.set_move_cursor(true); |
There was a problem hiding this comment.
In my testing I found that set_move_cursor(true) made things a bit better -- did it make things worse for you?
There was a problem hiding this comment.
Enabling set_move_cursor(true) causes a visual bug where the end of a longer, previously drawn line isn't cleared when a shorter line overwrites it.
It was not visible before because of the total redraw when calling suspend()
|
|
||
| /// The refresh rate for the progress bar, set to half the tick interval in hz. | ||
| pub const PROGRESS_REFRESH_RATE_HZ: u8 = 10; | ||
| pub const PROGRESS_REFRESH_RATE_HZ: u8 = 20; |
There was a problem hiding this comment.
Hmm, I feel like this might be too fast especially over a slower connection like sshing into a remote.
I think setting both to be the same might cause weird issues where both are slightly out of sync with each other. In any case the comment here needs to be updated.
There was a problem hiding this comment.
I've reduced the refresh rate to 10 Hz. I'm still experimenting with the tick interval.
| TestEventKind::TestShowProgress { | ||
| test_instance, | ||
| running, | ||
| .. | ||
| } => { |
There was a problem hiding this comment.
Hmm, again worried about this overwhelming the display with fast test suites like clap over a slower connection.
| // suspend forces a full redraw, so we call it only if there is | ||
| // something in the buffer | ||
| if !buf.is_empty() { | ||
| self.multi_progress | ||
| .suspend(|| std::io::stderr().write_all(buf)) | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This seems like a good change.
|
Something I noticed while testing this out is that we'd show "n-1 tests running" (e.g. 7 tests running or 31 tests running) a lot more than we do on main. |
Yes, I've noticed that too. I wonder if this is because of the blank line we're adding that may prevent the test from appearing. I'm preparing a change that adds the blank lines only before using |
|
I think we may need to improve our event model a bit — currently, when a test completes and the next test is started we send two separate events. I think we may need to coalesce them down into one event so that the number of running tests stays the same. (I learned this lesson in another event protocol I built later.) |
this helps to reduce the flickering
glehmann
force-pushed
the
gln/reduce-flicker-smooth-display-llsm
branch
from
4ff7914 to
5944924
Compare
Yes, it should help! |
|
Creating a bar with spaces only when writing a message to the user with |
Fascinating, is that because the added bar is the very last one and there's some weird logic in indicatif around the last bar? |
2 participants
ref #2720