Repository navigation
test(layout): load the runtime graph once before the layout capability tests - #33923
Merged
Merged
Conversation
…y tests The first test paid the cold transform of the whole runtime graph inside its 30 s budget and timed out whenever a shard placed it without warm neighbors.
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 of 5 tasks
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
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.
ELI5
The first test in
workspace-layout-paired-desktop.test.tshas to load the whole runtime cold, inside its 30 s limit. On a slow CI shard that load takes longer than 30 s, and the test times out even though nothing is wrong. Now the file does that load once, before its tests start.What Changed
beforeAll: it runsswitchedBuild(true)once, with a 120 s budget. The hook has a short comment saying why.orca-runtime, the dispatcher and the methods index inside its own 30 s budget.switchedBuildcalls stillvi.resetModules()and re-evaluate each build, but they reuse the transforms; in CI those tests already take about 1.5–2.5 s each. Assertions are unchanged.Why
#33897 failed this test three times in shard 1/5 (runs 40307544019, its rerun, and 40316052149). Meanwhile #33907 passed it with 22.4 s of its 30 s used.
Why it tipped over only on #33897: CI places test files into shards by weighted timing. #33897 adds a new test file with no timing history, which reshuffled shard 1. I compared the file lists of the two shard-1 runs: about 1040 of roughly 2650 files differ. The new file itself is not in shard 1 and doesn't import the runtime.
So the change is in this file's neighbors, not in what it loads. That points at a cold-import margin of a few seconds, which any reshuffle can push past 30 s.
Raising the test timeout alone would hide future slowdowns of the test itself. Loading once in a hook keeps each test's 30 s budget meaningful.
Linked Issue
Unblocks #33897.
Visual Proof
N/A: test-only change.
Testing
pnpm tcandpnpm run check:code-quality:changedpass.Locally the file can't finish at the Mac's current load (average around 80). Every
switchedBuildexceeds its budget on this machine both with and without this change, so CI is the check here.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
Notes
Test-only change: no product, SSH or wire impact.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)