alternative stack trace approach #2119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidjgoss
commented
Aug 23, 2022
| } | ||
| return format(error, { | ||
| colorFns: { | ||
| errorStack: (stack: string) => |
There was a problem hiding this comment.
I'm kind of abusing the assertion-error-formatter API here, which doesn't feel great. It could maybe do with a formatErrorStack option where you can provide a function that rewrites the stack as desired?
davidjgoss
changed the title
davidjgoss
commented
Sep 19, 2022
| @@ -309,7 +308,7 @@ | |||
| "prepublishOnly": "rm -rf lib && npm run build-local", | |||
| "pretest-coverage": "npm run build-local", | |||
| "pretypes-test": "npm run build-local", | |||
| "test-coverage": "nyc --silent mocha 'src/**/*_spec.ts' 'compatibility/**/*_spec.ts' && nyc --silent --no-clean node bin/cucumber.js && nyc report --reporter=lcov", | |||
| "test-coverage": "nyc --silent mocha 'src/**/*_spec.ts' 'compatibility/**/*_spec.ts' && nyc --silent --no-clean node bin/cucumber.js --tags \"not @source-mapping\" && nyc report --reporter=lcov", | |||
There was a problem hiding this comment.
The nyc instrumentation seems to mess these scenarios up when we check for coverage. We could try and fix up the environment for the child process but it didn't feel worth it.
|
This was released in 8.8.0 https://github.com/cucumber/cucumber-js/releases/tag/v8.8.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🤔 What's changed?
Stop using
stack-chainto temporarily modify stack trace behaviour in a V8-specific manner, for when we filter stack traces (i.e. with--backtraceoption). Pivot to parsing the stack witherror-stack-parserwithout modifying how it's created, and filtering from that.Also add some documentation around source maps.
⚡️ What's your motivation?
This started with me digging around to try and understand this aspect of our codebase better, but I wanted to run with this approach and see if it's viable.
Hooking into V8's stack trace API in the way that
stack-chaindoes in order to filter the stack trace for display seems like a bit of a sledgehammer to me. It also doesn't fail gracefully - in a non-V8 environment it will throw and exit, and this happens as soon as you importstack-chain, not just when you call methods on it. This is the only reason we can't do a live demo with StackBlitz, for example, and also seems likely to be a hindrance as we look to become less Node.js-centric and work with e.g. Deno and Bun.On the flipside, parsing the stack trace is inevitably less scientific and more error-prone, and there will be some edge cases that go weird. For example stacktracejs/error-stack-parser#62 which contributed to us to moving away from the stacktrace.js libraries before (I've raised a fix for this now). I think it would be worthwhile though.
This change has also fixed an issue I was seeing when creating an example TypeScript+ESM project, where source references and stack traces were coming out with wrong locations that didn't correspond to either the source or transpiled JavaScript.
This area isn't very well covered by tests, so I'm going to try and flesh some out before I mark this as ready.
🏷️ What kind of change is this?
📋 Checklist:
This text was originally generated from a template, then edited by hand. You can modify the template here.