Repository navigation
test(interaction-harness): a busy proof judges each predicted move - #10812
Conversation
A busy proof whose control moved on fewer than minJudgedFrames frames threw FrameStarvationError only when the cast also showed no frame for longer than FRAME_STARVATION_MS. On a loaded runner the gap can stay under that. The same shortfall then came back as verdict failures, and the spec failed as if the component had stalled. Under the floor, a run with no component failure on the frames shown is now too little to judge. busyWindowProof throws FrameStarvationError with the control's moves, the floor, the cast's frames and its longest gap, counting every frame the cast drew. A component that missed a frame the control moved on, or stalled longer than it, still fails the verdict. The cast's median interval judges no component, so it keeps no red there. pooledBusyProof does the same for too few judged runs or control frames. A pool left short of judged runs by a run that began too late keeps its failure. Every change turns a verdict failure into named starvation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
Changes requested: replaying the caught animation is a sound way to stop the zoom proof's false reds, but two harness defects need fixing first.
- Frames predicted to move under one cast step also count as component edges (
busy.ts:2758-2805), so a component frozen for 300 ms where the replay predicts 0.4 px a frame passes with 18 frames named while the cast-step rule fails it on the same frames; measure the predicted move from the last frame that showed a new position, so an accumulated full step is judged. predicts(busy.ts:1968-1971) ignoresanimation.unpredictable, so a window whose replay could not be read throwsBusyPredictionErrorin WebKit, without the ahead control and with no control at all, although the README says those windows fall back by name; countunpredictableinpredictsand cover the WebKit and no-ahead cases in the test for an unpredictable window.
Both cases reproduce with the synthetic curve and controls of busy.predicted.test.ts. Not blocking: the fade and pseudo-element (progress) path has no test or caller, and the README and the predictionOf JSDoc disagree with the code on when prediction needs the marker and when it throws.
[Automated review]
|
/approve-spec 0339061 |
0339061 to
bbc656c
Compare
|
/approve-spec bbc656c |
There was a problem hiding this comment.
Changes requested: both findings from the last review are fixed and now covered by tests, but the fix for the sub-step hold opens a similar gap.
- A hold that begins on a frame with no control reading is never judged (
busy.ts:2775-2785, then counted as a component edge atbusy.ts:2820): on the synthetic curve and controls ofbusy.predicted.test.ts, hiding both controls in the frame that first shows a held position and holding for 300 ms passes with 18 frames namedclock, while the cast-step rule fails the same frames, so decideundatedfrom frameiand the frame before it as before, measure the move from the run's first frame that has its own reading, and add that case to the tests.
Not blocking: the starvation for too many named frames counts judged frames as frames drawn and blames a loaded machine (busy.ts:2871-2875), and the README and the busyWindowProof JSDoc still make the harness marker a condition that the fade path does not check.
[Automated review]
A busy proof whose window caught an animation now predicts each frame's move, in Chromium, when the proof runs the control ahead. After the busy loop, the page replays the element's own runs on a hidden copy of its box. A frame the control moved on must show a new position when its predicted move is a full cast step. A frame predicted to move less is named as too little to judge. With a prediction, frames are dated on the controls' clock, from the starts the window now keeps. Any other window falls back by name to the cast-step rule, exactly as without a replay: no marker, no control ahead, WebKit, or a replay that disagrees with the cast. judgeEveryFrame keeps refusing an excused frame there. The zoom spec starts the control ahead.
bbc656c to
9956ef8
Compare
|
/approve-spec 9956ef8 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request is marked ready for review and a maintainer adds the ci:run label.
The hold that begins on a frame with no control reading and the starvation message from the last review are fixed, each with a test.
Not blocking:
- Miss: a hold whose judged frames each follow a frame with no control reading still passes (
busy.ts:2790-2791); decidingundatedfromseries[i]andseries[anchor]closes it and keeps every existing case and both recorded casts unchanged. - The README's fallback paragraph, the
controlPageTimesJSDoc and the zoom spec's verdict comment lag the code: a fade with no end anchor is judged on the controls' clock with no note, the median lag is undocumented, and named frames now leave the "every frame the control moves" count.
[Automated review]
| } | ||
| const move = Math.abs(predictedAt(t) - predictedAt(series[anchor].t)); | ||
| const undated = | ||
| unread.has(series[i].frame) || unread.has(series[i - 1].frame); |
There was a problem hiding this comment.
Not blocking. The move is now measured from anchor, but undated still looks at series[i - 1]. So when every other frame of a hold has no reading from either control, each judged frame is named clock and the hold is never judged: on the synthetic curve of busy.predicted.test.ts (3 px a frame), a 300 ms hold with both controls hidden on every other frame passes with 8 frames named, while the cast-step rule fails it ("moved on 33 of the 41 frames", "longest stall 300.0 ms"). unread.has(series[i].frame) || unread.has(series[anchor].frame) judges it ("moved on 33 of the 40 frames"), and the 27 cases and both recorded casts keep their results.
|
/approve-spec 99a1628 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request is marked ready for review and a maintainer adds the ci:run label.
The merge of main leaves this pull request's own changes as they were, and the earlier fixes stay covered by tests.
Not blocking:
- A hold whose judged frames each follow a frame with no control reading still passes (
busy.ts:2790-2791); decidingundatedfromseries[i]andseries[anchor]closes it and keeps all 27 cases passing. - The README's fallback paragraph, the
controlPageTimesJSDoc and the zoom spec's verdict comment lag the code: a fade with no end anchor is judged on the controls' clock with no note, the median lag is undocumented, and named frames now leave the "every frame the control moves" count. - Miss: the
readBusyWindowandBlockedWindow.controlStartsJSDoc (busy.ts:1941-1946,busy.ts:684) do not say that a predicting Chromium window is dated on the controls' clock, or that the window's starts are read only then.
[Automated review]
Maintainer impact
The busy double-tap zoom proof fails Chromium CI now and then. Under the CI profile, the current rule fails 10 of 240 recorded casts.
Both come from one cause. The rule guesses each frame's move from the frames around it. A cast that stamps frames late, or a spring's slow start, defeats that guess.
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 is named, and is never a red on the component or a pass.Change
unjudged.blockMainThreadnow returns the controls' starts for that.judgeEveryFramekeeps its meaning there. In WebKit the zoom spec still refuses an excused frame by name. With a prediction, no frame is excused, so nothing throws.Evidence
Scope
Testing
Merge conditions
Line numbers and results are at 99a1628.
busy.ts:1074,:1110,:1185). One channel alone misses: a single curve predicted 1.76 px where the edge moved about 0.85 px. A frame is named only when its composed move since the last new position is under one step (busy.ts:2768–2800,:2792). Every other frame the control moved on must show a new position.minJudgedFramesis unchanged and still required (busy.ts:2871,:2886). Too few judged frames throwFrameStarvationError(busy.ts:1925).LightboxZoom.interaction.browser.spec.ts:1264–1265). At 99a1628, both mutant tests passed 3 of 3 times in each engine, in the CI-profile container with one worker. So the proof failed every slowed zoom. WebKit falls back by rule (busy.ts:2004) and keeps thejudgeEveryFramethrow (LightboxZoom.interaction.browser.spec.ts:1153,:1156).The CI profile is the Playwright container with
--cpus=4,CI=1,LP_NUM_THREADS=4, one worker and eight busy loops. d5ac99dba2, 4bcd673, a670d34 and e4f6b93 are this change at earlier heads.busy.predicted.test.ts, 27 cases:judgeEveryFrame, the second cast and a synthetic slow start throwBusyFrameUnjudgedError; in Chromium they pass, the frames named;BusyPredictionError.busy.unjudged.test.ts(test(interaction-harness): a busy proof names a frame it cannot judge #10765's 5 cases) is unchanged and passes.tsc(harness and core): 0 errors. ESLint: 0. Prettier clean.pnpm review:pre --base bd57e0a91b --head 99a1628b5f: 1685 authored lines, merges cleanly, 0 findings.