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

feat(jest-message-util): render codeframe on failure #5087

Merged
merged 1 commit into from
Dec 15, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 15, 2017

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.

image

Test plan
Eventually green CI.

@SimenB SimenB force-pushed the render-stackframe branch 2 times, most recently from 10e9de9 to b9dee9d Compare December 15, 2017 10:40
@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

Can we get rid of one newline before and after the codeframe?


if (parsedFrame) {
renderedCallsite = codeFrameColumns(
fs.readFileSync(testPath, 'utf8'),
Copy link
Member Author

@SimenB SimenB Dec 15, 2017

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

Copy link
Member

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",
Copy link
Member

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?

Copy link
Member Author

@SimenB SimenB Dec 15, 2017

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

Copy link
Member

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`;
Copy link
Member

@cpojer cpojer Dec 15, 2017

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

image

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

What would this look like if we replaced the expect(received).toEqual(expected) with the line of code from code frame? I'm assuming what you have here is much better, especially for multiline expects, but I'd be curious if show a single line there could work.

@SimenB
Copy link
Member Author

SimenB commented Dec 15, 2017

Ohhh, that's an interesting idea! I'll see what I can do (later, out of "free time" this morning :P)

@codecov-io
Copy link

Codecov Report

Merging #5087 into master will decrease coverage by 0.03%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/jest-message-util/src/index.js 81.48% <36.36%> (-5.26%) ⬇️
packages/jest-cli/src/constants.js 100% <0%> (ø) ⬆️
packages/jest-config/src/index.js 23.8% <0%> (ø) ⬆️
packages/jest-cli/src/get_no_test_found_failed.js
packages/jest-cli/src/failed_tests_cache.js
packages/jest-jasmine2/src/index.js 5.97% <0%> (+0.33%) ⬆️
packages/jest-cli/src/run_jest.js 53.57% <0%> (+1.02%) ⬆️
packages/jest-cli/src/watch.js 76.11% <0%> (+1.29%) ⬆️
packages/jest-cli/src/lib/update_global_config.js 90.9% <0%> (+3.4%) ⬆️
packages/jest-cli/src/get_no_test_found_message.js 66.66% <0%> (+6.66%) ⬆️

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 6f7c85a...5d6c829. Read the comment docs.

@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