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: warn when there are multiple configs #11922

Merged
merged 41 commits into from Oct 6, 2021

Conversation

@JanKaifer
Copy link
Contributor

@JanKaifer JanKaifer commented Oct 2, 2021

  • - CHANGELOG.md is not updated yet, since this feature is not approved/finished and contains breaking changes.

Summary

resolveConfigPath throws when there are both package.json(with "jest") and jest.config.ext present in a single directory.

Fix #10124

There are 2 stale PRs that I took inspiration from: #10798 and #10213

It is a breaking change (error, not a warning) as proposed by @SimenB here: #10798 (review)

Closes #10798
Closes #10213

Test plan

I wrote/updated unit tests that check the behavior.

Questions

  • Just wanted to make sure that I understood correctly that package.json needs to have "jest" key in order to be a valid jest config file.
  • I'm reading and parsing the whole package.json don't know if that could be a performance problem.
  • The "jest" is hardcoded in two places now:
    configObject = configObject.jest || {};
    . I'm not sure where to move it.
@JanKaifer JanKaifer changed the title Throw when there are multiple configs. package.json and jest.config.js Throw when there are multiple configs - package.json and jest.config.js Oct 2, 2021
@mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Oct 2, 2021

Just thinking out loud. What if Jest would simply console.info a line telling which config it is using? Only if the config was auto detected or always?

This wouldn’t be a breaking change in this case.

(Would be good to check how this shall work with projects.)

Loading

@JanKaifer
Copy link
Contributor Author

@JanKaifer JanKaifer commented Oct 2, 2021

I just followed this comment by maintainers: #10798 (review) . I don't have strong feelings about this.

Loading

Copy link
Collaborator

@SimenB SimenB left a comment

lgtm.

One thing is that if people define a --config flag this should not throw. We should only error if Jest needs to traverse and find configuration on its own and there is ambiguity on which to use.

I'd also like to see some integration tests for this (i.e. a dir with both a config file and config in package.json, and run it once to see the error, then with --config jets.config.js which should not throw)

Loading

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 4, 2021

As for breaking or not - I guess I could be down for a config warning similar to deprecation warnings like using --browser

image

Not just a single line, but a more in your face warning that you really wanna fix

Loading

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@JanKaifer
Copy link
Contributor Author

@JanKaifer JanKaifer commented Oct 4, 2021

One thing is that if people define a --config flag this should not throw. We should only error if Jest needs to traverse and find configuration on its own and there is ambiguity on which to use.

I'd also like to see some integration tests for this (i.e. a dir with both a config file and config in package.json, and run it once to see the error, then with --config jets.config.js which should not throw)

Sorry, missed that. On it.

As for breaking or not - I guess I could be down for a config warning similar to deprecation warnings like using --browser

I will leave that decision to you. I like error a bit more, but I don't have the knowledge about jest to make a good decision here.

Loading

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
@JanKaifer
Copy link
Contributor Author

@JanKaifer JanKaifer commented Oct 4, 2021

I'd also like to see some integration tests for this (i.e. a dir with both a config file and config in package.json, and run it once to see the error, then with --config jets.config.js which should not throw)

Added e2e test for that (surprisingly there wasn't one - at least I didn't find any).

Loading

@JanKaifer JanKaifer requested a review from SimenB Oct 4, 2021
Copy link
Collaborator

@SimenB SimenB left a comment

thanks, this looks great! I just have a couple of nits for the tests, beyond that LGTM 👍

Loading

e2e/__tests__/dependencyClash.test.ts Show resolved Hide resolved
Loading
e2e/esm-config/cjs/package.json Show resolved Hide resolved
Loading
e2e/__tests__/configOverride.test.ts Show resolved Hide resolved
Loading
packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
Copy link
Collaborator

@SimenB SimenB left a comment

Ok, I think we're almost done now 😀

(remember the changelog entry)

Loading

e2e/__tests__/multipleConfigs.ts Outdated Show resolved Hide resolved
Loading
e2e/__tests__/multipleConfigs.ts Outdated Show resolved Hide resolved
Loading
e2e/esm-config/mjs/package.json Outdated Show resolved Hide resolved
Loading
}

if (configFiles.length > 1) {
throw new Error(makeMultipleConfigsError(configFiles));
Copy link
Collaborator

@SimenB SimenB Oct 5, 2021

Choose a reason for hiding this comment

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

if we throw it's a breaking change, and we need to wait for Jest 28. Thoughts on making it a warning for now so we can land it, then throw later?

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 5, 2021

Choose a reason for hiding this comment

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

Sounds reasonable.

Should I hide it behind a flag? Or just delete it and rewrite it for warnings?
This question is related to tests, the code itself is just one line, but tests are completely different.

Loading

Copy link
Collaborator

@SimenB SimenB Oct 5, 2021

Choose a reason for hiding this comment

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

I'd do console.warn instead of throwing, make the unit tests mock out the console, while the e2e test is essentially the same except for the exit code

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 5, 2021

Choose a reason for hiding this comment

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

resolveConfigPath has to be a pure function (no console.warn there), because it is run multiple times.

There is hasDeprecationWarnings: boolean flag somewhere. I will look into it later.

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 5, 2021

Choose a reason for hiding this comment

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

hasDeprecationWarnings has no text associated with it.

The best solution I found was to propagate shouldLogWarnings. But I'm not sure it is good enough.

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 5, 2021

Choose a reason for hiding this comment

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

The "second" usage is here:

const parsedConfig = await readConfig(argv, projects[0]);

It's reading jest config to determine if there are any additional projects defined.

And then it is called again for each project.

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 6, 2021

Choose a reason for hiding this comment

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

@SimenB ^ this one is still not solved.

Loading

Copy link
Collaborator

@SimenB SimenB Oct 6, 2021

Choose a reason for hiding this comment

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

gave that a whack with 6a52634 (#11922)

Loading

Copy link
Contributor Author

@JanKaifer JanKaifer Oct 6, 2021

Choose a reason for hiding this comment

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

Ok, so you went with shouldLogWarnings. 👍

Loading

Copy link
Collaborator

@SimenB SimenB Oct 6, 2021

Choose a reason for hiding this comment

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

yeah, it was either that or store some list of the exact warning we had logged (a Set to dedupe) then only log after all configs were resolved. This seemed cleaner

Loading

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
e2e/__tests__/multipleConfigs.ts Outdated Show resolved Hide resolved
Loading
JanKaifer and others added 2 commits Oct 5, 2021
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
SimenB
SimenB approved these changes Oct 6, 2021
Copy link
Collaborator

@SimenB SimenB left a comment

thanks! is a fantastic first contribution 🎉

Loading

Copy link
Collaborator

@SimenB SimenB left a comment

got a bit too eager, didn't notice the failing tests 😅

Loading

Loading
e2e/__tests__/multipleConfigs.ts Outdated Show resolved Hide resolved
Loading
packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
@JanKaifer
Copy link
Contributor Author

@JanKaifer JanKaifer commented Oct 6, 2021

Sorry, was busy with school. Thanks for finishing it @SimenB.

And thanks for guiding me through this PR.

Loading

SimenB
SimenB approved these changes Oct 6, 2021
Copy link
Collaborator

@SimenB SimenB left a comment

Now it seems CI is happy. Thanks again!

Loading

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved
Loading
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #11922 (b57cbf5) into main (24f9bc8) will increase coverage by 0.03%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11922      +/-   ##
==========================================
+ Coverage   68.75%   68.78%   +0.03%     
==========================================
  Files         322      322              
  Lines       16587    16608      +21     
  Branches     4787     4793       +6     
==========================================
+ Hits        11404    11424      +20     
- Misses       5151     5152       +1     
  Partials       32       32              
Impacted Files Coverage Δ
packages/jest-config/src/index.ts 20.00% <33.33%> (+1.01%) ⬆️
packages/jest-config/src/resolveConfigPath.ts 97.95% <100.00%> (+1.40%) ⬆️
packages/expect/src/utils.ts 95.85% <0.00%> (-0.52%) ⬇️

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 24f9bc8...b57cbf5. Read the comment docs.

Loading

@SimenB SimenB changed the title Throw when there are multiple configs - package.json and jest.config.js feat: warn when there are multiple configs Oct 6, 2021
@SimenB SimenB merged commit cdc64c6 into facebook:main Oct 6, 2021
29 of 30 checks passed
Loading
@JanKaifer JanKaifer deleted the jankaifer/warn-multiple-configs-found branch Oct 6, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Nov 6, 2021

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.

Loading

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants