Skip to content

fix: give e2e cleanup hooks timeout headroom - #45

Open
usercao wants to merge 2 commits into
mainfrom
fix/e2e-cleanup-hook-timeout
Open

usercao wants to merge 2 commits into
mainfrom
fix/e2e-cleanup-hook-timeout

Conversation

@usercao

@usercao usercao commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Give Vitest's e2e cleanup hooks the same timeout headroom as e2e test bodies. This lets the tsserver fixture's own process-exit deadline report a stalled server instead of Vitest ending onTestFinished or afterAll first with a generic hook timeout.

The response-timeout parser now reserves that headroom below Node's maximum timer delay. The Vitest e2e project configuration derives both testTimeout and hookTimeout from the bounded response timeout, and focused tests cover the parser cap and project wiring.

Testing

  • corepack yarn vitest run test/unit/tsserver-timeouts.test.ts test/unit/vitest-config.test.ts --configLoader runner

All 13 focused tests pass.

Copilot AI balanced review requested due to automatic review settings October 30, 2026 03:28
@changeset-bot

changeset-bot Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: c32c0ae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the e2e cleanup timeout changes.

No open issues 🎉

What changed in this PR

This PR gives e2e cleanup hooks enough time for the tsserver fixture to report a stalled process before Vitest reports a generic hook timeout.

Changes:

  • Applies the same timeout headroom to e2e tests and hooks.
  • Caps the response timeout so adding headroom stays within Node’s maximum timer delay.
  • Adds focused regression tests, maintenance documentation, and a patch changeset.
File Description
vitest.config.ts Shares the derived timeout between e2e tests and hooks.
test/​unit/​vitest-config.test.ts Checks e2e project wiring and timeout settings.
test/​unit/​tsserver-timeouts.test.ts Tests acceptance and rejection at the response-timeout cap.
test/​e2e/​tsserver-fixture/​timeouts.ts Defines shared headroom and bounds the configured response timeout.
docs/​maintenance.md Explains cleanup timeout behavior.
.changeset/​calm-cleanup-hooks.md Records the cleanup timeout fix for release.

Review effort: Balanced

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ No critical issues — two minor suggestions inline.

Reviewed changes

  • Cleanup hook headroom: the e2e project now sets hookTimeout to the same responseTimeoutMs + E2E_TIMEOUT_HEADROOM_MS as testTimeout, so onTestFinished/afterAll cleanup can let the harness's own close() deadline fire first.
  • Bounded response timeout: MAX_RESPONSE_TIMEOUT_MS caps TSSERVER_RESPONSE_TIMEOUT_MS at MAX_TIMER_DELAY_MS - E2E_TIMEOUT_HEADROOM_MS, and parsePositiveInteger accepts an explicit maximum so the derived hook timeout stays inside Node's timer range.
  • Testable e2e config: the e2e project is lifted into exported e2eTestConfig / e2eProjectConfig, with e2eTimeoutMs derived once and reused by both timeouts.
  • Tests, docs, changeset: new parser-cap and config-wiring unit tests, a maintenance doc note, and a patch changeset.

I confirmed against Vitest 5.0.5 that both onTestFinished and afterAll use runner.config.hookTimeout (node_modules/vitest/dist/chunks/run.BPv-BBRv.js:3411 and :1972), so the premise holds. Focused tests, the full unit suite (937 tests), typecheck, lint, and format:check pass locally.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using big-pickle (free) | 𝕏

Comment thread .changeset/calm-cleanup-hooks.md Resolved
Comment thread test/unit/tsserver-timeouts.test.ts Resolved

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Production cap now covered: the responseTimeoutMs computation moved into exported getResponseTimeoutMs(value), so the unit tests exercise the MAX_RESPONSE_TIMEOUT_MS cap through the real entry point rather than re-supplying a bound. Tests cover the default, accepted values including the cap, and rejection at cap + 1 with the exact message.
  • Changeset dropped: the earlier .changeset/calm-cleanup-hooks.md was removed, since this is test-harness-only work with no user-visible change.
  • Rest unchanged: e2eTimeoutMs still derives responseTimeoutMs + E2E_TIMEOUT_HEADROOM_MS for both hookTimeout and testTimeout, keeping the derived timeout within Node's maximum timer delay.

Both prior review threads are addressed and resolved. Focused tests (17), typecheck, and lint pass at c32c0ae.

Pullfrog  | View workflow run | Using big-pickle (free) | 𝕏

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.

2 participants