-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Improve message/stack parsing for Babel code frame errors #7319
Conversation
@@ -9,9 +9,8 @@ exports[`shows the correct errors in stderr when failing tests 1`] = ` | |||
|
|||
● fail with expected non promise values | |||
|
|||
Error | |||
Expected value to have length: |
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 this is worse? I dunno.
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.
nah, this is better 🙂 more closely matches the sync matcher
d6389b3
to
d1d4375
Compare
Codecov Report
@@ Coverage Diff @@
## master #7319 +/- ##
==========================================
+ Coverage 66.63% 66.65% +0.01%
==========================================
Files 241 241
Lines 9345 9344 -1
Branches 6 5 -1
==========================================
+ Hits 6227 6228 +1
+ Misses 3115 3113 -2
Partials 3 3
Continue to review full report at Codecov.
|
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.
LGTM! Mind adding a changelog entry as well? 🙂
Previously, _addMissingMessageToStack decides not to add in the message to what we display, because it can't identify it as a stack. If we fix that by setting stack to include the message upfront (as many browsers do anyway), then it is printed but not prominently -- it is grayed out and with the stack with still only "Error" on the first line. Now if separateMessageFromStack can't tell where the stack begins, we assume the first line is the message -- which will cause it to be displayed more prominently. We could also try to detect code frames in separateMessageFromStack specifically (as we do stacks) but that doesn't seem trivial, in part because the code frame has already been colorized by the time it gets here. Before: ![before screenshot](https://user-images.githubusercontent.com/6820/47889870-6d064700-de09-11e8-8d4a-a044f2ad278b.png) After: ![after screenshot](https://user-images.githubusercontent.com/6820/47889756-cae65f00-de08-11e8-99c1-acfc6e7cb90c.png)
Changelog added! |
Stack trace seems kinda messed up, though, it points to line 20 in your screenshot :( |
That's roughly correct; it's for a syntax error where I'm missing a comma on line 19. |
Oh, it's a syntax error! Missed that. Odd the error doesn't include that fact. Then the stack is correct, that's great |
Now it (roughly) does! See the after screenshot. |
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. |
Previously, _addMissingMessageToStack decides not to add in the message to what we display, because it can't identify it as a stack. If we fix that by setting stack to include the message upfront (as many browsers do anyway), then it is printed but not prominently -- it is grayed out and with the stack with still only "Error" on the first line. Now if separateMessageFromStack can't tell where the stack begins, we assume the first line is the message -- which will cause it to be displayed more prominently.
We could also try to detect code frames in separateMessageFromStack specifically (as we do stacks) but that doesn't seem trivial, in part because the code frame has already been colorized by the time it gets here.
Before:
After: