Skip to content

[vitest-pool-workers] Add regression tests for onUnhandledError callback - #13076

Merged
penalosa merged 1 commit into
mainfrom
penalosa/vitest-11532
Mar 30, 2026
Merged

[vitest-pool-workers] Add regression tests for onUnhandledError callback#13076
penalosa merged 1 commit into
mainfrom
penalosa/vitest-11532

Conversation

@penalosa

@penalosa penalosa commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #11532.

The onUnhandledError callback was not being invoked with @cloudflare/vitest-pool-workers on versions prior to 0.13.0. This was fixed by the nodejs_process_v2 compatibility flag enforcement added in the Vitest 4 support rewrite (#11632) — workerd's native process.on('unhandledRejection') support is what Vitest's internal listenForErrors() relies on.

This PR adds regression tests to ensure the callback continues to work:

  1. Unhandled rejection fails the run by default — verifies that an unhandled Promise.reject() causes the test run to exit non-zero
  2. onUnhandledError can suppress specific errors — verifies that returning false from the callback prevents the error from failing the run

Note: errors from workers are serialised plain objects (via Vitest's serializeValue), not Error instances — this is consistent with standard Vitest pools (threads/forks). The test uses .message property access rather than instanceof Error, matching the Vitest docs example.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: regression test only, no user-facing change

Open with Devin

@penalosa penalosa added the ci:no-changeset-required Skip pull request checks for a valid changeset label Mar 26, 2026
@changeset-bot

changeset-bot Bot commented Mar 26, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 357829a

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.

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

@ask-bonk

ask-bonk Bot commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

I've now thoroughly reviewed the PR. Let me assess the changes:

  1. The test file follows established patterns - it imports test from ./helpers, uses seed() and vitestRun() fixtures, and checks exitCode and stderr - consistent with console.test.ts, filtering.test.ts, etc.

  2. The inline vitest config doesn't use the vitestConfig() helper - but this is intentional and justified. The tests need specific config shapes (one without onUnhandledError, one with it), and the vitestConfig() helper adds extra boilerplate (DeterministicSequencer, 90s timeout) that isn't needed for a single-test-file scenario. Other tests like console.test.ts also sometimes build configs inline.

  3. The test logic is sound - The first test verifies that unhandled rejections cause a non-zero exit by default. The second test verifies the onUnhandledError callback can suppress them by returning false. The await new Promise<void>((resolve) => setTimeout(resolve, 50)) correctly ensures the promise rejection becomes "unhandled" before the test finishes.

  4. The await on result.exitCode at line 29 is consistent with the helper's interface (exitCode is a Promise<number>). Line 59 also correctly awaits it.

  5. The timeout of 45_000 is reasonable for E2E tests that spawn child processes.

  6. No missing assertions - the first test checks both exit code and stderr content; the second test checks exit code is 0 (success).

  7. The err?.message optional chaining in the seeded config is appropriate since the comment correctly notes errors from workers are serialized plain objects.

This is a clean, well-structured regression test that follows the existing patterns in the test suite. No logic bugs, no security issues, no backward compatibility concerns.

LGTM

github run

@pkg-pr-new

pkg-pr-new Bot commented Mar 26, 2026

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13076

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13076

miniflare

npm i https://pkg.pr.new/miniflare@13076

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13076

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13076

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13076

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13076

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13076

wrangler

npm i https://pkg.pr.new/wrangler@13076

commit: 357829a

@penalosa
penalosa marked this pull request as ready for review March 27, 2026 00:38
@penalosa
penalosa requested a review from a team as a code owner March 27, 2026 00:38
@penalosa
penalosa requested a review from emily-shen March 27, 2026 00:38
@workers-devprod

workers-devprod commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Mar 30, 2026
@penalosa
penalosa merged commit 49d0633 into main Mar 30, 2026
54 of 63 checks passed
@penalosa
penalosa deleted the penalosa/vitest-11532 branch March 30, 2026 12:22
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:no-changeset-required Skip pull request checks for a valid changeset

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

onUnhandledError callback not invoked with @cloudflare/vitest-pool-workers

3 participants