Add --show-progress and serialize worker output to fix interleaving#773
Open
MatthewMckee4 wants to merge 5 commits into
Open
Add --show-progress and serialize worker output to fix interleaving#773MatthewMckee4 wants to merge 5 commits into
MatthewMckee4 wants to merge 5 commits into
Conversation
MatthewMckee4
added
diagnostics
Merging this PR will degrade performance by 12.3%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | karva_benchmark |
50.2 s | 57.2 s | -12.3% |
Comparing worktree-fluttering-growing-rose (6fe0775) with main (8d4f87c)
MatthewMckee4
changed the title
Workers were spawned with Stdio::inherit(), so any byte they wrote directly to FD 1 — Python print() in a user test, panic messages, etc. — landed on the orchestrator's terminal without going through the file-based output drain. When that hit FD 1 in the middle of an orchestrator writeln!, the worker's PASS line came out spliced through foreign bytes (issue #502 in another guise). Spawn workers with Stdio::piped() for stdout, take the read end at spawn time, and hand it to a per-worker reader thread inside OutputDrain. The reader pushes whole lines through an mpsc channel that drain_loop polls alongside the per-worker output files; both sources emit through the same bar.suspend() path so nothing tears against the bar either. Stderr stays inherited because the orchestrator also writes tracing to stderr in real time and the drain's poll interval would shuffle the relative order of those lines. Two follow-on fixes were needed for ordering: The drain now reads the pipe channel before polling the result files in each iteration. A print() runs before the test returns and the reporter writes its PASS line, so within one iteration the pipe line should land in the emit batch ahead of the file line. attach_with_output now reconfigures sys.stdout / sys.stderr to line-buffer when show_output is true. Python defaults to block buffering when its stdout isn't a TTY; with the pipe in place that delayed every print() until the interpreter shut down, which is after the reporter had already emitted PASS.
Worker stdout is now a pipe, so the worker's TTY-based auto-detection always disables ANSI — colors got dropped on the way through the drain even when the orchestrator's stdout was a terminal. Forward an explicit --color=always or --color=never to every worker based on the orchestrator's own should_colorize() check (or the user's --color override if one was given). The drain pipes bytes through verbatim, so the orchestrator-side TTY decision is the one that should drive colorization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a live progress display while tests run, configurable via
--show-progress(none|counter|bar) or[terminal] show-progressinkarva.toml. The display is rendered on stderr byindicatifso it does not interfere with per-test result lines on stdout.Closes #570.
In the course of wiring this up, the per-test result lines themselves had a longstanding bug: in parallel runs, multiple worker processes lock their own stdouts independently, which does not actually serialize their writes. Lines from different workers could splice mid-line, producing output like
... oktest tests.test_currency.... Closes #502.This PR fixes the interleave at the source. Workers no longer write reporter lines directly to the inherited stdout — instead, each worker writes newline-delimited preformatted lines to a per-worker
outputfile in its cache directory (via a newFileLineSink), and the orchestrator runs a background thread that drains those files, splits on\n, and prints whole lines to its own stdout. Single-writer per file plus newline-bounded reads means partial-line splices are impossible.The same drain thread also drives the optional progress bar from the existing per-worker
progressfiles. The bar is sized bytest_count()(function definitions) and ticked once per test function — variants from@parametrizeno longer each tick, so the bar reaches 100% exactly when the run finishes regardless of how many cases each function expanded into.The bar style now matches ty's:
{msg:8.dim} {bar:60.green/dim} {pos}/{len} tests,progress_chars("--"). Output writes usebar.suspend(|| writeln!(stdout, …))so the bar's stderr redraws don't clash with stdout writes.Test Plan
ci