fix(test): the artifact block's timeout belongs to the block, not one case (#542) - #546
Merged
Merged
Conversation
… case (#542) Second pass. My first fix sized the budget from the ONE case #542 named - the heaviest, 12 tasks at FARM_CONCURRENCY 6 - and left the rest of the block on the 5000ms default. SEVEN cases then timed out together on windows-latest, and the ca coverage-union cell went red again on the very next PR. The slow thing was never that case's concurrency. It is that EVERY case in this block launches real farm subprocesses, and process creation on a hosted Windows runner is far slower than on a developer box. Sizing from the one case the report happened to name was always going to find the next-slowest case next. Budget moved to the describe block; the redundant per-case timeout is removed, because two sources of truth for one budget is how they drift apart. 30s remains ~6x the slowest measured run and is still bounded, so a genuine hang fails rather than sitting until the job timeout. Test-only: farm.js rebuilds unchanged and the payload gate agrees, so no version bump. Claude-Session: https://claude.ai/code/session_01WJgVfZw7J81PB7mwpHyUxx
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.
Reopens and completes #542.
What I got wrong the first time
#542 reported one timing-out case, so I measured that case and gave it a budget. It was the heaviest in the file — 12 tasks at
FARM_CONCURRENCY6 — and the numbers were real:But the rest of the block stayed on the default, and on the next PR seven cases in it timed out together on
windows-latest, turning the ca coverage-union cell red again.The actual cause
The slow thing was never that case's concurrency. Every case in
farm artifact publicationlaunches real farm subprocesses, and process creation on a hosted Windows runner is far slower than on a developer box. The heaviest case simply crossed the line first.Sizing from the one case the report named was always going to find the next-slowest case next.
The fix
The budget belongs to the block. The redundant per-case timeout is removed — two sources of truth for one budget is how they drift apart.
30s remains ~6× the slowest measured run and is still bounded, so a genuine hang fails rather than sitting until the job timeout. The cases themselves are untouched:
FARM_CONCURRENCYstays 6 and every assertion stands (#515 AC-4, #542 AC-2).Verification
farm.test.ts45 passed, typecheck clean.farm.jsrebuilds unchanged and the payload gate confirms no shipped payload change, so no version bump.The real proof is this PR's own
Coverage union <os: windows-latest>cell, which is what caught the incomplete fix.