Skip to content

test(engine): real-Chrome tests get 30 s per test on slow runners, not 5 s - #7456

Merged
miguel-heygen merged 1 commit into
mainfrom
oncall/browser-test-timeout
Nov 9, 2026
Merged

miguel-heygen merged 1 commit into
mainfrom
oncall/browser-test-timeout

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

What

Every engine test that drives a real Chrome (the browser project in packages/engine/vitest.config.ts) now gets 30 s per test, the same as its hooks already get. It used to be vitest's default 5 s unless the test set its own.

Why

frameCapture-verifyArming.browser.test.ts timed out at 5 s on windows-latest and took a PR out of the merge queue twice ("Test timed out in 5000ms" on two of its tests). It is not a hang: the test takes under a second on a normal runner. It was the one real-Chrome engine test that never asked for the 30 s most of its siblings set by hand. Setting the budget once on the project covers it and any browser test added later, instead of each test remembering to pass 30_000.

A test that sets its own limit keeps it: videoFrameTransport's 5 s guard against a stuck download and the 15 s, 45 s and 60 s limits elsewhere are unchanged.

Related work

None.

How

One line: testTimeout: 30_000 next to the existing hookTimeout: 30_000 on the browser project.

Size

One line on purpose: it is a lone test-config fix that unblocks the merge queue, and no open PR touches the engine's test config to carry it.

Test plan

Chrome and vitest held to 4% of one CPU core (a root-made systemd scope with CPUQuota=4%) on Linux, running npx vitest run --project browser src/services/frameCapture-verifyArming.browser.test.ts:

  • main: fails, 3 of 5 tests, each with "Test timed out in 5000ms", the same error as the Windows queue run.
  • this branch: passes 5 of 5; the slowest test took 7.5 s.

fails without the fix: packages/engine/src/services/frameCapture-verifyArming.browser.test.ts "takes the ground truth at the fixed fractions, each showing its own frame" times out at 5000 ms on main at 4% of one core (3 of the file's 5 tests do); with the fix, 5 of 5 pass.

Without the CPU limit, this branch passes 5 of 5, 3 runs in a row.

  • Unit tests added/updated (test config only)
  • Manual testing performed
  • Documentation updated (if applicable)
  • Comments follow CONTRIBUTING.md "Comments": they say why, not what, and a bug fix says what the code must do and how to reproduce the bug

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes for one reason: the PR body is missing the required line.

Blocker: this is a fix PR, so its body needs a fails without the fix: line that names the test. Right now the Test plan describes a failure on throttled main, but it never puts it on that line. Adding something like this resolves it:

fails without the fix: packages/engine/src/services/frameCapture-verifyArming.browser.test.ts under CPUQuota=4% (3 of 5 tests fail with "Test timed out in 5000ms" on main; 5 of 5 pass on this branch)

The code looks correct (no blocker):

  • Adding testTimeout: 30_000 on the browser project matches the existing hookTimeout. It only applies to the *.browser.test.ts set, so the unit project keeps vitest's 5 s default.
  • A timeout set on a single test still beats the project default, so the 5 s stuck-download guard in videoFrameTransport and the 15/45/60 s limits stay as they are.

At db9df57f, CI was still running when I reviewed (CI, CodeQL, Windows render verification, regression).

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at db9df57f (same head, body edited). Still requesting changes, for the same rule.

Blocker: the new line is there, but it doesn't name a test:

fails without the fix: on main, at 4% of one core, 3 of 5 tests fail with "Test timed out in 5000ms"; with the fix, 5 of 5 pass.

The rule needs the test named on that line, not only in the Test plan. Adding the file to the line resolves it, for example:

fails without the fix: packages/engine/src/services/frameCapture-verifyArming.browser.test.ts — on main, at 4% of one core, 3 of 5 tests fail with "Test timed out in 5000ms"; with the fix, 5 of 5 pass.

The code is unchanged and still has no blocker. CI for this head is still running (CI, CodeQL, Windows render verification, regression).

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving at db9df57f. The previous blocker is resolved.

  • The fails without the fix: line now names the file and the test: frameCapture-verifyArming.browser.test.ts › "takes the ground truth at the fixed fractions, each showing its own frame". That test exists at this head and sets no timeout of its own, so before this change it ran with vitest's 5 s default, and the new 30 s testTimeout on the browser project now covers it.
  • The code is unchanged since my last review: one line, limited to the browser project. Limits set on individual tests (the 5 s guard in videoFrameTransport, and the 15/45/60 s limits) still take priority.

CI for this head was re-triggered by the body edits and is still running (CI, CodeQL, Windows render verification, regression). Merge only after it's green.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit 4d29a64 Nov 9, 2026
139 of 140 checks passed
@miguel-heygen
miguel-heygen deleted the oncall/browser-test-timeout branch November 9, 2026 20:38
@miguel-heygen miguel-heygen mentioned this pull request Nov 9, 2026
3 of 4 tasks
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 9, 2026
…heygen-com#7456 and the heygen-com#7439 docs

heygen-com#7459: Studio renders take the same one-render-slot lock as the CLI and say
they are waiting; Cancel ends the wait.
heygen-com#7463: deleting an animated group parses its script once instead of once per
tween.
heygen-com#7454: seam-gate awaits the async player seek (both skill mirrors).
heygen-com#7456: the engine browser test project gets 30 s per test.
heygen-com#7439: current ChatGPT and Claude setup steps in the MCP guide.

Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
SanHsien added a commit to SanHsien/hyperframes that referenced this pull request Nov 9, 2026
…ve more; advance watermark

Twelve commits, PRs 7453-7468, issues 7457-7465 and eleven pin moves are
recorded with their evidence in the continued ledger, DECISIONS and UPSTREAM.
Adopted: heygen-com#7338, heygen-com#7459, heygen-com#7463, heygen-com#7454, heygen-com#7456 and the heygen-com#7439 docs; heygen-com#6732 was
already ported. Deferred: heygen-com#7455, heygen-com#7437. Not applicable: heygen-com#7453, heygen-com#7269, heygen-com#7464.
The watermark moves to 9e346f9, PR heygen-com#7468, issue heygen-com#7465; four new PRs are
pinned and the eleven moved pins re-pinned at their current heads. FORK.md
records the fork-only differences in the heygen-com#7338 port; the roadmap tracks the
standalone scripts that keep a bare mkdtemp and issue heygen-com#7458.

Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
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.

1 participant