feat: warn when there are multiple configs #11922

jankaifer · 2021-10-02T12:43:03Z

- 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: https://github.com/facebook/jest/blob/7109b8ccea2e0f70b6ad8eae02cd36b6c2841604/packages/jest-config/src/readConfigFileAndSetRootDir.ts#L58 . I'm not sure where to move it.

mrazauskas · 2021-10-02T13:26:26Z

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.)

jankaifer · 2021-10-02T13:41:28Z

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

SimenB

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)

packages/jest-config/src/resolveConfigPath.ts

SimenB · 2021-10-04T07:57:40Z

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

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

Co-authored-by: Simen Bekkhus <[email protected]>

jankaifer · 2021-10-04T08:42:41Z

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.

packages/jest-config/src/resolveConfigPath.ts

jankaifer · 2021-10-04T14:28:05Z

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).

packages/jest-config/src/resolveConfigPath.ts

SimenB

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

e2e/__tests__/dependencyClash.test.ts

e2e/esm-config/cjs/package.json

e2e/__tests__/configOverride.test.ts

packages/jest-config/src/resolveConfigPath.ts

Co-authored-by: Simen Bekkhus <[email protected]>

…nKaifer/jest into jankaifer/warn-multiple-configs-found

packages/jest-config/src/__tests__/__snapshots__/resolveConfigPath.test.ts.snap

SimenB

Ok, I think we're almost done now 😀

(remember the changelog entry)

e2e/__tests__/multipleConfigs.ts

e2e/esm-config/mjs/package.json

SimenB · 2021-10-05T08:42:43Z

packages/jest-config/src/resolveConfigPath.ts

+  }
+
+  if (configFiles.length > 1) {
+    throw new Error(makeMultipleConfigsError(configFiles));


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?

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.

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

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.

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.

The "second" usage is here: https://github.com/facebook/jest/blob/a22ed6592f772df6b19314d9f06a7285c3b0968b/packages/jest-config/src/index.ts#L287

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

And then it is called again for each project.

@SimenB ^ this one is still not solved.

gave that a whack with 6a52634 (#11922)

Ok, so you went with shouldLogWarnings. 👍

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

packages/jest-config/src/resolveConfigPath.ts

e2e/__tests__/multipleConfigs.ts

Co-authored-by: Simen Bekkhus <[email protected]>

packages/jest-config/src/__tests__/resolveConfigPath.test.ts

SimenB

thanks! is a fantastic first contribution 🎉

SimenB

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

packages/jest-config/src/__tests__/resolveConfigPath.test.ts

e2e/__tests__/multipleConfigs.ts

packages/jest-config/src/resolveConfigPath.ts

Co-authored-by: Simen Bekkhus <[email protected]>

jankaifer · 2021-10-06T08:45:26Z

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

And thanks for guiding me through this PR.

SimenB

Now it seems CI is happy. Thanks again!

packages/jest-config/src/resolveConfigPath.ts

codecov-commenter · 2021-10-06T10:22:40Z

Codecov Report

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

@@            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.

github-actions · 2021-11-06T00:04:29Z

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.

jankaifer added 2 commits October 2, 2021 14:37

chore: throw when there are multiple configs

b85b26b

fix: allowed package.json without 'jest' as valid config

4dff740

facebook-github-bot added the cla signed label Oct 2, 2021

test: unit test for multiple configs error in resolver

227f7d4

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

SimenB requested changes Oct 4, 2021

View reviewed changes

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

a678d28

Co-authored-by: Simen Bekkhus <[email protected]>

jankaifer added 2 commits October 4, 2021 13:01

test: fixed integration test

0f81fe4

tests: fixed config issues in old tests

b6ff8b8

SimenB reviewed Oct 4, 2021

View reviewed changes

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

jankaifer added 2 commits October 4, 2021 16:08

feat: multiple jest.config.ext files are not allowed

27fdaff

test: added e2e test for --config override

0d15a1f

mrazauskas reviewed Oct 4, 2021

View reviewed changes

packages/jest-config/src/resolveConfigPath.ts Show resolved Hide resolved

jankaifer requested a review from SimenB October 4, 2021 19:09

SimenB requested changes Oct 4, 2021

View reviewed changes

e2e/__tests__/dependencyClash.test.ts Show resolved Hide resolved

e2e/esm-config/cjs/package.json Show resolved Hide resolved

e2e/__tests__/configOverride.test.ts Show resolved Hide resolved

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

jankaifer and others added 4 commits October 5, 2021 09:20

fix: more explicit expression

eccf953

Co-authored-by: Simen Bekkhus <[email protected]>

fix: added more real-life package.json example

3d71585

Merge branch 'jankaifer/warn-multiple-configs-found' of github.com:Ja…

4cca511

…nKaifer/jest into jankaifer/warn-multiple-configs-found

test: added snapshot tests for multiple errors

752b1f9

SimenB reviewed Oct 5, 2021

View reviewed changes

packages/jest-config/src/__tests__/__snapshots__/resolveConfigPath.test.ts.snap Outdated Show resolved Hide resolved

jankaifer added 2 commits October 5, 2021 10:16

test: moved snapshots from unit tests to e2e tests

02b8ef2

test: fixed rootDir replacements in snapshots

cd7c9c9

SimenB requested changes Oct 5, 2021

View reviewed changes

jankaifer and others added 2 commits October 5, 2021 11:09

Apply suggestions from code review

9e13ff2

Co-authored-by: Simen Bekkhus <[email protected]>

added missing import

f30ab95

SimenB reviewed Oct 6, 2021

View reviewed changes

packages/jest-config/src/__tests__/resolveConfigPath.test.ts Show resolved Hide resolved

Update CHANGELOG.md

1f17acc

SimenB approved these changes Oct 6, 2021

View reviewed changes

SimenB requested changes Oct 6, 2021

View reviewed changes

packages/jest-config/src/__tests__/resolveConfigPath.test.ts Outdated Show resolved Hide resolved

e2e/__tests__/multipleConfigs.ts Outdated Show resolved Hide resolved

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

jankaifer and others added 9 commits October 6, 2021 09:50

Update packages/jest-config/src/resolveConfigPath.ts

2d6efff

Co-authored-by: Simen Bekkhus <[email protected]>

Merge branch 'main' into jankaifer/warn-multiple-configs-found

3682947

add slash import

7727e74

no flat in node 10

9926edf

rename tests

91655ce

slash path in e2e test as well

d60356f

skip duplicate warning

6a52634

add copyright header

3505a2c

make warning yellow

80e52de

SimenB approved these changes Oct 6, 2021

View reviewed changes

SimenB and others added 4 commits October 6, 2021 10:54

specify key in pkg.json

8abb1c1

use wrap

746f353

removed hardcoded filename

cae826c

made skipMultipleConfigWarning optional

1de582b

SimenB reviewed Oct 6, 2021

View reviewed changes

packages/jest-config/src/resolveConfigPath.ts Outdated Show resolved Hide resolved

Update packages/jest-config/src/resolveConfigPath.ts

b57cbf5

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 merged commit cdc64c6 into jestjs:main Oct 6, 2021

jankaifer deleted the jankaifer/warn-multiple-configs-found branch October 6, 2021 11:11

ajfisher mentioned this pull request Oct 9, 2021

chore(deps-dev): bump jest from 26.6.3 to 27.2.5 nodebots/nodebots-interchange#193

Closed

pbrisbin mentioned this pull request Oct 11, 2021

Bump jest from 27.2.4 to 27.2.5 freckle/commenter-action#68

Merged

ajfisher mentioned this pull request Oct 18, 2021

chore(deps-dev): bump jest from 26.6.3 to 27.3.0 nodebots/nodebots-interchange#197

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: warn when there are multiple configs #11922

feat: warn when there are multiple configs #11922

jankaifer commented Oct 2, 2021 •

edited by SimenB

Loading

mrazauskas commented Oct 2, 2021

jankaifer commented Oct 2, 2021 •

edited

Loading

SimenB left a comment

SimenB commented Oct 4, 2021

jankaifer commented Oct 4, 2021

jankaifer commented Oct 4, 2021

SimenB left a comment

SimenB left a comment

SimenB Oct 5, 2021

jankaifer Oct 5, 2021 •

edited

Loading

SimenB Oct 5, 2021

jankaifer Oct 5, 2021

jankaifer Oct 5, 2021

jankaifer Oct 5, 2021 •

edited

Loading

jankaifer Oct 6, 2021

SimenB Oct 6, 2021

jankaifer Oct 6, 2021

SimenB Oct 6, 2021

SimenB left a comment

SimenB left a comment

jankaifer commented Oct 6, 2021

SimenB left a comment

codecov-commenter commented Oct 6, 2021

github-actions bot commented Nov 6, 2021

feat: warn when there are multiple configs #11922

feat: warn when there are multiple configs #11922

Conversation

jankaifer commented Oct 2, 2021 • edited by SimenB Loading

Summary

Test plan

Questions

mrazauskas commented Oct 2, 2021

jankaifer commented Oct 2, 2021 • edited Loading

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Oct 4, 2021

jankaifer commented Oct 4, 2021

jankaifer commented Oct 4, 2021

SimenB left a comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankaifer Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankaifer Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

jankaifer commented Oct 6, 2021

SimenB left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 6, 2021

Codecov Report

github-actions bot commented Nov 6, 2021

jankaifer commented Oct 2, 2021 •

edited by SimenB

Loading

jankaifer commented Oct 2, 2021 •

edited

Loading

jankaifer Oct 5, 2021 •

edited

Loading

jankaifer Oct 5, 2021 •

edited

Loading