Repository navigation
test(interaction-harness): a busy-window control clock and a marker search in a region - #9872
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the new control clock still depends on the screencast's stamps, through the order of the frames it receives.
Blocking:
controlClockcounts a turn at every direction flip in input order, butcast.stop()returns frames sorted by stamp, so frames stamped more than one frame interval apart become false turns that shift every later reading by 800 ms without a throw (busy.ts:232); reject a reading that moves back or jumps a leg or more (or accept a flip only near an end) and add a test that sorts late-stamped frames withinPresentationOrder.- The description says frames "stamped late by up to a leg" read their animation time "whatever the screencast stamps say", but
busy.clock.test.tsstamps frames at most 12 ms late and keeps them in true order, so either fix the clock as above or narrow the claim to frames stamped less than one frame interval apart.
Also worth a look: the busy.ts header and the README's busy-main-thread section do not mention controlClock or trackControl yet. CI has not run on this head because the pull request is a draft.
[Automated review]
There was a problem hiding this comment.
Changes requested: the control clock now reads late, stamp-sorted frames at their own time, but it lands as a second copy of a clock core already has for the same control.
Blocking:
controlClockininternal/interaction-harness/src/busy.tscopies the origin-vote clock inpackages/core/src/motion/__tests__/controlClock.ts, whichanimator.interaction.browser.spec.tskeeps calling, and the two already disagree (a 24 px versus 36 px edge band, and repeated frames), so move the animator spec onto the harness clock and delete core's copy and its test (moving its two edge cases intobusy.clock.test.ts), or say in the description why the harness clock cannot serve it.
Also worth a look: the README now points to trackControl right after describing a control mounted within a dialog that moves with the sheet, but trackControl searches only the top 256 × 16 px, so that case should keep trackMarker(page, frames, {rgb: CONTROL_RGB}). The earlier points on stamp-order turns and the lateness claim are resolved. CI has not run on this head because the pull request is a draft.
[Automated review]
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 from a fork; once it is marked ready, a maintainer adds the ci:run label so CI runs.
The earlier point is resolved: the animator spec now reads its control with the harness's controlClock, core's copy and its test are gone, and the README sends a control mounted within a dialog to trackMarker. The animator's busy-window proof passes in Chromium on this head.
- Worth a look, not blocking: the 36 px edge band reads 43 frames per block where core's 24 px band read 47, so the proof's
toBeGreaterThan(20)floor has less margin on runs that drop many frames; if thin runs show up in CI, scale that floor with the band.
[Automated review]
…earch in a region A busy-main-thread proof reads the screencast. On a loaded machine the compositor presents fewer frames and the screencast stamps some late. So a proof that times its frames by their stamps fails with nothing wrong in the component. The control animation's own position now gives the time instead: `controlClock` reads it across the control's turns, and `trackControl` finds the control in its own strip. `trackMarker` takes an optional region, so a proof decodes only the strip it reads.
…ames at their own time `controlClock` counted a turn at each direction flip in stamp order. A frame stamped more than a frame late lands after frames that it came before, and the flip that it fakes shifted every later reading by a leg, with no error. The clock now takes the cycle origin that most frames agree with, each within 20 ms of one of the two times that its position gives. Each frame's stamp, measured from that origin, then picks its own time. A late or reordered frame moves only its own reading, and a gap moves none, so the whole-leg throw goes. A frame within 36 px of a turn, or one that repeats the position before it, gets no reading. Core's animator spec reads its control the same way (motion/__tests__/controlClock.ts). The unit test adds frames stamped 20 to 50 ms late and put in presentation order, a gap of more than a leg, and a repeated position. The file header and the README name trackControl and controlClock.
… harness clock, and core's copy goes The harness controlClock duplicated core's motion/__tests__/controlClock.ts, and the two already disagreed: core's left out only 24 px at a turn, and it read a repeated frame at its stale position. The animator's busy-window proof now reads `controlClock(control)` from the harness, over the whole screencast, and core's copy and its test go. Their two edge cases move into busy.clock.test.ts: a turn between the first two usable frames, and a first frame stamped 50 ms late. The README now says that a control mounted `within` a dialog moves with it, so trackMarker tracks it; trackControl searches only the top strip.
453dcde to
7877214
Compare
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Approved pending CI: docsite-test is still running on this head; test, build, lint, typecheck and the Chromium and WebKit interaction suites pass, the animator's busy-window proof among them.
The control clock reads late, reordered frames at their own time, Core's animator spec now reads its control with it, and Core's copy and its test are gone. No blocking findings.
[Automated review]
Maintainer impact
A busy-main-thread proof blocks the page's main thread and reads the screencast for the motion that must keep going. On a loaded machine the compositor presents fewer frames, and the screencast stamps some of them late. A proof that times its frames by their stamps then fails with nothing wrong in the component.
Intended invariant
A busy-main-thread proof judges the component against the harness's compositor control, recorded in the same screencast (
startBusyControl,busyWindowProof). A starved machine fails the control, not the component.Change
controlClockreads the control animation's own position as its clock. The cycle's origin is the one most frames agree with, and each frame's stamp then picks its own out or back time. So a late or reordered frame moves only its own reading, and a gap moves none. A frame near a turn gets no reading.busy.clock.test.ts.trackControlfinds the control in its own strip at the page's top, andCONTROL_SIDEandCONTROL_LEG_MSname that strip and its pace.trackMarkertakes an optional region, so a proof decodes only the strip it reads, which keeps a loaded machine inside the test budget.The harness, plus Core's animator spec, which switches to it. Its first new user is the Lab RefreshControl's interaction spec (#9865).
Evidence
busy.clock.test.tsfails against main'sbusy.ts(controlClockis not a function). Its five cases also fail 5 of 5 against the first version of this clock, which counted turns in stamp order.busy.clock.test.tspasses, including frames stamped up to 50 ms late and sorted by stamp, as the screencast hands them over. A gap longer than a leg reads right too. The harness's node tests pass.trackMarker's region is optional).Scope
Testing
Local gates at 7877214, on main: the harness's node tests (7 files / 132) and its type check. Strict ESLint reports 0 errors with the browser-spec import rule; Prettier and
review:prepass.