-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use stack-utils for nicer stack traces. #418
Conversation
Love love love this! |
@@ -118,6 +118,7 @@ | |||
"serialize-error": "^1.1.0", | |||
"set-immediate-shim": "^1.0.1", | |||
"source-map-support": "^0.4.0", | |||
"stack-utils": "github:tapjs/stack-utils#long-stack-traces", |
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.
No need for github:
prefix, it will work the same without it.
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.
I just did
npm i -S tapjs/stack-utils#long-stack-traces
and that is how it modified package.json
. I want to get that PR merged before we merge this anyways.
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.
npm apparently adds this now, but it's not needed as github is the default for shorthands.
😄 If you could review the issues and PR's on |
|
||
var Promise = require('bluebird'); | ||
|
||
test(() => { |
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.
Oops. This fixture isn't used anywhere. I just used it to create some of the above screenshots. It should be deleted.
I'm also wondering if we should do something like this: var opts = {};
// If debug is enabled, don't delete stack trace lines.
if (!debug('ava').enabled) {
opt.internals = StackUtils.internals().concat([
// ...
]);
} |
Probably adding those when debugging is enabled would be useful: + /[\\\/]ava[\\\/](?:lib[\\\/])?[\w-]+\.js:[0-9]+:[0-9]+\)?$/,
+ /[\\\/]node_modules[\\\/](?:bluebird|empower-core)[\\\/]/ |
If debugging is enabled, why not just print everything? |
Or that :) |
😁 Looks kinda weird to have it double here:
Maybe we can remove the AssertionError? |
found = found || relevant; | ||
return !found || relevant; | ||
function beautifyStack(stack) { | ||
return stack.split('\n')[0] + '\n' + stackUtils.clean(stack).split('\n').map(function (s) { |
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.
Maybe use https://github.com/sindresorhus/indent-string here.
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.
Why is the first line not indented? And why do we need to manually indent it? Shouldn't stack-utils
do that for us?
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.
See tapjs/stack-utils#2 (comment), if I'm not mistaken
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.
Yep. Whitespace at the beginning of any multi-line string makes the yaml output much harder to read. stack-utils
trims whitespace from the first line.
I guess we could add an indent
option to stack-utils
.
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.
I guess we could add an indent option to stack-utils.
👍 Can you open an issue on stack-utils
so we don't forget to simplify this?
Sure. |
/\(internal\/[\w_-]+\.js:[0-9]+:[0-9]+\)$/, | ||
/\(native\)$/, | ||
/[\\\/]ava[\\\/](?:lib[\\\/])?[\w-]+\.js:[0-9]+:[0-9]+\)?$/, | ||
/[\\\/]node_modules[\\\/](?:bluebird|empower-core)[\\\/]/ |
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.
Can you normalize the paths in stack traces to forward slash? That way they're consistent on all systems and with the added benefit of simpler regex here.
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.
Actually, stack-utils
already does that. I'm not sure if it does it before the regexp gets applied, but it probably should.
👍 Can you open a new issue about that? It's unrelated to this PR, just came to my mind when looking at your screenshots. |
👍 |
ebef6e4
to
0f16082
Compare
0f16082
to
af37305
Compare
Use stack-utils for nicer stack traces.
👍 |
@vdemedes Go ahead ;) |
This is super nice. |
I have extracted
node-tap
s stack cleaning algorithm into it's own module,tapjs/stack-utils
.It creates much nicer stack traces than
beautify-stack.js
does. There is a bit of work left to do in thestack-utils
module, but once these are done, this should be mergeable:internal
Screenshots below.