fix(ci): keep contact sheet test lightweight#2492
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
R1 — APPROVE. Right-direction fix, not a band-aid.
Freshness: head ad634d58, base main, mergeable=MERGEABLE (BLOCKED on approval + Windows checks still IN_PROGRESS as of writing).
Claim verification
- "One line, test-only" — confirmed. Files touched:
packages/cli/src/capture/contactSheet.test.ts+1/-0. That's it. - "Production default is
cellWidth: 600" — confirmed atpackages/cli/src/capture/contactSheet.ts:39(cellWidth = 600) with JSDoc at :20, and prod call sites (:192,:225,:254) explicitly pass their own sizes. Upscale mechanism verified at:51(const scale = cellWidth / srcW;) — 16→600 = 37.5×. - "1,835 passed, 2 skipped" — not independently reproduced, but the suite-size claim (142 files now vs 114) is consistent with recent CLI growth.
- Envelope clean: single commit, no
Co-Authored-By: Claude/ ChatGPT trailer.
Band-aid check (Miguel bar) — this passes.
The test asserts one thing: sharp(out).metadata() returns format: "png" (line 48). Nothing about scaling, layout, or SVG label rendering is asserted. The 37.5× upscale + SVG label composite + PNG encode of an ~600×338 sheet was pure incidental work — the assertion holds identically at 16×9. So this isn't hiding a Windows Sharp perf regression; it's removing work the test never intended to do. Prod paths (compare.ts, regressions, fallow presets) continue to run cellWidth=600/480/thumbSize on real inputs — no coverage there is affected because none of it flows through this one unit.
Extrapolation — the root cause chain the PR body draws (Windows runner slowness + 114→142 parallel-file pressure + 37.5× fixture upscale > 20s Vitest budget) is the actual failure mode. The surgical fix targets the only lever this test controls (the 37.5×), which is also the only irrelevant lever. Correct altitude.
Coverage integrity — createContactSheet has only this one unit test; the assertion is format-detection, which does not depend on cellWidth. Nothing lost.
Soft note (non-blocking, follow-up territory)
The default cellWidth = 600 is a footgun for future unit tests using tiny fixtures — a natural next step would be a one-line JSDoc addition ("If your source images are <100px, pass cellWidth matching source width to avoid multi-second Sharp upscales") on the option, or a createContactSheetForTest preset. Not a blocker.
Wait for the Windows-latest checks (currently IN_PROGRESS) to go green before merging — that's the dispositive proof.
— Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive review at exact head ad634d584723ab70e1c2b59e7bcfcf8261dcfb45; Via already covered the production-call-site and coverage-preservation audit.
Strengths:
packages/cli/src/capture/contactSheet.test.ts:40-48now makes fixture scale explicit while preserving the test's sole contract: PNG encoding selected by the output extension.packages/cli/src/capture/contactSheet.ts:39-53keeps the production600pxdefault and scaling behavior unchanged; the patch is exactly +1/-0 in a test file.
Independent evidence:
- Pulled the cited failing Windows log: this exact test exhausted the 20,000 ms Vitest timeout with no assertion failure while 1,828 sibling tests passed.
- Focused test passed three consecutive local runs; test execution was 20-55 ms.
- Direct Sharp smoke confirmed both widths still emit PNG; the explicit fixture width produced a 44x43 sheet versus 1212x372 at the production default, removing irrelevant upscale/composite work.
- Fresh current-head CI is fully green, including
Tests on windows-latest(8m42s) andRender on windows-latest(8m7s).
No blockers or additional findings.
Verdict: APPROVE
Reasoning: This is a narrowly scoped test-cost correction that preserves the asserted encoding contract and production behavior, directly addresses the observed Windows timeout, and is green on the exact affected lane.
— Deepwork
What
Keep the contact-sheet PNG unit test lightweight by setting its cell width to match the 16px fixture width.
Why
The Windows test job on main timed out in
packages/cli/src/capture/contactSheet.test.ts; no assertion failed.The test creates 16×9 fixtures but exercised the production default
cellWidth: 600, making Sharp upscale both inputs 37.5×, render SVG labels, composite, and PNG-encode a production-sized sheet while CLI test files run in parallel. This test already took 5.845s in its original green Windows PR run. The CLI suite has since grown from 114 to 142 files, and on the current Windows runner image the test exceeded Vitest's 20s timeout.Failing job: https://github.com/heygen-com/hyperframes/actions/runs/29432470663/job/87410880478
How
Set
cellWidth: 16in the unit test. The behavior under test is unchanged—a.pngoutput path must produce PNG—without doing irrelevant production-sized image work.Test plan
bun run buildbun run --cwd packages/cli test— 1,835 passed, 2 skippedbunx oxlint packages/cli/src/capture/contactSheet.test.tsbunx oxfmt --check packages/cli/src/capture/contactSheet.test.ts