Repository navigation
test(interaction-harness): a busy proof names a frame it cannot judge - #10765
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Lightbox · Storybook | 122.6 kB (0) · 10.6 kB CSS (0) | 3,377 (0) | ✓ · ✓ · ✓ |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 29 rows, core 36 rows, theme-butter 1 row, theme-y2k 1 row; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: one fix in the zoom proof's failure path.
- The zoom proof now throws
BusyFrameUnjudgedErroratLightboxZoom.interaction.browser.spec.ts:1087, beforewriteEvidence(line 1093) andcastStoryboard(line 1116), so a run that hits the sub-step frame uploads no frame JSON or storyboard (the evidencebusy.unjudged.fixture.tswas built from); write the evidence first, or catch the error, write it witherror.frames, and rethrow.
Two smaller points. pooledBusyProof accepts judgeEveryFrame through its options type but ignores it, so a pooled proof with the option still passes an excused frame; omit the option there or honour it. The description's "Intended invariant" credits spec:AST-094 FR5 with the named-failure rule, but FR5 only requires motion to keep advancing through a main-thread block; also, the replay's error reads 0.84 px, not 0.85 px.
[Automated review]
cixzhang
added
the
ci:run
24a5dbd to
32c29cc
Compare
|
/approve-spec 32c29cc |
32c29cc to
38e68a3
Compare
|
/approve-spec 38e68a3 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft, and it runs once the pull request is marked ready for review (the ci:run label is already on it).
Both points from the earlier review are fixed: the zoom proof keeps its frame JSON and storyboard when it throws BusyFrameUnjudgedError, and pooledBusyProof no longer accepts judgeEveryFrame. In a WebKit build that casts about every 22 ms, the zoom proof now fails by name on three or four sub-step frames near +1.2 to +1.4 s, where the base failed the same runs at belowStep === 0, so that flake remains for the planned control-clock follow-up.
- Non-blocking:
judgeEveryFrameonly acts when acontrolis passed; without one,busyWindowProofignores it and the recorded replay passes, so consider throwing when the option is set without a control.
[Automated review]
| * `BusyFrameUnjudgedError`. Such a frame is never a pass, and never a red | ||
| * on the component. | ||
| */ | ||
| judgeEveryFrame?: boolean; |
There was a problem hiding this comment.
Non-blocking: unjudged is only filled inside if (control != null) (line 1586), so judgeEveryFrame: true without a control is silently ignored: busyWindowProof(photo, block, {axis: 'y', settledWithin: 24, judgeEveryFrame: true}) on the recorded replay passes with no failures. The doc scopes the option to "With a control", and the zoom proof passes one, but a proof that forgets it gets a pass where it asked for every frame to be judged. Consider throwing when judgeEveryFrame is set without control.
A busy proof against a control counts a frame as moved when the cast's step explains a run of equal positions there (belowStep). Lightbox's double-tap zoom proof claims a new position on every frame the control moved, so it asserted belowStep 0. In CI the zoom's slow start moved the photo's top edge about 0.85 px a frame, under the cast's 1 px step. One frame read no change, and the assertion failed as an unnamed red. With the new judgeEveryFrame option, such a frame is too little to judge. busyWindowProof throws BusyFrameUnjudgedError with the frame, the component's move per frame there and the cast's step. A verdict that fails anyway comes back as it is, so a frozen or main-thread component stays a red on the component. Lightbox's zoom proof passes the option, and a run that throws still writes its frames and storyboard. No bound changes. busy.unjudged.test.ts replays the CI run's frames. Without the option the proof passes with belowStep 1. With it, the error names the frame at +38.9 ms.
38e68a3 to
36af927
Compare
|
/approve-spec 36af927 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is a draft, and it runs once the pull request is marked ready for review (the ci:run label is already on it).
In Chromium the zoom proof passes with judgeEveryFrame, its main-thread and frozen mutants still fail, and the replay shows the sub-step frame passing on main and failing by name here. Two non-blocking points:
- Miss:
BusyFrameUnjudgedError's message says the component moved "under two of the cast's steps" even whenmovePxis 2 or more (a replay with a 4 px catch-up after a 1.7 px/frame pace reports "about 2.07 px a frame, under two of the cast's 1 px steps"), so report the pace the rule used or drop that clause. judgeEveryFramewithout acontrolis still ignored silently; consider throwing when the option is set without one.
[Automated review]
| `busyWindowProof: too little to judge: ${frames | ||
| .map( | ||
| frame => | ||
| `at +${frame.atMs.toFixed(1)} ms the component moved about ${frame.movePx.toFixed(2)} px a frame, under two of the cast's ${frame.stepPx} px steps, and the frame shows no step while the control moved`, |
There was a problem hiding this comment.
Non-blocking (Miss): movePx is the run's catch-up over its frames (lines 1742-1744), but underStep can excuse the frame on the pace before the run (line 1678), which allows a catch-up of two frames of that pace plus a step. Replaying the recorded cast with the top edge held at +1106.8 ms and caught up 4 px on the next frame throws "at +1106.8 ms the component moved about 2.07 px a frame, under two of the cast's 1 px steps". Report the pace the rule used here, or drop the "under two" clause.
…ols (#10772) * fix(interaction-harness): a WebKit busy proof dates frames on controls WebKit stamps the frame after a delivery gap about 60 ms late, carrying one frame of motion. A busy proof on the stamps then reads a stall the control does not show. A WebKit busy proof with a control now dates each frame from the controls' positions. A frame with no reading keeps its stamp's distance from the nearest frame whose stamp agrees with its reading. The dated frames keep the cast's order, each at its own time, and the proof reports its judged part on the cast's stamps. A proof that judges every frame near the control's turns opts in to the control ahead. Chromium casts and casts with no control keep their stamps. Each cast frame records its engine for this. * fix(interaction-harness): WebKit busy proofs keep one frame per interval WebKit's compositor can draw two frames 6 to 9 ms apart. A curve then shows no new step between them, and a busy proof read a held frame. A WebKit busy proof now keeps one dated frame per display interval, the earlier one, read to one px of the controls' travel. A frame dated from a neighbour always stays, so no held frame is left out. A WebKit cast rarely shows two frames one interval apart, so the proof judges no 30 Hz cadence there. Its summary says so in a note, with the count of such pairs. * Merge main into harness/webkit-busy-dating Resolves the one conflict with #10765 in pooledBusyProof's options: the Omit list keeps both additions. * test(core): the Switch colour proof dates frames through the harness The Switch track-colour busy proof dated WebKit frames itself, with its own reading, display-rate and cadence helpers. With the harness now dating every WebKit busy proof with a control, two rules ran on it. It now passes both controls and their starts to pooledBusyProof, which dates and keeps its frames. The harness exports pageTimesOnControls, the readings its dating starts from, for checks that need a frame's own time: the colour's landing and the thumb's curve. The replay of two late-stamped WebKit casts tags its frames as WebKit's: dated by the harness it passes, while their stamps and their readings alone fail. * test(core): the animator proofs read frame times from the harness The animator's spring and retarget proofs read each frame's page time from the two controls inline: the reading pageTimesOnControls returns. They now call it, so a fix to the harness's reading reaches them too. A unit case pins each of the order rule's two passes, not their removal together. In a late burst, a held frame with no reading moves to just before the next frame with one, and still reads as a stall.
Maintainer impact
LightboxZoom.interaction.browser.spec.ts:1039, the busy double-tap zoom proof, failed in Chromium CI atexpect(proof.control?.belowStep).toBe(0). It received 1.The control moved its full 10 px on that frame. The photo's top edge moved about 0.84 px, the zoom's slow start at tempo 5, under the cast's 1 px step. So the frame read no change. The proof's own rule counted it as moved (
belowStep), and the spec's stricter claim failed as an unnamed red.Intended invariant
spec:AST-094FR5: motion keeps advancing at display rate through a main-thread block. The busy proof judges that claim frame by frame. A frame the cast cannot judge fails by name: it is never a red on the component, and never a pass.Change
busyWindowProofgainsjudgeEveryFrame(default off). With it, a frame the control moved on where the cast could not show the component's step (onebelowStepcounts) throwsBusyFrameUnjudgedError. The error carries each such frame, the component's move per frame there and the cast's step.belowStep === 0included. A run that throws still writes its frames and storyboard.busy.unjudged.test.tsreplays the failing CI run's 175 frames frombusy.unjudged.fixture.ts.Evidence
belowStep1, which the spec rejects without a name.judgeEveryFrame, the replay throwsBusyFrameUnjudgedErrornaming +38.9 ms, 0.84 px a frame under the 1 px step.Scope
Testing
The CI profile is the Playwright container with
--cpus=4,CI=1,LP_NUM_THREADS=4, one worker and eight busy loops. It was measured at 53f7843. Since then the head added a sharedrunEndhelper and comments, with no behaviour change. The zoom spec now writes its evidence on the throw path too.busy.unjudged.test.ts, 5 cases:belowStep1;FrameStarvationError, with the control moving on 2 or 3 frames in the 2.5 s block. That is this container's load on WebKit, unchanged by this PR. CI's WebKit leg judged the same proof on 6 frames.judgeEveryFrame. Chromium 6 of 6 and WebKit 6 of 6 still fail as reds on the photo.BusyFrameUnjudgedError. Each still attaches the frame JSON (withunjudged) and the storyboard.tsc(harness and core): 0 errors. ESLint: 0. Prettier clean.pnpm review:pre --base 3e27971f3a --head 36af927ac0: 527 authored lines, merges cleanly, 0 findings.