Conversation
isaacs
requested review from
logaretm,
mydea and
s1gr1d
and removed request for
a team
September 16, 2026 22:41
Contributor
size-limit report 📦
|
s1gr1d
reviewed
Sep 17, 2026
s1gr1d
left a comment
Member
There was a problem hiding this comment.
Thanks for making the tests faster!!!
isaacs
force-pushed
the
isaacs/de-flake-slow-tests
branch
from
September 17, 2026 15:14
d786a06 to
e116423
Compare
Three unrelated flakes, all of which pass locally and fail only under CI load. **Tests that time out loading the SDK** Several unit tests time out in CI on the first test in a file. The first dynamic import of a large SDK graph pays for transforming it, and that cost is charged to whichever test triggers it. Locally it is a few hundred milliseconds. On a loaded runner it crosses the 5000ms test timeout. Swept every unit test in the repo and fixed the files that show the pattern. Fixed with a bare side-effect `import` at the top of the file. Collection is not bounded by the test timeout, so pulling the load into it takes the cost off the test without changing what the test exercises. Exception: `nuxt vite/orchestrion` has to register its `vi.doMock` calls before the module under test loads, and a static import is hoisted above them, so it warms the graph from `beforeAll` instead. | file | before | after | | -------------------------------------- | -----: | ----: | | browser-utils inp | 594ms | 9ms | | browser-utils metricObserverOrdering | 409ms | 22ms | | nextjs appRouterRoutingInstrumentation | 750ms | 233ms | | nuxt vite/orchestrion | 536ms | 1ms | | sveltekit index.workerd | 625ms | 8ms | | tanstackstart-react client/tunnelRoute | 433ms | 1ms | Which file pays is not fixed: the two browser-utils files traded places between runs, because the transform lands on whichever test reaches the graph first. Three tests in these packages still run over 400ms, and all three are slow for a reason: - The two nextjs `appRouter` tests busy-wait 1100ms on purpose. - `conflictingDebugOptions` re-evaluates three SDK entry points for each `debug-build` mock, which no warmup can avoid. **INP span never sent** `hidePage` waits for one `requestIdleCallback` before hiding, assuming web-vitals has already queued its own idle callback and will therefore run first. But web-vitals only queues its callback once the click's Event Timing entry reaches its observer, which is after `hidePage` queues its own. So `hidePage` wins the race and hides the page too early. That loses the interaction. The forced report web-vitals does on `visibilitychange` runs while the metric is still -1, and the pending callback that sets the real value afterwards reports unforced. No INP is emitted at all and the test waits out its 30s timeout. Fixed by waiting for the entry and queuing the idle callback from a task after it, which puts web-vitals' callback first. Callers that never interacted skip the wait, so the 13 that only need the page hidden pay nothing. The react-router-6 e2e suite now passes 10/10 and drops from 1.4m to ~53s. **Cron job that never runs** Two bugs in `suites/cron/cron`, both reproducible under CPU load. `cron`'s `start()` sizes the delay to the next tick from two clock reads, clamps a negative result to -1, and then treats that -1 as "stop". A pause between the two reads that straddles a second boundary is enough. The job is left scheduled for nothing, and the scenario exits on its own 15s timer having sent nothing. That is the reported failure, which reads as `0/5 expected envelope(s)` on a clean exit 0 with no captured output. A probe measured that clamp firing 5 times in 613,775 calls under load and never on an idle machine. The scenario now starts the job again, which re-reads the clock away from the boundary, and throws if it still refuses, so a recurrence fails loudly instead of silently. Separately, each check-in is its own HTTP request, so the order they reach the mock server is not the order the SDK sent them in. Under load the `ok` check-in overtook `in_progress`. This is fallout from #20661, which moved these envelopes off stdout, where they were inherently ordered. The assertions are now `.unordered()`. Failed roughly 1 run in 30 under load before, 120/120 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
isaacs
force-pushed
the
isaacs/de-flake-slow-tests
branch
from
September 17, 2026 15:20
ae8b0f4 to
015bdd0
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 63f0621. Configure here.
s1gr1d
approved these changes
Sep 18, 2026
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.

Three unrelated flakes, all of which pass locally and fail only under CI load.
Tests that time out loading the SDK
Several unit tests time out in CI on the first test in a file. The first dynamic import of a large SDK graph pays for transforming it, and that cost is charged to whichever test triggers it. Locally it is a few hundred milliseconds. On a loaded runner it crosses the 5000ms test timeout.
Swept every unit test in the repo and fixed the files that show the pattern. Fixed with a bare side-effect
importat the top of the file. Collection is not bounded by the test timeout, so pulling the load into it takes the cost off the test without changing what the test exercises.Exception:
nuxt vite/orchestrionhas to register itsvi.doMockcalls before the module under test loads, and a static import is hoisted above them, so it warms the graph frombeforeAllinstead.Which file pays is not fixed: the two browser-utils files traded places between runs, because the transform lands on whichever test reaches the graph first.
Three tests in these packages still run over 400ms, and all three are slow for a reason:
appRoutertests busy-wait 1100ms on purpose.conflictingDebugOptionsre-evaluates three SDK entry points for eachdebug-buildmock, which no warmup can avoid.INP span never sent
hidePagewaits for onerequestIdleCallbackbefore hiding, assuming web-vitals has already queued its own idle callback and will therefore run first. But web-vitals only queues its callback once the click's Event Timing entry reaches its observer, which is afterhidePagequeues its own. SohidePagewins the race and hides the page too early.That loses the interaction. The forced report web-vitals does on
visibilitychangeruns while the metric is still -1, and the pending callback that sets the real value afterwards reports unforced. No INP is emitted at all and the test waits out its 30s timeout.Fixed by waiting for the entry and queuing the idle callback from a task after it, which puts web-vitals' callback first. Callers that never interacted skip the wait, so the 13 that only need the page hidden pay nothing.
The react-router-6 e2e suite now passes 10/10 and drops from 1.4m to ~53s.
Cron job that never runs
Two bugs in
suites/cron/cron, both reproducible under CPU load.cron'sstart()sizes the delay to the next tick from two clock reads, clamps a negative result to -1, and then treats that -1 as "stop". A pause between the two reads that straddles a second boundary is enough. The job is left scheduled for nothing, and the scenario exits on its own 15s timer having sent nothing.That is the reported failure, which reads as
0/5 expected envelope(s)on a clean exit 0 with no captured output. A probe measured that clamp firing 5 times in 613,775 calls under load and never on an idle machine. The scenario now starts the job again, which re-reads the clock away from the boundary, and throws if it still refuses, so a recurrence fails loudly instead of silently.Separately, each check-in is its own HTTP request, so the order they reach the mock server is not the order the SDK sent them in. Under load the
okcheck-in overtookin_progress. This is fallout from #20661, which moved these envelopes off stdout, where they were inherently ordered. The assertions are now.unordered().Failed roughly 1 run in 30 under load before, 120/120 after.