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

use default cwd even if config contains a cwd property #7923

Merged
merged 1 commit into from Mar 26, 2019

Conversation

@jeysal
Copy link
Collaborator

jeysal commented Feb 17, 2019

Summary

Fixes #7857, check that issue for details

Test plan

// cwd is an exception, it must not appear in InitialOptions
const {cwd: _cwd, ...defaults} = Defaults;

normalize({...defaults, rootDir: '/root'}, {});

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 17, 2019

Collaborator

I don't follow - shouldn't options just override default options?

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 17, 2019

Author Collaborator

As I understand it, cwd is not a supported option for users to specify in the config. I mentioned in the issue that it does not appear in the docs or in InitialOptions

This comment has been minimized.

Copy link
@SimenB

SimenB Feb 18, 2019

Collaborator

@rubennorte you added it in #7146 - thoughts?

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 18, 2019

Author Collaborator

(I also think it's right that it is not something that can be explicitly set, that should only be the case for rootDir)

This comment has been minimized.

Copy link
@jeysal

jeysal Feb 18, 2019

Author Collaborator

Either way, cwd becoming undefined if it is explicitly set is definitely not expected behavior 😅

@SimenB SimenB requested review from thymikee and rubennorte Feb 18, 2019
@jeysal jeysal force-pushed the jeysal:config-cwd-undefined branch from ef17b38 to 2da5f57 Feb 18, 2019
@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Feb 18, 2019

Updated to do this in a much cleaner way, don't know what I was thinking yesterday ^^

@jeysal jeysal force-pushed the jeysal:config-cwd-undefined branch from 2da5f57 to 529d73d Feb 18, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #7923 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7923   +/-   ##
=======================================
  Coverage   62.31%   62.31%           
=======================================
  Files         265      265           
  Lines       10534    10534           
  Branches     2557     2556    -1     
=======================================
  Hits         6564     6564           
  Misses       3387     3387           
  Partials      583      583
Impacted Files Coverage Δ
packages/jest-config/src/Defaults.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/normalize.ts 79.52% <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 54ce3f3...cff4b4a. Read the comment docs.

@Aghassi

This comment has been minimized.

Copy link
Contributor

Aghassi commented Feb 24, 2019

@jeysal @SimenB Any movement on this? Anything I can help with so this can be merged? Would love to keep on top of the most current version of Jest and not be stuck because of #7857

@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Feb 24, 2019

@Aghassi I think we're just waiting for @rubennorte who originally added cwd in some places to comment what on what the correct behavior would be. I think this is the correct solution because cwd is not and should not be a supported config option and should thus be ignored instead of breaking. The alternative would be to allow cwd to be overwritten in user config, which would also resolve the issue.
I think you could upgrade regardless of this issue though, if you just remove cwd from your config it should work?

@Aghassi

This comment has been minimized.

Copy link
Contributor

Aghassi commented Feb 25, 2019

@jeysal I'm actually never setting it explicitly. Seems like if I do delete config.cwd it works though. I will do that for now and note these issues.

EDIT: I lied it doesn't. Still digging in. Doesn't matter if I remove cwd. The default config has it, and also Jest still tries to validate it (which breaks if it is undefined). So... I can't upgrade unless I try overwriting the console object, which I don't feel is something I wish to spend time on right now. We will downgrade until there is a clear upgrade path.

@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Feb 25, 2019

@Aghassi the reproduction repo you provided (https://github.com/Aghassi/oclif-jest-demo-failure/blob/master/jest.config.js) does set it though? And when I checked out that repo a week ago, the error went away after removing cwd

@Aghassi

This comment has been minimized.

Copy link
Contributor

Aghassi commented Feb 26, 2019

@jeysal Sorry, I was trying it on an internal repo and as far as I can tell we never set it. I was able to validate and realized I had deleted cwd from the wrong config. But even when I deleted the cwd from the config, it only seems to work when the repo is linked (we do something similar to fbjs internally where we store configs and then leverage them in multiple repos. So I'm definitely lost 😅

@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Feb 26, 2019

Okay. I hope your internal repo doesn't surface a whole different issue than the reproduction then 😬

@Aghassi

This comment has been minimized.

Copy link
Contributor

Aghassi commented Mar 1, 2019

@jeysal For now, we are downgrading until the typescript changes are ironed out. Since we support larger systems, and I can't seem to figure out why beyond deleting cwd doesn't fix the problem, I want to wait. It's possible this change will resolve the problem and I'm just missing something on my end. However, since both seem to still run most tests fine, our developers won't be missing much. I have Renovate running internally so I will get a ping as soon as the next version gets published. I'll be testing each version to ensure I can upgrade without fail 😄 . Trust me, I don't want to be stuck. Just don't have many cycles to spend debugging this at the moment.

@Aghassi

This comment has been minimized.

Copy link
Contributor

Aghassi commented Mar 25, 2019

@rubennorte any chance you can chime in here? Ideally I’d like to be able to update to the latest jest, but am still blocked in part due to this.

@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Mar 25, 2019

@SimenB do we absolutely need Ruben's input on this?
The purpose of #7146 was to move cwd assignment from jest-cli to jest-config. This is preserved here.
The second sentence over there states that cwd is added to the defaults so it can be imported from there in JS config. This is lost here, but I don't see why that's a problem - you can't set cwd in your config anyway, it doesn't work (and IMO shouldn't).
This PR just fixes the behavior when you try to set it - instead of cwd becoming undefined, it is just ignored like setting asdf in the config. I think this PR is just the right way of moving the assignment from jest-cli to jest-config like #7146 did.

Copy link
Contributor

rubennorte left a comment

@jeysal and @Aghassi sorry for the late reply, I've been a bit busy and some things starts to stack up.

This looks good. As @jeysal said, my main reason to add this change was to have it in jest-config and I see that allowing it in userland was a bad side-effect I wasn't counting on.

Thanks for the fix!

@jeysal

This comment has been minimized.

Copy link
Collaborator Author

jeysal commented Mar 26, 2019

Rebased (manually)

@jeysal jeysal force-pushed the jeysal:config-cwd-undefined branch from 529d73d to fdb20cd Mar 26, 2019
@jeysal jeysal force-pushed the jeysal:config-cwd-undefined branch from fdb20cd to cff4b4a Mar 26, 2019
@jeysal jeysal merged commit 831085c into facebook:master Mar 26, 2019
11 checks passed
11 checks passed
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190326.10 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.