Skip to content

fix(ci): keep contact sheet test lightweight#2492

Merged
jrusso1020 merged 1 commit into
mainfrom
fix/windows-contact-sheet-timeout
Jul 15, 2026
Merged

fix(ci): keep contact sheet test lightweight#2492
jrusso1020 merged 1 commit into
mainfrom
fix/windows-contact-sheet-timeout

Conversation

@jrusso1020

Copy link
Copy Markdown
Collaborator

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: 16 in the unit test. The behavior under test is unchanged—a .png output path must produce PNG—without doing irrelevant production-sized image work.

Test plan

  • Unit tests added/updated
  • bun run build
  • bun run --cwd packages/cli test — 1,835 passed, 2 skipped
  • bunx oxlint packages/cli/src/capture/contactSheet.test.ts
  • bunx oxfmt --check packages/cli/src/capture/contactSheet.test.ts
  • Documentation updated (not applicable)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 at packages/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 integritycreateContactSheet 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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-48 now 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-53 keeps the production 600px default 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) and Render 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

@jrusso1020 jrusso1020 merged commit dbdf03c into main Jul 15, 2026
43 checks passed
@jrusso1020 jrusso1020 deleted the fix/windows-contact-sheet-timeout branch July 15, 2026 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants