Repository navigation
test(interaction): frame evidence - #8045
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📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
5516f7e to
c5b69d0
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision. The BottomSheet detent spec asserts gesture and settle outcomes that the current BottomSheet contract does not state: a slow release settles on the nearest detent, a downward flick dismisses, and the settle is a composited transition with no Layout or Paint. An engineering owner needs to provide or identify the governing spec for those outcomes, for example in the BottomSheet contract or the motion design record, or the exemplar should stop pinning them; any engineering owner, the author included, records that decision by commenting /approve-spec <full head SHA> on this pull request. Whatever that decision, these changes are needed:
- Join
pr-interactionto the existingtestowner, astest-native-swiftdoes, or admit it through an AST-030 amendment, so a failing proof fails a required check. - Build the settle-curve oracle from fixed easing and duration values: the current check passes a 900 ms ease-in or 150 ms linear settle as readily as the real one.
- Make
busyWindowProoffail, and give the exemplar a frame floor, when the judged window is empty; with the block started 244 ms into the settle, all five assertions pass with zero frames. - Land #8043 and #8044 first, so this diff carries its own 1,443 authored lines instead of 4,561.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job routes itself with its own pr-scope-touches.sh pattern, runs only for pull_request, and no required join reads its result. spec:AST-030 FR11 rules that out for new coverage inside an existing surface ("MUST join those owners and MUST NOT create a separately routed lane or required context"), and FR7 requires that "Any owned lane failure MUST fail its join". test-native-swift shows the conforming shape: a fail-closed scope job, plus entries in test.needs and ci-test-join.mjs.
| ); | ||
| const from = samples[0].value; | ||
| const to = samples[samples.length - 1].value; | ||
| const ease = timingFunction(transition.easing); |
There was a problem hiding this comment.
The oracle takes its easing and duration from the animation under test, so this check cannot fail on the curve's shape or duration. On the Snap points story in Chromium it also passes with the settle forced to ease-in over 900 ms (0.094 px) or linear over 150 ms (0.30 px), and CI ran it on a 300 ms settle while the header says 410 ms. Pin the expected values, for example cubic-bezier(0.24, 1, 0.4, 1) and the story theme's 300 ms, and assert transition.easing and transition.durationMs against them (architecture:component-test-sufficiency INV6).
| end = Math.min(end, arrived.t); | ||
| } | ||
| } | ||
| const inner = {from: window.from + marginMs, to: end}; |
There was a problem hiding this comment.
With settledWithin, end can come before window.from + marginMs, which leaves inner empty or inverted. frames and medianFrameMs are then 0, expectedFrames is -1 and longestStallMs is negative, so every assertion in the BottomSheet exemplar passes with nothing judged. Starting the block 244 ms into the 300 ms settle on the Snap points story does exactly that. Throwing, or returning a failing proof, when the judged window is shorter than a few frames would close it.
# Conflicts: # internal/interaction-harness/src/curves.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
|
The scope rule caps a pull request at about 2,000 changed lines. Now that #8505 has landed, this branch's own commits change 3,070 against #8348's head, and astracat has asked in each of its last six reviews for instruments to move out. Moving only the five it names leaves about 2,300, so a split that clears the rule also takes out the frame-evidence core and leaves the busy-window proof here:
Each slice is a byte-identical part of this branch, stacked on #8348. This pull request merges F, so its head and tree stay as they are, every open pull request keeps importing from it, and each slice folds cleanly when main squashes it in. Should it split this way, or does your Oct 25 line-count waiver for new behavior modules (cited on #8055 and #8056) cover the harness? |
There was a problem hiding this comment.
Changes are needed for scope only: the frame-evidence instruments pass their positive and negative controls in Chromium and WebKit, and no defect was found in the code. The diff is 4,120 authored lines, and this pull request's own 3,105 stay above the 2,000-line scope threshold even after #8348 (the other 1,053) lands. Please land #8348, restack, and move the long-task reporting, the Storybook highlight default, castStoryboard, sampleRects and the container shader cache into their own pull requests. If the busy-main-thread proof alone still exceeds 2,000 lines, please explain why it cannot be split further under spec:AST-094 FR5.
[Automated review]
…r docker Docker ran the Playwright image as root over the bind-mounted checkout, so the test-results/ it wrote were root's and the next host run failed with EACCES clearing them. Under docker the container now runs with the host uid and gid (HOME=/tmp); rootless podman already maps the user. The README says so.
…-user Rootless Docker maps the container's root to the invoking user, and any other container uid to a subordinate host uid that cannot write the mounted checkout, so --user made the WebKit run fail with EACCES there. The script passes --user only to a rootful daemon (docker info reports no name=rootless); the README and header say so.
Scope: castStoryboard, sampleRects, openStory's Storybook highlight default and the WebKit container's shader cache move to a follow-up pull request, unchanged, with their self-tests and README text. The frame evidence keeps the screencast, marker tracking, frame stats and the busy-main-thread proof.
…f the frame evidence Scope: traceRendering, countBetween and the long-task log (with the harness-read marks the trace uses, their self-tests and README text) move to a follow-up pull request, unchanged. trace.ts keeps startCompositorTrace, which the busy-main-thread proof reads.
…e frame evidence Scope: startCompositorTrace (trace.ts), its Chromium self-test and the recorded skip fixture with its unit test move to the follow-up pull request with the rendering traces, unchanged. busyWindowProof keeps its presented option and its synthetic compositor-record tests; the README and JSDoc say where a record comes from without naming the moved helper.
There was a problem hiding this comment.
Changes are needed for sequencing more than for the code: the busy-main-thread instruments pass their positive and negative controls in Chromium and WebKit, and after the split this pull request's own commits change 1,872 lines, but the diff is still 2,918 because #8348 (1,080 lines) sits beneath it. Please land #8348 and merge main so this diff carries only its own commits; those need no line-count waiver.
README.mdline 36 promises busy-main-thread frames intest-results/interaction-evidence/, and lines 153-156 point to aDisplay::FrameDisplayedtrace that no instrument records at this head.- The description still lists
trace.ts,castStoryboard, the highlight default, the shader cache and their tests, which have left this pull request.
[Automated review]
| node internal/interaction-harness/scripts/webkit-container.mjs [playwright test args…] | ||
| ``` | ||
|
|
||
| Evidence (storyboards, curve traces, busy-main-thread frames) is written to |
There was a problem hiding this comment.
Nothing at this head writes busy-main-thread frames here: only storyboard.ts calls writeEvidence, and the busy proof returns a summary that the self-tests attach as an annotation (the busy-window filmstrip left with castStoryboard). Dropping "busy-main-thread frames" until that instrument lands keeps this line true.
|
|
||
| On a loaded machine Chromium's screencast can skip frames the compositor | ||
| still presents. Pass when the compositor presented each frame (epoch ms on the | ||
| page clock, from a trace of `Display::FrameDisplayed`) as `presented`. A gap between two moved |
There was a problem hiding this comment.
No harness instrument records Display::FrameDisplayed at this head (startCompositorTrace left with the split), so a proof author cannot follow this sentence. Saying that presented comes from the proof's own trace until the recorder lands would fix it.
|
A busy-main-thread proof on WebKit can fail for frames its screencast cannot resolve, and callers now work around it one spec at a time. Inside 2 px of rest, a spring's tail moves less than one of WebKit's 2 px cast steps a frame, so it reads 362, 362, 362 before a 366 rest. With
Harness unit tests: 54 passed. |
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/package.json # internal/interaction-harness/src/harness.interaction.browser.spec.ts # internal/interaction-harness/src/story.ts
There was a problem hiding this comment.
Changes are requested for two documentation fixes only: the busy-main-thread instruments discriminate as described in Chromium and WebKit (a compositor animation moves on every delivered frame through the block while the rAF control presents none), and with #8348 landed the diff is this pull request's own 1,872 lines.
internal/interaction-harness/README.mdline 36 says busy-main-thread frames are written totest-results/interaction-evidence/, but nothing in this change writes them; please drop that phrase until a writer lands.- The description's Change, Evidence and Testing sections still describe
trace.ts, the compositor recorder,castStoryboard, the highlight default and the shader cache, which have left this pull request; please rewrite them for what it now contains.
[Automated review]
…hat ships Nothing in the frame evidence writes busy-main-thread frames to the evidence directory, so the README no longer lists them there; and the busy recipe's block is spec:AST-094 FR5's 1 s rather than 500-1000 ms.
There was a problem hiding this comment.
Changes are requested for two fixes in the harness's own tests and docs; otherwise the busy-main-thread instruments discriminate as described, with a compositor animation moving through the block and a rAF one failing on its stall.
- In WebKit the rAF negative control fails at
harness.interaction.browser.spec.ts:331: a frame from before the block arrives about 45 ms into it and reads as "1/1 moved (100%)", because the moved criterion divides by the delivered frames (busy.ts:464-474), so judging a window that delivered fewer thanminJudgedFramesframes against its span (with abusy.test.tscase), or dropping that expectation, fixes it. README.md:11-14and the spec file's header (lines 6-7) say every positive check has a negative control, which theduring, curve-sampling and timer-gate tests do not have, so please scope that sentence to the busy-main-thread proof.- Non-blocking: the compositor positive control can fail on a 59-69 ms screencast gap even though every delivered frame moved, which the compositor record from #8548 absorbs once it feeds
presented.
[Automated review]
| const judgedFrames = | ||
| compositorFrameMs != null | ||
| ? changes.length > 0 | ||
| ? Math.round( | ||
| (changes[changes.length - 1] - (onScreenFrom ?? changes[0])) / | ||
| compositorFrameMs, | ||
| ) + (onScreenFrom == null ? 1 : 0) | ||
| : expectedFrames | ||
| : deliveredFrames === 0 | ||
| ? expectedFrames | ||
| : deliveredFrames; |
There was a problem hiding this comment.
Without a compositor record this divides by the frames delivered in the judged part, so a frozen animation with one late frame (a pre-block commit presented 43-45 ms into the block, as WebKit's slower cast delivers it) reads "1/1 moved (100%)" and only the stall check fails it, despite the comment above ("a frozen animation misses here too"). Judging a part that delivered fewer than minJudgedFrames frames against its span (expectedFrames) keeps this criterion independent; a busy.test.ts case with one stale frame at +45 ms would pin it.
| expect(raf.longestStallMs).toBeGreaterThan(600); | ||
| expect(raf.verdict.pass).toBe(false); | ||
| expect(raf.verdict.failures.join('\n')).toMatch(/longest stall/); | ||
| expect(raf.verdict.failures.join('\n')).toMatch(/expected frames moved/); |
There was a problem hiding this comment.
This expectation fails in WebKit when the cast is slow: the rAF control read "1 frames presented, 1/1 moved (100%) ... longest stall 635.4 ms (10 intervals) at +45 ms", so failures holds only the stall (cause at busy.ts:464-474).
| [`src/harness.interaction.browser.spec.ts`](src/harness.interaction.browser.spec.ts): | ||
| every positive check has a negative control beside it (a rAF animation that | ||
| freezes under a blocked main thread). |
There was a problem hiding this comment.
Only the busy-main-thread proof has a negative control; the two during tests, the curve-sampling tests and the timer-gate wait test assert only the positive outcome. Scoping this to "the busy-main-thread proof has a negative control beside it" keeps it true.
| * @output Proof that each harness instrument measures what it claims, with a | ||
| * negative control beside every positive one: scripted timestamps reach the |
There was a problem hiding this comment.
Same as the README: tests in this file such as lines 344-424, 427-449 and 1613-1638 have no negative control beside them.
| /** One display frame, ms: the least progress `blockMainThread` waits for. */ | ||
| const ONE_FRAME_MS = 16; |
There was a problem hiding this comment.
Non-blocking: with this constant between them, the JSDoc above attaches to ONE_FRAME_MS, so blockMainThread shows no documentation on hover; moving the constant above the JSDoc fixes it.
… against its span In WebKit the rAF negative control could read "1/1 moved": one frame from before the block arrived 45 ms into it, and the moved criterion divided by the delivered frames. A part that delivered fewer than minJudgedFrames frames is now judged against its span, as one that delivered none was (busy.test.ts case; it fails on the previous rule). The README and the self-test header now say only the busy-main-thread proof carries a negative control beside its positive check.
|
This branch counts frames in the cast's median, never in less than one 60 Hz frame:
|
The README's busy-main-thread paragraph and busyWindowProof's JSDoc said an undelivered frame is never a miss; when fewer than minJudgedFrames frames were delivered in the judged part, its span counts every frame. One clause in each says so.
There was a problem hiding this comment.
This is ready to approve: a frozen animation with one stale frame now fails the moved criterion as well as the stall (pinned by the new busy.test.ts case, which fails on the previous rule), the negative-control sentence is scoped to the busy-main-thread proof, and the frame-evidence self-tests pass in Chromium and WebKit. Nothing else is required; two optional follow-ups:
- On an alternating WebKit cast (about 11.3 and 17.1 ms) the stall limit follows the median's parity, so a 45.5 ms gap from two undelivered frames fails at an 11.3 ms median and passes at 17.1 ms; your display-cadence branch closes this.
- One frame in six of a WebKit cast slower than 27.5 ms leaves the self-test's slow case under the four-frame floor, which then reports the animation as not moving although every frame moved.
[Automated review]
| const cadence = | ||
| intervals.length >= 4 | ||
| ? intervals[Math.floor(intervals.length / 2)] | ||
| : stats.medianFrameMs || 1000 / 60; |
There was a problem hiding this comment.
Non-blocking: when WebKit's cast alternates short and long intervals (about 11.3 and 17.1 ms), this median lands on either value by the parity of the frame count, and the stall check divides by it (line 421). In a synthetic cast of that shape with two consecutive frames undelivered, the 45.5 ms gap reads 4 intervals and fails at an 11.3 ms median, and 3 intervals and passes at 17.1 ms, against the README's "two dropped frames" tolerance. Flooring the cadence at one 60 Hz frame, as your display-cadence branch does, takes the parity out of the verdict.
| const slow = await run('compositor', 6); | ||
| expect(slow.cadenceMs, slow.summary).toBeGreaterThan(60); | ||
| expect(slow.verdict.failures, slow.summary).toEqual([]); |
There was a problem hiding this comment.
Non-blocking: on a WebKit cast slower than 27.5 ms a frame, keeping one frame in six delivers a frame less often than every 165 ms, so expectedFrames falls under the four-frame floor and this case fails as "not moving during the block" although every frame moved (a synthetic 210 ms cast reads "660 ms judged = 3 frames < 4"). Choosing the stride from the measured cast interval would keep it near the ~100 ms the comment above describes.
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
Maintainer impact
"Runs on the compositor" is claimed in reviews but never shown: nothing records what the screen presents while the main thread is busy.
Intended invariant
No product change. A motion claim comes with frame evidence: per-frame positions from composited frames, judged through a busy main thread.
Change
frames.ts: timestamped composited frames (Chromium's; WebKit's, rebased from its monotonic clock), in presentation order, with per-frame marker boxes and frame stats.busy.ts: block the main thread mid-flight, from a task queued once the animation is on the compositor (not pending, a frame in), and judge the frames the screencast delivered: no stall over three intervals, ≥ 90% of the delivered frames moving (an undelivered frame is a gap for the stall check, not a miss), at least four judged intervals, a median limit only for display-rate claims. A just-started animation is judged from when it reaches the screen; given the compositor's own presentation record (presented), frames the screencast skipped while the compositor presented them are not stalls; the proof reports the cast's position step (WebKit's is 2 px).Split for scope: the rendering traces, the long-task log and the compositor presentation recorder are #8548;
castStoryboard,sampleRects,openStory's Storybook highlight default and the WebKit shader cache are #8549 (both stacked on this one). #8423 shows these instruments on BottomSheet's detents.Evidence
Scope
Testing
busy.test.ts29 passed (the verdict's limits, the on-screen start, the position step, the synthetic compositor-record cases, and a window that delivered under four frames judged against its span, which fails on the previous rule); harness typecheck and eslint clean.