Harden four CI-flaky tests without weakening their guards - #211
Merged
Conversation
Each fix separates the deterministic assertion (kept strict) from the noise-exposed part (bounded honestly), per the documented semantics. Sampled forced-blame (e2e): the sub-interval flushes in forces-layout.mjs lag a statement and recall low, so an exact-line gate is inherently noisy. Drive a new wide-forced-layout.mjs probe whose every read forces a flush wider than one sampler interval, which blame-semantics.md pins to the exact forcing line at ~100% recall (a slower runner only widens the flush, so recall does not degrade under load). Derive the expected read lines from the probe's `// R:` markers so the assertion never re-stales on a line shift. Measurement B-01: assert the profiler-start prefix's self-time stays far under the 9ms warmup floor (the windowing PROPERTY), not that `post` is not functions[0]. On a near-no-op every function holds ~one sample, so which sorts first is random; a stray in-window sample on post is not a windowing regression. INP entry-delivery race (e2e): a trusted 45ms click always emits an Event Timing entry, so the flake is delivery timing under load, not a dropped entry. Add WPD_INP_ENTRY_WAIT_MS to extend the bounded in-page drain (default 250ms unchanged) and set it generously for this test; the INP assertion stays strict. Settle re-attach (e2e): arm the reload on settle's FIRST animation frame instead of a fixed 10ms timer, so the commit lands mid-settle on the same frame clock settle uses (a slower host slows both together). A wall timer, or arming on the second frame, can drift past settle onto the end-mark/flush evaluates outside the re-attach loop and hard-fail. Confirmed the re-attach path still fires via instrumentation.
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.
Four tests were observed failing rarely on shared GitHub runners and passing on re-run. Each is hardened by separating the deterministic assertion (kept strict) from the noise-exposed part (bounded honestly per documented semantics), never by loosening the guard.
Local loop evidence was gathered on a quiet host (1-min load ~7) after killing an orphaned load generator; every number below is from the quiet machine.
1. Sampled forced-blame
±1line (test/cli.e2e.test.mjs)Diagnosed race:
forces-layout.mjsforces sub-interval flushes. Perblame-semantics.md, a flush narrower than one CPU-sampler interval (~132-150us) has recall~duration/intervaland its sampled read-site can lag a statement or land on the adjacent line. So the sampled lines spread across the read AND itsbump()write line, and recall is low. Reproduced: at 5 iterations,onKnownRead >= 3failed 4 of 12 runs. The hardcodedknownReadLineslist had also gone stale against the current fixture.Fix shape: drive a NEW
examples/probes/wide-forced-layout.mjswhere each read reflows a large subtree, forcing a flush wider than one interval, which the same doc pins to the EXACT forcing line at ~100% recall. Assert exact recovery of the read lines, derived from the probe's// R:markers so the list can never re-stale on a line shift.Why it does not weaken the guard: the assertion is now STRICTER (exact-line recovery of
>= 6/8markers, not a±1fuzz around>= 3). Crucially a slower runner makes each flush WIDER -> more samples -> higher recall, so CI load only helps. Loops: 15/15, then 12/12 after the fixture's final reformat.2. Measurement B-01 (
test/measurement/node-lane.test.mjs)Diagnosed race: the test asserted the
post (node:inspector)profiler-start prefix is notfunctions[0]. On a near-no-op every function holds ~one sample, so which sorts first is random. Reproduced:postranked #1 with a single 0.334ms stray sample in 1 of 8 quiet runs — not a windowing regression.Fix shape: assert the windowing PROPERTY directly —
post's self-time stays far under the 9ms warmup floor (< 5ms). If windowing regressed, the ~9-30ms prefix would land here and trip it; a stray sub-ms sample passes. This covers exactly the case the rank check flaked on (when post ranked #1 its selfMs was 0.334). Loop: 15/15 under the gate lock.3. INP entry-delivery race (
test/cli.e2e.test.mjs)Diagnosed race: a trusted 45ms click ALWAYS emits an Event Timing entry (Chrome emits for any interaction > 16ms), so this is delivery TIMING, not a dropped entry: under a loaded runner the entry's task slips past the default 250ms in-page drain.
Fix shape (product + test): add
WPD_INP_ENTRY_WAIT_MSto raise the bounded drain for a slow host; the default 250ms and all output are unchanged. The test sets it generously (3000ms) and keeps the INP/processing assertions strict. The wait sits after the step's end mark, so a longer ceiling never grows the measured window. Loop: 15/15.4. Settle re-attach mid-navigation (
test/cli.e2e.test.mjs)Diagnosed race: the fixture armed the reload on a fixed
setTimeout(reload, 10). Under load that wall timer can drift relative to settle and commit AFTER settle resolves — onto the end-mark/flushpage.evaluates, which are NOT under the settle re-attach loop, so the destroyed-context error propagates and hard-fails the record.Fix shape: arm the reload on settle's FIRST animation frame (its rAF is registered before settle's), so the commit lands mid-settle on the same frame clock settle uses — a slower host slows the reload frame and settle's frames together. Verified empirically that arming on the SECOND frame instead fails 15/15 (settle resolves near frame two, commit drifts onto the flush), which is exactly the drift being removed.
Why it does not weaken the guard: instrumented the settle re-attach catch and confirmed the destroyed-context re-attach path fires exactly once per run across 6/6 runs, with
navigation === "hard". Loop: 15/15 (also 15/15 under a residual load of ~14).Product change
Only
src/browser/driver.ts— theWPD_INP_ENTRY_WAIT_MSenv override on the INP drain ceiling (default behaviour and output unchanged). Patch changeset included. The other three fixes are test/fixture-only.Gates
lint, format:check, build, unit (738), full e2e (53), full measurement (6) all green locally.
npm run knipexits 1 in this worktree onpackage.jsondevDependency/binary resolution, but it does so identically on the clean stashed tree, so it is pre-existing and environmental, not from this diff.