Repository navigation
test(CircularProgress): the tracing test judges a window it marks inside the change - #9450
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 |
|---|---|---|
| lab/CircularProgress · Storybook | 8.5 kB (0) · 1.1 kB CSS (0) | 597 (0) |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: core 5 rows; run pnpm api:report.
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
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.
Requesting changes. The fix works: tracing now starts before the change, and only a marked second inside the change is judged. But the new window logic re-implements traceRendering and countBetween from @astryxdesign/interaction-harness, which exist for this exact no-Layout/Paint proof. Please run the change inside traceRendering, count the judged second with countBetween, and drop the spec's own trace parsing; the inline comment sketches the shape.
[Automated review]
| const mark = (name: string) => | ||
| trace.traceEvents.find( | ||
| e => e.cat === 'blink.user_timing' && e.name === name, | ||
| )?.ts ?? NaN; | ||
| const from = mark('window-start'); | ||
| const to = mark('window-end'); | ||
| expect(to - from).toBeGreaterThanOrEqual(1_000_000); |
There was a problem hiding this comment.
This marked window re-implements traceRendering + countBetween from @astryxdesign/interaction-harness (already imported here), which trace blink.user_timing marks and count Layout/Paint on the page's main thread between them. Wrapping setArgs and the waits in traceRendering(page, async () => …), returning performance.timeOrigin + performance.now() for the window's bounds, and counting with countBetween(trace, from, to) keeps one path for this proof and lets the local trace parsing go.
|
Done in 1d62d9d. The change runs inside The seams still hold: the change starting 2.5 s after tracing passes, and a per-frame repaint inside the second fails (Paint 120). |
There was a problem hiding this comment.
Recommending approval. The value change now runs inside traceRendering and the judged second is counted with countBetween, so the duplicated trace parsing from the first revision is gone, and the test still fails a fill that repaints every frame. One optional cleanup: the description's Change and Testing sections still describe the first commit's in-page performance.mark window and the gates run at that commit.
[Automated review]
…ide the change "a value change lays out and paints nothing per frame" stretched the change to 3 s, waited 200 ms, started tracing, waited a second, and then required the change to still be running. Starting tracing takes no fixed time: on a loaded machine it took long enough that the second ended after the change did, and the test failed on `moving` with nothing wrong in the product (1 run in 5 with one worker). Tracing now starts before the change, so the time it takes to start is not taken from the change. The page marks the window the test judges in the trace itself (performance.mark, recorded under blink.user_timing): from two frames after the change's animations are ready, 200 ms in, to a second later, where it also checks that the change is still running. Layout and Paint are counted on the renderer's main thread inside that window only. The waits are the same as before.
…aceRendering Review: the marked window re-implemented the interaction harness's traceRendering and countBetween, which trace the page's main thread between marks and count its Layout and Paint events by time. The change now runs inside traceRendering, which starts tracing before it as before, and the judged second is counted with countBetween between the page times the test reads at its start and end; the spec's own trace parsing is gone.
1d62d9d to
aa8b44d
Compare
There was a problem hiding this comment.
Recommending approval. The value change runs inside traceRendering and the judged second is counted with countBetween, so tracing's start-up no longer eats into the change, and the test still fails a fill that repaints every frame. The rebase onto main leaves the test's own change as it was approved.
[Automated review]
cixzhang
added
the
ci:run
Maintainer impact
CircularProgress's "a value change lays out and paints nothing per frame" fails now and then on a loaded runner with nothing wrong in the product:
expect(moving).toBe(true)readsfalse. It failed in 1 of 5 runs with one worker, and 1 of 40 pinned to two cores.The test stretches the change to 3 s, waits 200 ms, starts tracing, waits a second, and then requires the change to still be running. Starting tracing takes no fixed time. When it ran long on a loaded runner, the second ended after the change did.
Intended invariant
The test judges a full second inside the change, however long tracing takes to start. Over that second, nothing on the main thread lays out or paints.
Change
traceRendering, which starts tracing before it. The time tracing takes to start is not taken from the change.countBetween; the spec's own trace parsing is gone. The waits are the same as before.Evidence
expect(moving).toBe(true).Scope
Testing
packages/lab/src/CircularProgress/__tests__/CircularProgressMotion.a11y.chromium.spec.tsin Chromium, against the built Storybook.Local gates at aa8b44d:
pnpm lint(0 errors);