-
-
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
feat(jest-message-util): render codeframe on failure #5087
Conversation
10e9de9
to
b9dee9d
Compare
Can we get rid of one newline before and after the codeframe? |
|
||
if (parsedFrame) { | ||
renderedCallsite = codeFrameColumns( | ||
fs.readFileSync(testPath, 'utf8'), |
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.
main thing is that I really don't wanna do this - we have the test file content in a cache somewhere in memory
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.
You are right but unless we share the global state somehow we can't avoid it. We have the file in jest-runner
and we could possibly put it somewhere as state. Let me think about this for a bit.
@@ -8,6 +8,7 @@ | |||
"license": "MIT", | |||
"main": "build/index.js", | |||
"dependencies": { | |||
"@babel/code-frame": "^7.0.0-beta.35", |
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.
How much stuff does this pull into Jest?
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.
Check the lockfile.
"@babel/code-frame@^7.0.0-beta.35":
version "7.0.0-beta.35"
resolved "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0-beta.35.tgz#04eeb6dca7efef8f65776a4c214157303b85ad51"
dependencies:
chalk "^2.0.0"
esutils "^2.0.2"
js-tokens "^3.0.0"
We can go with the babel 6 version which we already have transitively in the dep tree as it's used by babel itself when transpiling, and just use this one whenever we bump to @babel/core@7
internally
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.
Oh yeah, good point. This is fine then.
.map(line => MESSAGE_INDENT + line) | ||
.join('\n'); | ||
|
||
renderedCallsite = `\n\n${renderedCallsite}\n\n`; |
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.
rm one newline before and after plsss
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.
What would this look like if we replaced the |
Ohhh, that's an interesting idea! I'll see what I can do (later, out of "free time" this morning :P) |
576a95a
to
5d6c829
Compare
Codecov Report
@@ Coverage Diff @@
## master #5087 +/- ##
=========================================
- Coverage 60.63% 60.6% -0.04%
=========================================
Files 201 199 -2
Lines 6676 6653 -23
Branches 4 4
=========================================
- Hits 4048 4032 -16
+ Misses 2628 2621 -7
Continue 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
I had a bit of time this morning, so tried to throw something together for #3415. This is not ready to merge, but pushing to get some early feedback.
Test plan
Eventually green CI.