Repository navigation
test(interaction-harness): too little to judge is named starvation - #10788
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 ·
|
c6692f9 to
1c6cd25
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
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 ready for review, a maintainer adds the ci:run label so CI runs.
The proofs now name a run with too few compositor frames as starvation while keeping every component failure on the frames shown, and the new tests fail against the current busy.ts and pass with this change.
Optional follow-ups:
internal/interaction-harness/README.md:386says a pool with a run that began too late "fails instead", but the code keeps that failure only when late runs leave the pool short of judged runs, so a pool with enough judged runs and too few control frames throws starvation.- The comments at
internal/interaction-harness/src/harness.interaction.browser.spec.ts:2085-2086and:2112-2113still say the verdict fails where the test now expects a throw.
[Automated review]
|
/approve-spec 1c6cd25 |
There was a problem hiding this comment.
Changes requested: one fix in the pooled starvation error.
- Miss: when a pool is short of judged runs and its judged run has fewer than four control moves,
pooledBusyProoftakes the error's frames and longest gap from the judged runs only (internal/interaction-harness/src/busy.ts:2113) but words them for every block (:2118), so the new four-run case reports "5 frames while 4 blocks held the main thread" when they drew 20, and a 16.7 ms gap when the other blocks drew nothing for 90 ms;const counted = tooFewRuns ? runs : runs.filter((_, i) => judged[i]);fixes it, and the test should assert the error's frames and gap.
The rest holds: the new tests fail against the current busy.ts and pass here, and every component failure under the floor still fails. Since this head, main changed busy.ts (#10772) and GitHub reports a conflict, so a rebase is needed. The two earlier non-blocking notes (the README's late-run sentence and the stale comments in the harness's pooled browser test) still apply.
[Automated review]
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.
1c6cd25 to
e1f9922
Compare
cixzhang
added
the
ci:run
|
/approve-spec e1f9922 |
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. It runs once the pull request is marked ready for review; the ci:run label is already on it.
The busy proofs now name an under-floor run as starvation and keep every component red, and the pooled error's frame count and gap now match its wording, with tests that fail on the current busy.ts and pass here. The earlier notes on the README's late-run sentence and the stale comments in the harness's pooled browser test are fixed.
Optional follow-ups:
- Miss (non-blocking):
FrameStarvationError's JSDoc ininternal/interaction-harness/src/frames.tsstill says the error means no frame came for longer thanFRAME_STARVATION_MS, while the busy proofs now also throw it when the control moved on too few frames with no long gap. - Miss (non-blocking): no test fails when the 50 ms bound in
busyWindowProof's throw (busy.ts:1399) or the pool's!lateFailureguard (busy.ts:2506) is removed; one unit test each would pin the two README rules they implement.
[Automated review]
| // frame or a longer stall. That failure is the component's red. | ||
| if ( | ||
| starvation != null && | ||
| (starvation.longestGapMs > FRAME_STARVATION_MS || !componentFailed) |
There was a problem hiding this comment.
Non-blocking: no test pins the starvation.longestGapMs > FRAME_STARVATION_MS half of this condition. With it removed all 151 busy tests still pass, and a starved run (no frame for 200 ms, control on 3 frames) whose component froze returns its red instead of throwing. A unit test with starvedCast() and a component frozen through the block would pin the README's "throws either way".
| // pool's failure. So does a judged run's component red. | ||
| const lateFailure = judgedReads.length < minJudgedRuns && lateRuns > 0; | ||
| const componentFailed = failedRuns.some((failed, i) => judged[i] && failed); | ||
| if ((tooFewFrames || tooFewRuns) && !lateFailure && !componentFailed) { |
There was a problem hiding this comment.
Non-blocking: no test pins !lateFailure. With it removed all 151 busy tests still pass, and a pool of two runs that settled before their block beside one 90 ms run (control on 3 frames) throws starvation instead of keeping "1 of 3 runs judged, under 2: 2 began too late". The existing late-run test uses a 600 ms judged run, so tooFewFrames never holds there.
|
/approve-spec 1c1bef1 |
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; it runs once the pull request is marked ready for review, and the ci:run label is already on it.
The busy proofs now name an under-floor run with no component failure as starvation and keep every component red on the frames shown, and the current busy.ts fails 10 of the new and changed tests.
Optional follow-ups:
FrameStarvationError's JSDoc ininternal/interaction-harness/src/frames.tsstill says the error means no frame came for longer thanFRAME_STARVATION_MS, but busy proofs now throw it with shorter gaps.- No test pins the 50 ms half of the throw condition in
busyWindowProofor the late-run guard inpooledBusyProof; both are flagged in the inline comments on those lines.
[Automated review]
Maintainer impact
A busy-main-thread proof can fail as if the component stalled when the runner presented too few frames. The control moved on under four frames, and no gap passed 50 ms.
Intended invariant
A busy proof fails the component only on frames the compositor presented. Under the control's floor, a run with no component failure on the frames shown is too little to judge. Every timeout and floor is unchanged.
Change
busyWindowProof: a control underminJudgedFramesalone sets the starvation. A missed frame or a stall longer than the control's stays a verdict failure.maxMedianFrameMsjudges the cast, so it keeps no red.pooledBusyProof: too few judged runs or control frames throws the same way unless a judged run failed. A pool left short of judged runs by a run that began too late keeps its failure.Evidence
busy.ts(542b12003dd74fe22cfe160797e3716433c891c9), 10 tests fail. All pass at this head, e1f9922afdc43cc7fd4c1816a671834a00514849.Scope
Testing
busy.control.test.ts,busy.webkitclock.test.ts,busy.test.ts,busy.unjudged.test.tsandbusy.clock.test.tspass, 151 tests.busy.ts, to read each proof's verdict and starvation.tsc --noEmit: 0 errors. ESLint: 0 errors in the changed files. Prettier: clean.node scripts/release/changeset-coverage.mjs prreports nothing that ships.