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

Improve message/stack parsing for Babel code frame errors #7319

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

sophiebits
Copy link
Contributor

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

After:

after screenshot

@@ -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:
Copy link
Contributor Author

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.

Copy link
Member

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

@sophiebits sophiebits force-pushed the babel-err branch 2 times, most recently from d6389b3 to d1d4375 Compare November 2, 2018 06:50
@codecov-io
Copy link

Codecov Report

Merging #7319 into master will increase coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-message-util/src/index.js 82.3% <80%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 223c3db...d1d4375. Read the comment docs.

Copy link
Member

@SimenB SimenB left a 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)
@sophiebits
Copy link
Contributor Author

Changelog added!

@SimenB SimenB merged commit f2b51c1 into jestjs:master Nov 2, 2018
@SimenB
Copy link
Member

SimenB commented Nov 2, 2018

Stack trace seems kinda messed up, though, it points to line 20 in your screenshot :(

@sophiebits
Copy link
Contributor Author

That's roughly correct; it's for a syntax error where I'm missing a comma on line 19.

@SimenB
Copy link
Member

SimenB commented Nov 2, 2018

Oh, it's a syntax error! Missed that. Odd the error doesn't include that fact. Then the stack is correct, that's great

@sophiebits
Copy link
Contributor Author

Odd the error doesn't include that fact.

Now it (roughly) does! See the after screenshot.

@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 12, 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.

5 participants