Skip to content

fix: break dependency cycle in jest-cli#7707

Merged
SimenB merged 2 commits into
jestjs:masterfrom
SimenB:correct-jest-cli-programmatic
Jan 26, 2019
Merged

fix: break dependency cycle in jest-cli#7707
SimenB merged 2 commits into
jestjs:masterfrom
SimenB:correct-jest-cli-programmatic

Conversation

@SimenB
Copy link
Copy Markdown
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

Comment thread .eslintrc.js

const path = require('path');
const customImportResolver = path.resolve('./eslintImportResolver');
const customImportResolver = require.resolve('./eslintImportResolver');
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

Comment thread CHANGELOG.md Outdated
Comment thread yarn.lock
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
Collaborator

jeysal commented Jan 25, 2019

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

@jeysal
Copy link
Copy Markdown
Collaborator

jeysal commented Jan 25, 2019

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

@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented Jan 26, 2019

Thanks @jeysal!

@codecov-io
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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