Skip to content

Relax wall-clock bound in canonical capture timeout test - #1245

Merged
malpern merged 1 commit into
masterfrom
claude/vibrant-maxwell-f6f9c1
Jul 29, 2026
Merged

Relax wall-clock bound in canonical capture timeout test#1245
malpern merged 1 commit into
masterfrom
claude/vibrant-maxwell-f6f9c1

Conversation

@malpern

@malpern malpern commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

  • The Swift Testing test "Canonical capture timeout returns first-class timed-out evidence" asserted started.duration(to: clock.now) < .milliseconds(250) against a real ContinuousClock. On a loaded CI runner it measured 268 ms and failed PR Add per-key wear and highlight variation to the keyboard stage #1243's build-and-test job (run 30470376137) despite being unrelated to that change.
  • SystemValidator.boundedCapture has no injectable clock (its timeout uses Task.sleep on the default clock), so rather than a larger refactor, the assertion keeps the real-clock measurement but widens the bound to a generous 5-second hang guard with a comment. The test still fails if the timeout path hangs instead of promptly returning first-class timed-out evidence, and the captureStatus == .timedOut assertion is unchanged.

Testing

  • TEST_FILTER=SystemValidatorTests ./Scripts/run-tests-safe.sh — all 17 tests passed.

Review gate

  • ./Scripts/review-gate.sh: remote review gate selected; GitHub claude-review must pass before merge.

🤖 Generated with Claude Code

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Widening the upper bound from 250ms to 5s (with the explanatory comment) is a reasonable fix for CI flakiness, but it substantially weakens this assertion's ability to catch a real regression — a runaway boundedCapture that overruns its 10ms deadline by, say, 1-2s would now pass silently. If flakiness was the driver, consider a middle-ground bound (e.g. 1-2s) or CI-only widening via an environment check, to keep some signal against a hang while still tolerating scheduler jitter. No other correctness/reliability issues in this diff.

The 250ms bound measured real elapsed time and failed on a loaded CI
runner (268ms in PR #1243's build-and-test job) despite being unrelated
to the change under test. boundedCapture has no injectable clock, so
keep the real-clock measurement but widen the bound to a generous
5-second hang guard: it still fails if the timeout path hangs instead
of returning first-class timed-out evidence, without depending on
scheduler latency.

Co-Authored-By: Claude Fable 5 <[email protected]>
@malpern
malpern force-pushed the claude/vibrant-maxwell-f6f9c1 branch from 23c9546 to 7d64407 Compare July 29, 2026 16:56
@malpern
malpern enabled auto-merge (squash) July 29, 2026 16:56
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Reviewed the diff — no blocking issues. One minor observation: widening the bound from 250ms to 5s fixes the flakiness on loaded runners, but it also means the test would now pass even if boundedCapture's actual deadline enforcement regressed to, say, 1-4s instead of its documented 10ms — i.e. the test can no longer catch a meaningful mid-range latency regression, only a true hang. If that trade-off is intentional for stability, no changes needed; just flagging it as reduced test-coverage precision.

@malpern
malpern merged commit 9e0d6d1 into master Jul 29, 2026
3 checks passed
@malpern
malpern deleted the claude/vibrant-maxwell-f6f9c1 branch July 29, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant