Repository navigation
fix: tolerate a 1s timing tie in test-run-all-timing.sh (#482) - #486
Merged
Merged
Conversation
test-b-slow.sh now sleeps 2s instead of 1s, widening the margin against a loaded CI runner crossing a second boundary on the near-instant fixture. The ordering assertion also compares the two suites' printed times: when they differ, the strict slow-before-fast line-position check still applies (the positive control against a real sort regression); when they tie at 1s resolution, the order is genuinely undefined, so the check reports it as an inconclusive skip rather than a flaky failure. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Review (Explore) found the tie-detection added for #482 was too permissive: comparing only the two suites' parsed printed times meant a stub that always printed the same constant (e.g. "1s" for every suite, a broken $SECONDS measurement) would be read as a legitimate tie and silently SKIPPED rather than failed. test-b-slow.sh sleeps a full 2 seconds, so a correct measurement can never read below 2 for it -- that is now asserted unconditionally, before the tie check runs, so a stuck/stubbed timer fails hard instead of sliding into the tie-tolerant branch. The same review also noted the closing tally line (PASS/TOTAL passed, FAIL failed) silently excluded a tie from its denominator with no indication anything was skipped. Added a SKIP counter and a skip() helper alongside ok()/bad(), and the closing line now names the skip count when one occurred. Co-Authored-By: Claude Sonnet 5 <[email protected]>
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.
Closes #482
On a loaded Windows runner,
tests/test-run-all-timing.sh's ordering assertion could fail on a genuine whole-second-resolution tie between the fast and slow fixtures:run-all.sh's timing block sorts on a single key (sort -t ... -k1,1rn, no secondary key), so a tie falls back to name order andtest-a-fast.shsorts beforetest-b-slow.sheven though it is not actually slower.Two changes, both suggested in the issue:
test-b-slow.shnow sleeps 2s instead of 1s, so a tie needs the fast fixture to straddle two second boundaries instead of one.A guard was added so a tie can never mask a broken timer: test-b-slow.sh sleeps a full 2 seconds, so a correct measurement can never read below 2 for it. If the slow suite's own printed time is below 2s, that is a hard FAIL (not a tie, a broken timer), not a SKIP -- this closes a self-review finding where a stuck-at-1s stub for every suite would otherwise have been silently absorbed into SKIPPED.
Verified: reproduced the exact pre-fix failure mode with a synthetic tied-at-1s fixture (matches the issue's own reported slow_line=10 fast_line=9 shape); confirmed the new logic reports SKIP instead on the same fixture; confirmed a synthetic genuine-regression fixture (distinct times, reversed order) still fails. bash tests/test-run-all-timing.sh: 13/13 passed. Adjacent bash tests/test-run-all-shard.sh: 16/16 passed, no regression. python3 .oss/assemble_changelog.py --check: exit 0.
Self-review (two independent spawns) found two issues with the first draft, both fixed in a follow-up commit: the tie-tolerant SKIP branch could not distinguish a genuine tie from a broken timer (fixed via the SLOW_SECS >= 2 guard above), and the closing tally line silently excluded SKIPPED checks from its denominator (fixed via the new SKIP counter and tally line).
[AI-generated]