Skip to content
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

Always remove node internals from stacktraces #4695

Merged
merged 7 commits into from
Oct 15, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 14, 2017

Summary
As discussed in #4686, this removes node core stuff from the stacktraces we present to the user, not just from our own tests.

Stack trace with this PR:
image

Without this PR:
image

Test plan
New test added

at promise.then (node_modules/jest-jasmine2/build/queue_runner.js:74:39)
at <anonymous>
at process._tickCallback (internal/process/next_tick.js:188:7)
at internal/process/next_tick.js:188:7
Copy link
Member Author

Choose a reason for hiding this comment

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

this last one I just saw in the integration tests. It should be the same as the one above it. Ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to come from the remaining replace in cleanupStackTrace in integration_tests/utils.js. Not sure how to handle it

Copy link
Member Author

Choose a reason for hiding this comment

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

If we really want to do it, I can move this next_tick part down there, no reason to have it in message-util

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured it out, this is node 4 missing the calling function's name.

@SimenB
Copy link
Member Author

SimenB commented Oct 14, 2017

Heh, breakes the browser test. Fair enough

return false;
}

if (STACK_TRACE_IGNORE.test(line)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about renaming this to JEST_INTERNALS_IGNORE?

@@ -27,6 +37,9 @@ type StackTraceOptions = {
// filter for noisy stack trace lines
const JASMINE_IGNORE = /^\s+at(?:(?:.*?vendor\/|jasmine\-)|\s+jasmine\.buildExpectationResult)/;
const STACK_TRACE_IGNORE = /^\s+at.*?jest(-.*?)?(\/|\\)(build|node_modules|packages)(\/|\\)/;
const ANONYMOUS_TRACE_IGNORE = /^\s+at <anonymous>.*$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ANONYMOUS_FN_IGNORE?

.replace(/\n.*at.*next_tick\.js.*$/gm, '')
.replace(/\n.*at (new )?Promise \(<anonymous>\).*$/gm, '')
.replace(/\n.*at <anonymous>.*$/gm, '')
.replace(/\n.*at Generator.next \(<anonymous>\).*$/gm, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -158,19 +158,12 @@ const extractSummary = (stdout: string) => {
// unifies their output to make it possible to snapshot them.
const cleanupStackTrace = (output: string) => {
return output
.replace(/\n.*at.*timers\.js.*$/gm, '')
.replace(/\n.*at.*assert\.js.*$/gm, '')
.replace(/\n.*at.*node\.js.*$/gm, '')
.replace(/\n.*at.*next_tick\.js.*$/gm, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this needs to be updated on Node 4? Not sure why it fails now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

next_tick is not under internal/ on node 4...

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not the integration test which fails, it's the new test I added in message-utils

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants