-
Notifications
You must be signed in to change notification settings - Fork 158
Fix step stack trace propogation and refactor e2e error tests #720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 14b0723 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (16 failed)mongodb (1 failed):
starter (14 failed):
turso (1 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
|
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors error handling end-to-end tests by breaking up consolidated tests into 9 individual, isolated tests organized into semantic groups. The restructuring improves test debuggability and maintainability through clearer organization using nested describe blocks and consistent naming conventions.
- Breaks up 3 consolidated error workflow tests into 9 focused individual tests
- Organizes tests into 3 semantic groups: error propagation (workflow/step errors), retry behavior (Error/FatalError/RetryableError/maxRetries), and catchability (FatalError.is())
- Adds helper functions in helpers.ts to support cross-file error testing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| workbench/example/workflows/helpers.ts | Adds new helper functions and documentation for testing cross-file error propagation in both workflow and step contexts |
| workbench/example/workflows/99_e2e.ts | Removes old consolidated error test workflows and adds 9 new focused error test workflows with consistent naming and clear documentation |
| packages/core/e2e/e2e.test.ts | Restructures error handling tests into nested describe blocks with individual test cases, replacing 3 consolidated tests with 9 specific tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Step error workflows now catch the error and return message/stack, making assertions cleaner. Tests verify both: - Workflow return value (caught error message) - CLI step result (original stack with function names) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
| correlationId: stepId, | ||
| eventData: { | ||
| error: errorMessage, | ||
| stack: step.error?.stack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty much a noop here - it's just relevant later for event sourcing
| // Steps execute in Node.js context and inline sourcemaps ensure we get | ||
| // meaningful stack traces with proper file names and line numbers when errors | ||
| // occur in deeply nested function calls across multiple files. | ||
| sourcemap: 'inline', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only change that gives me a tiny bit of pause - does this affect performance? It'd be nice to take a quick look at bundle times and see if it's relevant, given we already have complaints about bundling/discovery time, and the caching PR from JJ was reverted recently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the perf issues were primarily happening in the "discovering workflows/steps" bit - which is not the same as this esbuild.
This shouldn't meaningfully affect benchmarking yet, but we probably should start benchmarking build times soon too @ijjk as you dig deeper into that
Summary
FatalErrorutils.tsfile includes specific checks to change the test behavior based on the context. It's pretty ugly but will attempt to fix this in future PRs to be consistent. Atleast this PR is an improvement over no stack trace propagation at allCloses #310