Make cleanupStackTrace work on older and newer Node 8 version #4686
Conversation
| @@ -162,7 +162,7 @@ const cleanupStackTrace = (output: string) => { | |||
| .replace(/\n.*at.*assert\.js.*$/gm, '') | |||
| .replace(/\n.*at.*node\.js.*$/gm, '') | |||
| .replace(/\n.*at.*next_tick\.js.*$/gm, '') | |||
| .replace(/\n.*at (new)? Promise \(<anonymous>\).*$/gm, '') | |||
| .replace(/\n.*at (new )?Promise \(<anonymous>\).*$/gm, '') | |||
There was a problem hiding this comment.
Should we have unit tests for out test utils? 😀
There was a problem hiding this comment.
I won't be the one writing them 😄
There was a problem hiding this comment.
FWIW I think this should be stripped in here:https://github.com/facebook/jest/blob/8a427ebba88c6ab15da7c987716216728f5ea093/packages/jest-message-util/src/index.js#L150-L166
It being in the stack is not helpful at all (along with at <anonymous> and process._tickCallback (the latter should be stripped by removeInternalStackEntries already, shouldn't it?)), so why not just remove it for everyone, not just in the test utils for the integration tests?
Only the first line in the stack is useful. (Ignore the "expected" part, it's from another matcher)
There was a problem hiding this comment.
I'm a fan of removing at process._tickCallback and at <anonymous> without any further info (like line number) from the stack trace 👍.
There was a problem hiding this comment.
If we do that, I think you can remove the line with a diff in this PR entirely
There was a problem hiding this comment.
I'm happy to modify this PR or send a followup when we have the discussed behavior merged.
There was a problem hiding this comment.
Yeah, this should probably be merged in the meantime
Codecov Report
@@ Coverage Diff @@
## master #4686 +/- ##
=======================================
Coverage 57.13% 57.13%
=======================================
Files 195 195
Lines 6569 6569
Branches 3 3
=======================================
Hits 3753 3753
Misses 2816 2816Continue to review full report at Codecov.
|
|
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. |
Summary
As @SimenB pointed out in #4680 (comment) the regex should be updated, so it works on older and newer versions of Node 8
Test plan
Make sure all CI pass.