test(producer): gate source tests by execution lane#2146
Conversation
|
🤖 Automated code-review pass — clean ✅ Reviewed the incremental diff — no blocking issues found. Checked in depth:
|
a254155 to
8e2f7fc
Compare
eec5354 to
b24c7bb
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
#2146 — test(producer): gate source tests by execution lane
Verdict: LGTM 🟢
Introduces a test classification system for producer: every .test.ts under src/ is classified by runner (bun:test vs vitest, detected from imports) and lane (unit vs integration, from explicit manifest). Key SSOT properties:
- Single runner per file enforced.
classifyTestSourcerejects files importing both or neither runner — no ambiguity. - Manifest staleness caught.
discoverProducerTestsvalidatesINTEGRATION_TEST_FILESagainst what's on disk — a renamed/deleted test errors at classification time, not silently drops from the lane. - Bun mock isolation. Each
bun:testfile runs in a separate process somock.moduleregistries don't leak. Vitest files batch into a singlevitest runinvocation (vitest handles isolation internally).
CI gets a new producer-source-tests job with matrix: [unit, integration] — clean separation from regression shards. The regression-harness.ts becomes import-safe via import.meta.url guard.
The captureStreamingStage.test.ts mock additions are engine API surface keeping — new exports like captureFrameToBufferPipelined, DrawElementVerificationError, getCapturePerfSummary etc. need stubs so the test compiles against the current engine. No behavioral change.
data-no-timeline on test fixture compositions is a correctness fix — minimal test compositions shouldn't trigger timeline binding.
Review by Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Final direct pass at current head b24c7bb. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at b24c7bbc.
Substantive test-infra work. The classifier + lane split is right — read through the whole surface. Highlights:
- Runner classification (
test-classification.mjs:35) rejects both "imports neither" and "imports both" — no silent-drop default (feedback_dispatch_map_silent_drop_default_typessafe). TheINTEGRATION_TEST_FILEShardcoded allowlist is caught for staleness attest-classification.mjs:56(throws if a listed file no longer exists), so drift is loud, not silent. - Process-per-file for bun tests at
run-test-lane.mjs:33with the explanatory comment ("Bun's mock.module registry is process-global"). Correct hazard call-out; each.test.tsgets a clean process somock.modulefrom one file can't bleed into another. regression-harness.ts:1514is-main-module guard —if (process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1])). Prevents the harness from auto-running when imported by a unit test.- Engine mock shape in
captureStreamingStage.test.ts:23-33— I verified againstpackages/engine/src/index.ts:captureFrameToBufferPipelined,captureFramesBatchPipelined,DrawElementVerificationError,completeDeferredDrawElementInitare all real exports (notfeedback_external_dep_mock_shape_verify_runtimehallucinations). The locally-declaredclass DrawElementVerificationError extends Error {}is legitimately used as the mocked stand-in — the real class lives inframeCapture.ts:201inside@hyperframes/engine. data-no-timelineadditions to fixture divs (crossWorkerIdempotency.test.ts:44,renderChunk.test.ts:44) are anchored — the attribute is honored atengine/services/frameCapture.ts:1178, 1231. Legitimate opt-out for fixtures without GSAP timelines.allowSystemFontCapture: falsein the 3deterministicFonts-failClosed.test.tscases: the option's declaration isn't in this PR (I couldn't find it onmain), so it must have landed in an earlier stack PR. Assuming that's the case, explicit-false here is proper test-isolation to keep the unit lane from reaching into host font paths.
A couple of nits — none blocking:
bun.lockversion bumps0.7.45 → 0.7.48across ~11 packages (aws-lambda,cli,core,engine,gcp-cloud-run,lint,parsers,player,producer,sdk,shader-transitions,studio,studio-server) look like Graphite auto-sync noise, not test-infra intent. If they are — fine; worth a line in the PR body so reviewers don't hunt for a version-bump rationale.test:regressionmoved fromproducer/test→producer/test:regression. The top-leveltestnow runstest:unit. Any tooling (docs, contributor CLAUDE.md, shell aliases, CI in adjacent repos) that ranbun run testexpecting regression coverage now gets the unit lane. Not visible from this diff; heads-up worth landing with the batch.INTEGRATION_TEST_FILESallowlist grows manually. Staleness is caught but new-file adds require editing this list. Cheap alternative:describe.integration()marker + parser scan. Not for this PR — landing this as-is is fine and the manual list is loud enough that new tests won't accidentally slip to the unit lane.
LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: 🟢 LGTM at b24c7bb — with one scope-flag on the harness auto-run guard
Endorses Miga + Rames-D on classifier + lane split. The classifier at test-classification.mjs:35 fails loud on both "imports both runners" and "imports neither", and the INTEGRATION_TEST_FILES staleness check at test-classification.mjs:56 catches renames — those are good, no silent-drop paths. Adding one finding on top that neither reviewer surfaced: this PR carries a real bug fix that the test(producer): title doesn't advertise.
Findings
F1. Load-bearing scope hidden by title — regression-harness.ts auto-run guard. packages/producer/src/regression-harness.ts:1513-1523 moves the top-level void run().catch(...) behind if (process.argv[1] && fileURLToPath(import.meta.url) === resolve(process.argv[1])). This is not "gate tests by lane" — it's fixing a real side-effect bug where packages/producer/src/utils/streamDurationParity.test.ts:2 imports checkStreamDurationParity from ../regression-harness.js, and the prior top-level auto-run fired the entire regression suite as a module-init side effect during that test's import. The guard is load-bearing. Every prior legitimate caller (Dockerfile.test's bunx tsx src/regression-harness.ts … entrypoint, every producer test:* script) still triggers main() because process.argv[1] matches the harness path. Correct fix; the PR body should acknowledge this hunk exists.
F2. Classifier is direct-import only — theoretical, non-blocking. /\bfrom\s+["']bun:test["']/.test(source) at test-classification.mjs:35 doesn't chase transitive imports through helper modules. Today every producer .test.ts imports its runner directly (verified via search/code — all 20+ hits are the test files themselves). If a future author factors shared setup into _testHelpers.ts and moves the bun:test/vitest import there, the classifier will throw "imports neither runner" — fail-loud, so acceptable. Worth a comment near the regex noting the direct-import assumption.
F3. HYPERFRAMES_TEST_LANE — set but not read (info). Phase-1 recon anticipated per-file env gates. Actual behavior: run-test-lane.mjs:22 sets HYPERFRAMES_TEST_LANE in the sub-process env; no .test.ts in the diff reads it as a runtime skip gate. The 4 modified test files got unrelated mock-signature fixes (deterministicFonts allowSystemFontCapture, data-no-timeline, expanded mock.module(...) shapes) to make them lane-runnable. No inconsistency issue.
F4. CI changes path filter covers producer-source-tests inputs — verified. ci.yml paths-filter code includes packages/**, scripts/**, package.json, bun.lock, tsconfig*.json, Dockerfile*, .github/workflows/** — so packages/producer/scripts/** and packages/producer/**/*.test.ts route to the guarded producer-source-tests job (ci.yml:238) correctly.
Adversarial pass
F1 initially read as a scope-drift request-changes finding; downgraded to LGTM-with-scope-flag because the code IS right and the title-fix ask is a body edit, not a new commit. F2 is theoretical (no such helper exists today) — kept only as a comment-worthy nit. Considered flagging the auto-run guard change as a Windows-path edge case (resolve() case-sensitivity vs fileURLToPath on C:\...) — dropped because HF's CI runs on Linux and Windows dev isn't a supported target.
R1 by Via
b24c7bb to
7c45972
Compare
38d62f7 to
179c942
Compare
7c45972 to
a9d7223
Compare
a9d7223 to
9e7b119
Compare

What
Classify every producer source test into exactly one Bun or Vitest unit/integration lane and run those lanes in CI.
Why
Producer source tests were present but outside the package and CI test commands, creating a false-green safety net.
How
Add a machine-checked classifier, explicit lane runners, and separate unit versus visual regression commands.
Test plan