Repository navigation
Balance coverage shards on Linux test times - #1806
Mikola Lysenko (mikolalysenko) merged 4 commits into
Conversation
ci-test-shard.py now echoes cargo's merged output and records each test binary's `finished in` time, then prints one `ci-test-shard: timings` JSON line at the end of the shard, with the build time up to the first test binary. The log tail is all the log API returns, so this is how a Linux coverage runtime sample can be read back: the only sample in the repo is from Windows, which models Linux coverage badly (#1768). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeQpFXjqvowuU1RFbdQ
|
bugbot run Generated by Claude Code |
coverage (2) sets the merge-queue critical path: with the count split its test step ran 248 s against 173 s for shard 1 on this PR's first run, and 307-335 s against 176-179 s in recent merge_group runs. #1768 balanced on the Windows sample and saved nothing because Windows times don't model instrumented Linux targets (e2e_vendor_pypi_build: 35-72 s on Linux, 0 s in the Windows sample). ci-coverage-durations.json is this PR's own first coverage run, read back from the new timings line. unit_seconds adds ~34 s to the unit runtime for building the unit-test harnesses, and compile_seconds is 0.3 s, both fitted from the two shards' build and report times. Coverage uses it unless CI_TEST_TIMINGS is set. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeQpFXjqvowuU1RFbdQ
Review fixes for the timings capture: forward cargo's output as raw bytes (Windows decoded UTF-8 as cp1252 and could raise on write), stop reading 10 s after cargo exits so a daemon a test left holding the pipe can't hang the shard, strip ANSI colour before matching, and return the first-binary time from run() instead of a sentinel key. The first balanced run overshot: shard 1's test step went 173 -> 229 s and shard 2's 248 -> 202 s. unit_seconds goes up by that 27 s gap. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeQpFXjqvowuU1RFbdQ
|
bugbot run Generated by Claude Code |
If echoing to stdout failed (BrokenPipeError when tee dies), the reader thread exited and cargo blocked on a full pipe, so proc.wait() never returned and the shard sat until the job timeout. The reader now stops echoing but keeps reading and recording timings. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011f9aeQpFXjqvowuU1RFbdQ
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 489db94. Configure here.
|
Ready for review at 489db94.
Generated by Claude Code |
Final review briefWhat it does: Coverage shards were split by test-target count, which left Risk: Low. It's CI-only. Exit statuses are unchanged, and each target still runs in exactly one shard. The only behaviour change outside coverage is that cargo's stderr is now merged into stdout and forwarded by a Python thread. The measured saving is modest (~0.2 min on fast hosts). Look here:
Verified:
Changes I made: none. Open questions: none blocking. The sample comes from one run and was refreshed by hand. The PR already lists that and the hyphenated-target-name limitation as known. Auto-merge is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
Self-picked from the CI performance dashboard (#1182). No
ci-perfissue was eligible this run: #1670, #1512 and #1248 are settings issues, and #1720 isagent:needs-human. This retries #1768 the way its closing comment asked, with a Linux coverage timing sample.Problem
coverage (2)sets the merge-queue critical path: it finished last in 9 of 10 merge_group runs on the dashboard at 10-29 00:16. The coverage shards use the count split, and shard 2 is much slower than shard 1:coverage (1)job / test stepcoverage (2)job / test step#1768 balanced on
scripts/ci-test-durations.jsonand saved nothing, because that sample was measured on Windows and Windows times don't predict instrumented Linux runs. Two examples:e2e_vendor_pypi_buildtakes 35–72 s under Linux coverage but 0.0 s in the Windows sample, ande2e_vex_lockfiletakes 11–42 s on Linux vs 119 s.Change
scripts/ci-test-shard.pyforwards cargo's merged output byte for byte and records each test binary'sfinished intime. At the end of the shard it prints oneci-test-shard: timings {...}JSON line with the build time. The log API returns only the last 5,000 lines, so this is the only way to read a full sample back. It also applies to the Windows, macOS and release shards, which makes refreshing their samples possible too.scripts/ci-coverage-durations.json: all 234 targets, from this PR's first coverage run (40801155956).unit_secondscovers the unit-test runtime plus building the unit-test harnesses, and was tuned once against run 2.compile_secondsis 0.3, fitted from the two shards' build and report times.coveragepartitions on that sample, using the longest-first split the Windowstestshards already use, unlessCI_TEST_TIMINGSis set. An emptyCI_TEST_TIMINGSkeeps the count split./code-review high:UnicodeEncodeErroron cp1252.ci.yml.Expected saving
Balancing the shards' test steps at about (173 + 248) / 2 ≈ 210 s should cut ~0.5 min per PR and merge-queue run on fast hosts. Slow hosts should save more, since shard 2 had the slow, network-bound targets: in the mg runs above, its step took 307–335 s. Job-minutes stay about the same.
Measured result
Same PR, same workflow, fast Depot hosts:
coverage (1)job / stepcoverage (2)job / stepe2e_vendor_pypi_build39.9 vs 34.9 s,e2e_vendor_pnpm_build15.2 vs 12.6 s. The saving should be larger on slow hosts, where shard 2 took 307–335 s with the count split.coverage (2)no longer sets the mg critical path and that mgci-okp50 (6.53 at 10-29 00:16) drops.Where each test runs
Unchanged. Every integration target still runs in exactly one coverage shard (
test_ci_test_shard.py). Targets added after the sample fall back todefault_seconds, and a test fails once more than 10% of targets are missing from the sample.Review findings not fixed
cargo metadata).Risk
Low. Exit statuses are unchanged and all 380
scripts/teststests pass. All three CI runs on this PR were green.https://claude.ai/code/session_011f9aeQpFXjqvowuU1RFbdQ
Generated by Claude Code