Repository navigation
test(interaction-harness): name a touch play the runner starved - #10551
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 ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/NavigationScreen | 82.6 kB (0) · 1.5 kB CSS (0) | — | — · ✓ · ✗ |
| lab/NavigationStack | 82.6 kB (0) · 1.5 kB CSS (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.
Changes requested: one fix in the new failure naming.
internal/interaction-harness/src/harness.interaction.browser.spec.ts:346starts the runner-starvation span at the move's scripted time instead of the due time (in practice the send before the move) that the comment above it describes, so under intermittent runner stalls a pacer that sends each step late is reported as "the runner was starved"; start the span atsentAt[i - 1], or say in the comment why a stall before the previous send can make this move late.
The rest looks right: dueAt matches the harness's keep-the-gap pacing, and the one-take check still fails a slow or bursting pacer. Optional: the >= at line 351 blames the runner at exactly half the off-pace moves, while the comment says "most".
[Automated review]
c27fcdd to
c8d4b5e
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, so once this draft is marked ready for review, a maintainer adds the ci:run label so CI runs.
Both points from the last review are fixed: a slow pacer under intermittent runner stalls is now named as the pacer, and the runner is named only for a strict majority of the off-pace moves.
- Miss: (optional, non-blocking)
internal/interaction-harness/src/touch.ts:29-30still says a watched'schedule'play does not keep gaps, and its SYNC note names this test, so dropping that clause keeps the two in step.
[Automated review]
vjeux
changed the title
c8d4b5e to
9b36c38
Compare
|
/approve-spec 9b36c38 |
There was a problem hiding this comment.
Changes requested: one blocking fix in the WebKit touch pacing self-test.
- The self-test credits a late move to the runner whenever a control timer in the same test process woke about as late (
harness.interaction.browser.spec.ts:414-415), but harness code that holds that process delays the timer too, so a pacer whose wait spins 20 ms past every deadline, or a send that holds the process 20 ms, now passes 3 of 3 wheremainfails both; please credit a wake to the runner only when the process used little CPU across it (for example withprocess.cpuUsage()), and model the runner-stall control as a stop from outside the process.
Non-blocking:
- The off-pace count can list one move twice ("left late" and "followed a slow send"), so
:429-432compares reasons, not moves, with half the moves. - The README's engine-limits table (
README.md:576) still ends with "elseTouchPacingError"; addTouchStarvationErrorthere, as the prose below it now does. - The comment at
harness.interaction.browser.spec.ts:325-326says no step waits for WebKit's answer, which the rewritten paragraph right below it contradicts. - Miss: the NavigationStack proof (
NavigationStack.interaction.browser.spec.ts:1037-1046) still chooses betweenTouchPacingErrorandTouchPageStallErroritself, so its flick on a starved runner never reads asTouchStarvationError; routing it throughtouchPlayErrorwith frames would close that gap.
The new unit cases each fail when their branch is removed, and the harness typechecks and passes its unit tests.
[Automated review]
A paced WebKit play whose page drew no frame across its late gap, with no page work holding the event, now fails as TouchStarvationError, not TouchPacingError. The touch driver reads the page's frames from the input run's frame log, after its first frame. The touch pacing self-test judges each move against its paced deadline, with no re-take. It names a move the runner woke late only when a control timer woke as late while the test process ran for under half of that wait.
9b36c38 to
8af3622
Compare
|
/approve-spec 8af3622 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, so once this draft is marked ready for review, a maintainer adds the ci:run label so CI runs.
The earlier blocking point is fixed: the touch pacing self-test now fails a pacer that holds its own process past each deadline (a 20 ms spin or a 20 ms held send fails 3 of 3 in WebKit), while a 30 ms stop from outside the process is still named as the runner. The off-pace count, the README's engine-limits table, the self-test comment and the NavigationStack proof's error choice are fixed too, and each new unit case fails when its branch is removed.
[Automated review]
cixzhang
added
the
ci:run
|
/approve-spec 9b4050f |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The ci:run label is already applied, so CI runs once this draft is marked ready for review.
The merge of main leaves the change itself untouched. On this head the touch pacing self-test still fails a pacer that holds its own process past each deadline (a 20 ms spin or a 20 ms held send fails 3 of 3 in WebKit), while a 30 ms stop from outside the process is named as the runner, and each new touchPlayError unit case fails when its branch is removed.
[Automated review]
|
/approve-spec 9f1de34 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The ci:run label is already applied, so CI runs once this draft is marked ready for review.
The merge of main leaves the change itself untouched. On this head the touch pacing self-test still fails a pacer that holds or delays its own sends (a 20 ms spin, a 20 ms held send or a 20 ms late sleep each fail 3 of 3 in WebKit), while a 30 ms stop from outside the process is named as the runner, and each new touchPlayError unit case fails when its branch is removed.
- Miss: (non-blocking)
TouchStarvationError's message leaves out the gesture-wide frame gap that the README says every pacing error names; usingdescribeFault(fault)there, or narrowing that README sentence, keeps them in step.
[Automated review]
There was a problem hiding this comment.
Changes requested: one fix in the WebKit touch pacing self-test.
- Miss: the self-test credits a late move to the runner even when the control timer woke on time (
harness.interaction.browser.spec.ts:433adds 4 ms of slack whateverrunnerMsis), so a pacer that leaves every move 3 ms past its paced deadline passes in WebKit astouch-runner-latewith "control 0 ms late", wheremain's self-test fails it; credit the runner only when the control itself woke late, for examplemove.runnerMs > 2 && move.ms <= move.runnerMs + 4.
The runner-stall case this pull request targets works as intended: a 30 ms outside stop of the test process before every third step passes and is named, while a pacer that spins 6 ms or 20 ms past each deadline still fails, and every new touchPlayError unit case fails when its branch is removed. The earlier non-blocking note on TouchStarvationError's message (touch.ts:887) still stands.
[Automated review]
The touch pacing self-test credited a late move to the runner when it left no more than 4 ms after the control timer's wake, even when that timer woke on time. A pacer that left every move 3 ms past its paced deadline then passed as the runner's. A move is now the runner's only when the control itself woke more than 2 ms late, and a wake counts only when the test process ran for less than half of how late it was. The test reads the moves and the control's wakes on the clock the play paces on, to a fraction of a millisecond: each played event now carries its send and its return on that clock. TouchStarvationError also names the gesture's longest frame gap, as the other pacing errors do.
|
/approve-spec 0d9e667 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The ci:run label is already applied, so CI runs once this draft is marked ready for review.
Both points from the last review are fixed. The touch pacing self-test now credits a late move to the runner only when its control timer itself woke late, so a pacer that leaves every move 3 ms past its paced deadline fails again, while a 30 ms outside stop of the test process still passes and is named touch-runner-late. TouchStarvationError now names the gesture's longest frame gap, pinned by a unit case.
[Automated review]
|
/approve-spec 07443c3 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The ci:run label is already applied, so CI runs once this draft is marked ready for review.
The newest commit only merges main and changes nothing this pull request does: the harness still typechecks and passes its unit tests, and the WebKit touch pacing self-test still passes a healthy take and fails a pacer that leaves every move 3 ms late.
[Automated review]
Maintainer impact
A paced WebKit touch play on a starved runner failed as
TouchPacingError, as a harness-stalled dispatch does. A reader could not tell the two apart. The touch pacing self-test failed 20 of 20 under four CPUs and eight busy loops. Its one re-take hid the cause.Intended invariant
A paced touch play stretched by a starved runner fails by name, as
TouchStarvationError, with no pass, re-take or re-pace. The touch pacing self-test fails only on a move the harness itself sent late.Change
TouchStarvationError: the page drew no frame for longer thanFRAME_STARVATION_MSacross the late gap, and no listener hold or wait explains it. The lift stall,TouchPageStallErrorand a named wait still come first.armInputRun's frame log, after its first frame.FRAME_STARVATION_MSmoves to a leaf module.touch-runner-late) only when a control timer woke over 2 ms late. The process must have run under half that delay.touchPlayError, with the page's frames.Evidence
TouchPageStallErrorcomes first.Scope
Testing
tsc: 0 errors.pnpm review:pre: 0 findings against main.