-
-
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
Use process.cwd as config.rootDir fallback #4587
Conversation
This is failing CI. Would it be possible to add an integration test as well that fails without your patch? |
0f82f92
to
8bc7e2c
Compare
Thank you for your patience. I've added failing test 8bc7e2c. CI results are failing for this commit:
Jest shows this error (not visible in CI):
After applying a fix (05484d7) the error is fixed, but another one was introduced:
P. S. Something strange with Jest's tests on local machine (macOS 10.12.6, Node 8.9.0, Yarn 1.3.2). I was doing everything according to Contributions instructions, but |
Can we take this over the finish line? |
05484d7
to
ab0f8a3
Compare
I would like too help. But I can't figure out how. I hope my research could help make proper fix. I rebased PR on latest master and CI tests failing with totally unrelated error :( |
Does |
Same error. I did
|
// If rootDir is not there, we'll set it to this file's __dirname | ||
configObject.rootDir = path.dirname(configPath); | ||
// If rootDir is not there, we'll set it to cwd | ||
configObject.rootDir = process.cwd(); |
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.
I don't think this is the correct change - from what I can gather it looks at the filename now to support projects
.
I'm not really sure what the correct behavior should be. I'm leading towards using cwd
and requiring rootDir
in the config of nested projects
.
@hudochenkov what do you think about @SimenB's last comment? Should we close this? |
@rickhanlonii unfortunately, I'm not familiar with the concept of projects in Jest, so I don't understand the meaning of that comment. Also, I won't be able to fix the problem without creating other problems. So this PR is more like research for someone who can actually fix #3613. Because currently, Jest doesn't behave like it's stated in the documentation for Feel free to close this PR, because I have doubts I contribute something helpful to Jest :( |
@hudochenkov ok, I think the move here would be to update the docs instead - if you want to submit that PR I'm happy to review and merge! |
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
Fixes #3499 and #3613.
I compared v19.0.2 and latest
master
.In 19.0.2
rootDir
isprocess.cwd()
ifrootDir
isn't specified in a config: https://github.com/facebook/jest/blob/ed45267e9d8b8cabc83f69a538e03465d7376868/packages/jest-config/src/loadFromFile.js#L36-L38In latest master
rootDir
ispath.dirname(configPath)
: https://github.com/facebook/jest/blob/7aa3017f97a4dfa76f566f9bbeded5d03fdd35e7/packages/jest-config/src/read_config_file_and_set_root_dir.js#L54-L55Apparently this change in behavior was made in #4122 and probably in #3472.
Test plan
I used https://github.com/hudochenkov/jest-rootdir-error provided #3499 for testing on a “real” project.
There is one failing test and I have no idea in what direction to go to fix it.
In this test
configObject.rootDir
is different.With
path.dirname(configPath)
it's:While with
process.cwd()
it's:Current documentation says:
Which is true for a failing test case (in conditions when it's not failing), but it's not true for situations reported in issues mentioned above. For last situations
rootDir
is config dir.I'm sorry this PR isn't complete. I hope my investigation will be a good headstart for fixing those issues.