[test] Stabilize flaky browser tests - #5543
Merged
michaldudak merged 9 commits intoAug 21, 2026
Merged
Conversation
Fixes the two structural causes behind the browser-only flakes reported by
CircleCI Insights. Both are invisible in jsdom, and CI's `retry: 1` hides them
from the check status.
`timeStamp` is read-only and not a valid `EventInit` member, so passing it to
`fireEvent` never set it. jsdom stamps events off the (faked) clock, but real
browsers stamp them off the real monotonic clock, so the swipe velocity math in
`useSwipeDismiss` was reading whatever wall-clock gap the runner happened to
leave between two calls rather than the timeline the test describes. Requesting
`[1050, 1100]` yielded `[1000, 1100]` in jsdom but `[711.9, 712]` in chromium.
With `MAX_RELEASE_VELOCITY_AGE_MS` at 80, an 80ms scheduling hiccup flips a
dismiss decision. Adds `firePointer`, which applies the timestamp explicitly,
generalizing the approach `DrawerRoot.test.tsx` already took by hand.
The PreviewCard hover tests ran three sequential real 300ms close delays, each
consuming ~315ms of testing-library's 1000ms `waitFor` budget; their p95 on CI
sat at 1.00s and 1.02s. They assert which trigger opens the card, not close
timing, so `closeDelay={0}` removes the timing race.
Adds the `resetBrowserPointer` guard that sibling popup test files already have
to two tooltip files that were missing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
commit: |
Bundle size
PerformanceTotal duration: 1,092.00 ms +26.67 ms(+2.5%) | Renders: 76 (▼-16) | Paint: 1,762.58 ms +6.09 ms(+0.3%)
13 tests within noise — details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
`Benchmark tests` fails intermittently on unrelated PRs with
Tooltip mount (300 contained roots)
AssertionError: Iteration N render events differ from iteration 0:
expected [ 'bench:mount' ] to deeply equal [ 'bench:mount', 'bench:update', ...(8) ]
seen at iterations 10, 11 and 16 on three different branches, and once in the
baseline step that runs merge-base code.
The benchmark rendered 300 hoverable `delay={0}` triggers. The real Playwright
cursor persists across a run, and when it comes to rest over one of them the
tooltip opens by itself, adding ~9 render passes for the open cascade. The
harness closes its recording window on the first paint entry, so those passes
land inside the window only some of the time and iterations stop matching.
Measured locally: cursor away 1 render, cursor parked over the trigger area 10
renders — the same count CI reports — and 1 render again with the triggers made
inert. `delay={0}` is dropped along with it; a mount benchmark never hovers.
Render counts are unchanged on an uncontaminated run (mixed stays at 5), so this
removes the interference without altering what is measured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
michaldudak
marked this pull request as ready for review
August 20, 2026 07:50
michaldudak
requested review from
atomiks,
colmtuite,
flaviendelangle and
jjenzz
as code owners
August 20, 2026 07:50
The first pass converted only call sites matching `timeStamp:`, which missed
`swipe()` in DrawerSwipeArea.test.tsx — it passes the stamp as shorthand
(`...(useTimeStamp ? { timeStamp } : null)`), so both the rewrite and the
leftover check skipped it. That left `allows a later click-only activation to
dismiss after a swipe`, cited as the top CI flake, still deriving release
velocity from real wall-clock gaps. `swipe()` now routes through `firePointer`,
and the timeline is no longer opt-in: pointer swipes default to a 1ms start with
16ms steps, so every `swipeUp`/`swipeLeft` caller gets a fixed timeline instead
of only the two that passed `timeStepMs`.
`uses a size-based swipe threshold by default` aged its drag sample past
`MAX_RELEASE_VELOCITY_AGE_MS` with a real `wait(81)` — an 81ms sleep clearing an
80ms threshold by one millisecond. It now steps the gesture at 100ms on its own
timeline.
React's synthetic event reads `event.timeStamp || Date.now()`, and
`useSwipeDismiss` reads the synthetic event, so a zero stamp reached handlers as
wall-clock time; `getValidTimeStamp` rejects `<= 0` anyway. `firePointer` now
throws on non-positive stamps, and the three `timeStamp: 0` sites start at 1.
Also folds `simulateTimestampedPointerSwipe` and `simulateTimedSwipe` into one
`firePointer`-based helper, dropping the browser branch's real sleeps, and adds
the pointer guard to TooltipTrigger.test.tsx.
The benchmark fix moves `pointer-events: none` off the 300/200 individual
triggers and onto the containers, keeping per-row style objects out of the mount
path being timed. Verified still 1 render with the cursor parked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
simulateTimedRightSwipe and simulateTimedDownSwipe were missed in the previous pass. Their jsdom branches were unreachable — every caller is skipIf(isJSDOM) with useFakeTimers = isJSDOM — so the nine browser-only tests using them ran the untimed branch, sleeping on the wall clock while their startTime/moveTime/endTime arguments went unread. Both now build a step list and delegate to simulateTimedSwipe, which drops the last real sleeps from the file: DrawerRoot.test.tsx goes from ~2017ms to 422ms in chromium. A canary throw confirmed simulateTimedSwipe's `vi.isFakeTimers()` branch is dead in both environments, so it is removed rather than left as decoration. Nothing pinned the helper's behaviour, so a future @testing-library or React change — or a well-meaning simplification back to `fireEvent[type](element, init)` — would silently return all 53 call sites to real-clock timing with every test still green. test/pointer.test.tsx asserts a React handler observes the requested stamps, and that a non-positive stamp throws. Verified it fails on that exact regression: jsdom reports epoch values and chromium reports [297.5, 298, 298] against the expected [1000, 1050, 1100]. `timeStamp` is now required rather than optional, so omitting it is a compile error instead of a silent return to the runner's clock. Typecheck is unchanged (77 pre-existing errors before and after), confirming all call sites pass one. `useTimeStamp` in swipe() was constant once the touch branch returns early, so the dead spreads and guards are inlined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes the 30 per-event `vi.setSystemTime` calls interleaved with `firePointer` in DrawerViewport.test.tsx and useSwipeDismiss.test.tsx. They were the mechanism that made these gestures work under jsdom while `timeStamp` was being dropped; now that the stamp is applied explicitly, they state the timeline a second time without feeding anything. The pre-render base in each test is kept. Confirmed inert rather than assumed: collapsing every `setSystemTime` in both files to a single constant, leaving the `timeStamp` values untouched, kept all 117 tests green in both environments. Had the gesture logic read the wall clock, that collapsed timeline would have changed the outcomes. This also settles a disagreement from the previous round in the reviewer's favour. The objection raised there — that `useDismiss.ts` and `safePolygon.ts` read the clock — does not reach these files: the drawer never uses `safePolygon`, and the `Date.now()` in `useDismiss` sits behind `event.touches[0]`, which pointer gestures do not take. It also removes an inconsistency this PR introduced, having stripped the same pattern from DrawerRoot.test.tsx while leaving it in two sibling files. The benchmark comments claimed per-trigger styling would allocate 300 style objects. `INERT_TO_POINTER` is module-level, so it would pass one shared object 300 times; the real cost is React writing an inline style onto 300 nodes inside the timed path. Reworded to say that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replacing simulateTimestampedPointerSwipe with firePointer regressed twelve drawer tests. The old helper set every field via Object.defineProperties, which bypassed the constructor; firePointer routes them through createEvent, and an unconditional `beforeAll` shim replaced window.PointerEvent with MouseEvent — so `pointerId` and `pointerType` were dropped. Probed through a React handler: with the shim both read `undefined` in jsdom *and* chromium, without it both deliver `pointerId: 1, pointerType: 'mouse'`. jsdom implements PointerEvent now, so the jsdom#2527 workaround is stale and was degrading the browser run too. It is removed from all four files that carried it. Nothing failed today because every consumer keys on 'touch' or 'pen' and `undefined` falls the same way as 'mouse', but a positive `=== 'mouse'` branch would have been silently untested. `beforeRelease` had no way to advance the gesture timeline, so the moves it fires were stamped off the real clock — landing out of order against the synthetic timestamps and skipping the release-velocity refinement at useSwipeDismiss.ts:786. It now receives `nextTimeStamp()`. Measured end to end, the gesture is [1, 17, 33, 49, 65, 81] rather than [1, 17, 397.5, 49]. A no-restricted-syntax rule now rejects `timeStamp` inside a `fireEvent.*` init and points at firePointer, so the original footgun is unavailable rather than merely fixed once. Also drops the five `useFakeTimers = isJSDOM` blocks, unreachable inside it.skipIf(isJSDOM) bodies, and the beforeAll imports left unused by the shim removal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `no-restricted-syntax` entry added for `fireEvent`/`timeStamp` replaced the
base config's list rather than extending it — flat config overwrites rule
options — so every `**/*.test.*` file silently lost 11 shared restrictions,
including the React namespace-import rule and the `window.setTimeout` family
that AGENTS.md mandates. Verified with `eslint --print-config`: 11 entries
before, 1 after, 12 now. The base entries are re-included, and extracting zero
of them throws rather than quietly dropping the list again.
Three release-velocity tests were jsdom-only because they needed
`vi.setSystemTime` to control event timing. `firePointer` removed that
dependency, so they now run in both environments — the velocity contract this
work exists to stabilise was still only asserted in jsdom.
pointer.test.tsx asserted `timeStamp` but not the fields that actually
regressed. It now pins the whole init; confirmed it fails on the real
regression by re-adding the PointerEvent shim, which reports `pointerId` and
`pointerType` as undefined in both environments.
The three PreviewCard hover assertions are synchronous again. With `delay={0}`,
useHover.ts calls `setOpen` inline rather than through a timeout, so the bare
assertion is a real guarantee about immediate opening that `waitFor` cannot
distinguish from opening a tick later. The flake was the close path, which
`closeDelay={0}` already fixed.
Also: drops the eight remaining pre-render `vi.setSystemTime` calls; makes
`nextTimeStamp` throw on the touch path, where no event carries it, rather than
handing out a timeline that silently does nothing; rewords the lint message,
which fires on touch events too, to name the constraint instead of a
pointer-only helper; documents `firePointer` in AGENTS.md.
The PointerEvent shim is gone from DrawerVirtualKeyboardProvider too. It stays
in SliderRoot, where it turns out to be load-bearing for a reason its comment
did not state: jsdom implements PointerEvent but not the pointer capture
methods. Comment corrected to say so.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes the structural causes behind the browser-only test flakes. All of them are invisible in jsdom, which is why they survive local runs — and CI's
retry: 1retries them into a green check, so they never show up in the check status either. The failure data came from CircleCI Insights (/insights/gh/mui/base-ui/flaky-testsand.../workflows/<wf>/test-metrics, both public).fireEventsilently dropstimeStamptimeStampis read-only and not a validEventInitmember, sofireEvent.pointerMove(el, { timeStamp: 1050 })never sets it. jsdom stamps events off the (faked) clock, but real browsers stamp them off the real monotonic clock. Probing both environments with identical input:event.timeStamp[1050, 1100][1000, 1100][1050, 1100][711.899…, 712]So in the browser these tests weren't simulating the gesture they describe — the velocity math in
useSwipeDismisswas reading whatever wall-clock gap the runner happened to leave between two calls. WithMAX_RELEASE_VELOCITY_AGE_MSat 80, an 80ms scheduling hiccup silently flips a dismiss decision.This matches the reported failures:
DrawerSwipeArea.test.tsx > allows a later click-only activation to dismiss after a swipeis the top failing test across all three browser jobs (2/82 each), andDrawerViewport.test.tsx > does not dismiss from a fast swipe that was never attributed to the snap point axisreproduced locally on a full run, settling on'200px'instead of'100px'.firePointerAdds
firePointertopackages/react/test/pointer.ts, exported from#test-utils. It builds the event withcreateEvent, applies the timestamp with an explicitObject.defineProperty, then dispatches — generalizing whatDrawerRoot.test.tsxpreviously did by hand. AtimeStampis required, and one that isn't> 0throws rather than falling back: React's synthetic event readsevent.timeStamp || Date.now(), andgetValidTimeStampinuseSwipeDismissrejects anything<= 0, so a zero stamp can only reintroduce the real-clock dependency.packages/react/test/pointer.test.tsxcovers all three properties.Two guards keep this from regressing:
no-restricted-syntaxrule ineslint.config.mjsrejectstimeStampinside anyfireEvent.*init in a test file. Flat config replaces rule options rather than merging them, so the block re-includes the base config's ownno-restricted-syntaxentries (React namespace imports,throw Error(), thewindow.setTimeoutfamily) and throws at config load if the extraction ever comes back empty.AGENTS.md.swipe()now runs on a fixed timeline by defaultDrawerSwipeArea.test.tsx'sswipe()helper previously applied timestamps only when a caller opted in withtimeStepMs; every other pointer swipe inherited the runner's clock. It now defaults tostartTimeMs: 1, timeStepMs: 16, so every pointer gesture is timed.This is the change with the widest blast radius in the PR — roughly twenty tests are re-timed, and in jsdom several move from recording no drag samples at all (identical frozen timestamps meant
recordDragSampleskipped every one) to recording real velocity. Both environments now exercise the same branch.beforeReleasereceives the gesture's timeline so anything it injects stays on the same clock, and the one caller that needed to age a sample past the flick window (slowSwipe) expresses that astimeStepMs: 100instead of a realawait wait(81).window.PointerEvent = window.MouseEventremovedjsdom 27 implements
PointerEvent, so thebeforeAllshim from jsdom#2527 is obsolete — and actively harmful, because it also ran under browser mode, where it costpointerIdandpointerTypeon every event in the file. Removed fromDrawerRoot,DrawerSwipeArea,DrawerViewport,DrawerVirtualKeyboardProvideranduseSwipeDismisstests. ThreeuseSwipeDismissrelease-velocity tests that wereskipIf(!isJSDOM)now run in the browser too.SliderRoot.test.tsxkeeps the shim, because jsdom still doesn't implement the pointer-capture methods onElement. Its comment now records what the shim actually costs and how to drop it.waitForbudget consumed by real delaysThe two flakiest tests by CircleCI's count are the PreviewCard "any trigger on hover" tests, with p95 durations of 1.00s and 1.02s against testing-library's 1000ms
waitForbudget. Each runs three sequential real 300msCLOSE_DELAYwaits, every one consuming ~315ms of a 1000ms budget.They assert which trigger opens the card, not close timing, so
closeDelay={0}removes the timing race entirely.PreviewCardRoot.detached-triggers.test.tsxgoes 7833ms → 5881ms.Missing pointer guard
TooltipRoot.detached-triggers.test.tsx,TooltipProvider.test.tsxandTooltipTrigger.test.tsxlacked thebeforeEach(resetBrowserPointer)that every sibling popup test file has. All appear in the CI flaky data, and TooltipProvider failed once locally — a fake-timer test failing only in the browser, which points at the real pointer, the one input fake timers can't control.Benchmark determinism
The real cursor persists across a benchmark run and can come to rest over one of the 300 tooltip triggers in
tooltip.bench.tsx, opening a tooltip on its own. That adds ~9 render passes which land inside the measurement window only some of the time, so iterations stop matching and the harness's equality check fails. Mount is what these benchmarks measure, so the triggers never need to be hoverable:pointerEvents: 'none'is set on the container (not per trigger, to keep React from writing an inline style onto all 300 nodes inside the timed path) and the unuseddelay={0}is dropped.mixed.bench.tsxgets the same treatment;MountListinshared.tsxgrows an optionalstyleprop to allow it.Verification
pnpm typescript,pnpm eslint,pnpm prettiercleanDrawerSwipeArea.test.tsx443ms → 248ms in chromium, from no longer waiting on real wall-clock gaps.DrawerViewport.test.tsxis unchanged within noise — its gestures were already driven byvi.setSystemTime, so there was no real sleep to remove, only bookkeeping.Follow-up, not in this PR
Velocity-sensitive gestures that never specified a timestamp keep the same exposure, since there was no written-down timeline to honor — fixing them means choosing one per test, which is a semantic change better made deliberately, probably alongside a shared swipe-gesture helper. Two groups remain:
DrawerSwipeArea.test.tsxandDrawerViewport.test.tsx.firePointerhas no touch equivalent yet. The touch flake reproduced 0/25 in isolation and only surfaced once under full-suite contention.fireEvent.pointer*— 9 inDrawerViewport.test.tsx, 6 inuseSwipeDismiss.test.tsx, 3 inDrawerSwipeArea.test.tsx.DrawerViewport.test.tsx > navigates snap points from a drag that was never attributed to the swipe axisis the clearest case: it is the direct sibling of a test fixed here, asserts a snap-point outcome that depends on release velocity, and fires five untimed events. Note the new lint rule cannot see these — it only fires when atimeStampis present — so they need finding by hand or a broader rule.🤖 Generated with Claude Code