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

fix: break dependency cycle in jest-cli #7707

Merged
merged 2 commits into from
Jan 26, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jan 25, 2019

Summary

Fixes #7704

Test plan

I tested it 😀 node -p "require('jest').run(process.argv.slice(2))" before and after this change

.eslintrc.js Outdated
@@ -5,8 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

const path = require('path');
const customImportResolver = path.resolve('./eslintImportResolver');
const customImportResolver = require.resolve('./eslintImportResolver');
Copy link
Member Author

@SimenB SimenB Jan 25, 2019

Choose a reason for hiding this comment

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

new beta version of intellij (2019.1) tries to be too clever for its own good and failed the resolution here. This fixes it, I snuck it in 😀

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @segrey (I don't have time to open up a proper bug report on youtrack, sorry!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually any path resolution that resolves modules should go through require.resolve (to include the extension and work with PnP :D)

Copy link

Choose a reason for hiding this comment

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

Sorry for being slow. Thanks, reproduced (https://youtrack.jetbrains.com/issue/WEB-37116).
/cc @undeadcat

CHANGELOG.md Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Would it be possible for us to e2e test programmatic usage?
It may not be officially supported but it's apparently a somewhat common thing to do and once we have a public API, we could just adapt the existing test.

@SimenB
Copy link
Member Author

SimenB commented Jan 25, 2019

The real programmatic api will look nothing like this, but it'd be great if you wrote a simple sanity check :)

@jeysal
Copy link
Contributor

jeysal commented Jan 25, 2019

Will do 👍
(in a few hours or tomorrow, feel free to merge this without it)

@jeysal
Copy link
Contributor

jeysal commented Jan 25, 2019

@SimenB you can pull the e2e test from `jeysal/jest:correct-jest-cli-programmatic" :)

@SimenB
Copy link
Member Author

SimenB commented Jan 26, 2019

Thanks @jeysal!

@codecov-io
Copy link

Codecov Report

Merging #7707 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7707      +/-   ##
==========================================
- Coverage   68.28%   68.27%   -0.02%     
==========================================
  Files         251      252       +1     
  Lines        9682     9682              
  Branches        5        6       +1     
==========================================
- Hits         6611     6610       -1     
- Misses       3069     3070       +1     
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-cli/src/jest.js 0% <ø> (-100%) ⬇️
packages/jest-cli/src/version.js 100% <100%> (ø)

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 a7cbeb6...a126260. Read the comment docs.

@SimenB SimenB force-pushed the correct-jest-cli-programmatic branch from a126260 to 42d3797 Compare January 26, 2019 09:06
Copy link
Contributor

@rubennorte rubennorte left a comment

Choose a reason for hiding this comment

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

Looks good!

@SimenB SimenB merged commit 88713fc into jestjs:master Jan 26, 2019
@SimenB SimenB deleted the correct-jest-cli-programmatic branch January 26, 2019 15:33
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@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.

getVersion is not defined
7 participants