Repository navigation
test(interaction-harness): a block that begins after the motion ended throws BusyWindowTooLateError - #9505
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.
Requesting changes for one gap in the new during wait. When it passes over an animation that was already in its held tail, and that animation finishes or is cancelled before the timeout (Core's animator cancels held runs once the curve ends), blockMainThread rejects with "no animation started" rather than BusyWindowTooLateError. For example, the test's keyframes started 1.5 s into a 3 s run with timeout: 2_000 give that message, while a run still held at the deadline gets the named error. Remembering the held run the wait saw and rejecting with BusyWindowTooLate at the deadline, plus a browser case where that run ends before the timeout, would close it.
[Automated review]
| // In its held tail: started while the page waited, or what is left | ||
| // when the wait times out. | ||
| const late = | ||
| ran.find(a => !before.has(a)) ?? | ||
| (performance.now() > deadline ? ran[0] : undefined); |
There was a problem hiding this comment.
An animation already in before and in its held tail is only read again here, at the deadline, through ran[0]. If it finishes or is cancelled first (Core's animator cancels a held run once the main thread passes its curve end), ran is empty and the wait falls through to "no animation started". Remembering the held run the wait saw, for example heldSeen ??= ran[0] when nothing in ran is running, and using it here would keep this a BusyWindowTooLateError.
|
Thanks, fixed in 3561649.
|
There was a problem hiding this comment.
This looks ready to approve: the follow-up commit closes the earlier gap, so a held run the wait saw that ends before the timeout now rejects with BusyWindowTooLateError instead of "no animation started". The new browser case in harness.interaction.browser.spec.ts pins that path, and nothing else needs to change.
[Automated review]
3561649 to
d5fe905
Compare
There was a problem hiding this comment.
Requesting changes for one gap in the proof half of this change. The held-tail handling in blockMainThread with during works as described.
busyWindowProoftakes the settle time from the first frame at or afterwindow.from(busy.ts:696-709), so on a one-in-six screencast a block that starts 100 ms after the value settled still returns "judged = 0 frames < 4: the animation under test was not moving during the block" in 4 of 6 frame phases, although the title and README say such a block throwsBusyWindowTooLateError.
Also throwing when the last frame before the block and the first frame in it are both within the bound, with settledAtMs read from the earlier frame and a one-in-six unit case, catches all six phases without changing the existing cases. Narrowing the title and README to a frame in the first marginMs would work too.
- Not blocking: the branch now conflicts with
maininbusy.tsandbusy.test.tsafter #9545. Once rebased, the new throw sits in thesettledWithinPxblock, and its message should readsettledWithinPx, notsettledWithin.
[Automated review]
| // block started after the motion had settled, so the window holds | ||
| // nothing to judge. That is when the block ran, not how the component | ||
| // moved. | ||
| if (arrived != null && arrived.t <= window.from + marginMs) { |
There was a problem hiding this comment.
On a sparse screencast this misses a late block. The first frame at or after window.from can land after marginMs even when the frame before the block already shows the final value. With one frame in six and the value settled 100 ms before the block, 4 of 6 phases fall through to the "judged = 0 frames < 4" verdict. Checking the last valued frame before window.from as well, with settledAtMs read from it, catches all six phases.
There was a problem hiding this comment.
Fixed in 0d4adb1. When the block's first frame comes after the judged part begins, the proof now also reads the last frame before the block. If that frame and the block's first frame are both within the bound, it throws BusyWindowTooLateError, with settledAtMs from the earlier frame. A new unit case keeps one frame in six, with the value settled 100 ms before the block, and all six phases throw; it fails without the new rule. The message now prints settledWithinPx.
blockMainThread's `during` counted an animation once it had run `minProgressMs`, however far along. On a starved runner the page first looked when a curve was already in its held tail (its keyframes ending in a run of identical values), blocked there, and the busy proof then judged an empty or negative window, which read as the component failing. The wait now counts an animation only before its curve's held tail, read by the same curve end `waitForRest` reads, now defined in its own module so the wait can install it without the animation recorder. When the only animation running there started while the page waited, or the wait times out with only such animations, it rejects with BusyWindowTooLateError, with the times. It keeps the first held run it saw, so a run that finishes or is cancelled first still names the late block. busyWindowProof throws the same error when the tracked value had settled before the judged part began; a short positive window stays a verdict failure. It also reads the frame before the block, so a sparse screencast whose first frame in the block comes after the judged part begins still names a block that began late.
d5fe905 to
0d4adb1
Compare
There was a problem hiding this comment.
This looks ready to approve. The frame-before rule closes the sparse-screencast gap: a block that began after the value settled now throws BusyWindowTooLateError in every phase of a one-in-six cast. The held-tail handling in blockMainThread with during works as described in Chromium and WebKit.
One non-blocking point you may want to fix while you are here:
- The proof's error does not report when the value settled.
settledAtMsis the first in-block frame within the bound, or the last frame before the block (busy.ts:788-797), so a value that settled 100 ms early reports anywhere from +17 ms to −67 ms by cast phase, and 0 on a dense cast. Walking back through the frames still within the bound would report the real settle time.
[Automated review]
| before = f; | ||
| } | ||
| const firstInBlock = valued.find(f => f.t >= window.from); | ||
| const settledBy = |
There was a problem hiding this comment.
Non-blocking: settledBy is the first in-block frame within the bound, or the last frame before the block. It is never when the value settled. On a one-in-six cast, a value that settled 100 or 300 ms before the block reports settledAtMs of 0, +17, −67, −50, −33 or −17 ms by phase. A dense cast reports 0 for a settle 50, 100 or 200 ms early, and the message reads "from 0 ms after the block began". Walking back from that frame while Math.abs(value - final) <= settledWithinPx holds would report the settle time the error promises.
| // nothing to judge. That is when the block ran, not how the component | ||
| // moved. | ||
| if (settledBy != null) { | ||
| const presented = tracked.filter( |
There was a problem hiding this comment.
Nit: this presented shadows the presented option, which holds the compositor's presentation times. These are screencast frames, so delivered would keep the two apart in both the code and the message.
cixzhang
added
the
ci:run
Maintainer impact
On a starved runner, a busy-main-thread proof can block after the motion it is meant to catch has ended, and the result reads as the component failing.
blockMainThread'sduringcounted an animation once it had runminProgressMs, however far along it was.busyWindowProofthen judged an empty or negative window, for example "−19 ms judged … 0 frames presented … the compositor presented too little in this window to judge".Intended invariant
A block that begins after the motion it was meant to catch had ended is named for what it is, a red that carries the times:
BusyWindowTooLateError. It never reads as a judgment of the component, and it is never a pass.Change
duringcounts an animation only before its curve's held tail. The curve end is read the waywaitForRestreads it. It rejects withBusyWindowTooLateError(current time, curve end) in two cases:busyWindowProofthrows the same error when the tracked value had settled before the judged part began (settle time, judged start, frames presented). A short positive window stays a verdict failure ("N frames < 4").curve-end.ts, so the wait installs it without the animation recorder.installCurveEndinstalls it and the hold in one round trip, as before.Evidence
BusyWindowTooLateError(curve end 900 ms, current time at least 1,500 ms, started while waiting);BusyWindowTooLateErrorfrom the run it saw (curve end 900 ms, started before the wait).BusyWindowTooLateErrorin all 6 frame phases, with the settle read from the frame before the block (for example, "from 50 ms before the block began").during: same time, pinned, 8 of 8 on main and 8 of 8 here.Scope
Testing
internal/interaction-harness: its unit tests andharness.interaction.browser.spec.tsin Chromium and WebKit (playwright.interaction.config.ts);packages/corespecs that useblockMainThreadwithduring.Rebuilt on
main547d90a.busy.tsandbusy.test.tsconflicted with three changesmainmade since this change's first base:The rebuild keeps all three. The late-block branches time out on that real clock, and the settled-before check uses the bound
busyWindowProofnow computes (settledWithin, orsettledWithinStepscast steps), so its message prints that bound. The other four files carry this change's diff unchanged.Local gates at 0d4adb1:
--cpus=4,CI=1, 1 worker): 112 passed, 17 Chromium-only skipped;pnpm review:pre: no findings.Local gates at 3561649, on the earlier base:
pnpm lint(0 errors);