sets project name if empty fixes #5597 #5862

cbelsole · 2018-03-23T03:25:26Z

Summary

When a project name does not exist it uses the name based on the main rootDir. This causes the incorrect resolver to be selected since all projects names resolve the same. The fix is to create a unique name for projects that do not have one.

fixes: #5597

Test plan

Added unit test:

projects without names
projects with rootDir
projects with names

cbelsole · 2018-03-23T03:29:28Z

The tests do not pass right now because I am having trouble with the types. If anyone has advice I would appreciate it.

SimenB · 2018-03-23T06:27:25Z

Huh, I had no idea name was a thing. You learn something every day!

packages/jest-config/src/normalize.js

types/Config.js

CHANGELOG.md

SimenB · 2018-03-23T06:32:24Z

packages/jest-config/src/normalize.js

+  options.projects = (options.projects || []).map(
+    (project: string | ProjectConfig, index) => {
+      if (!typeof project !== 'string' && !project.hasOwnProperty('name')) {
+        project.name = crypto


this seems to fail on CI: https://circleci.com/gh/facebook/jest/17682

packages/jest-config/src/normalize.js

types/Config.js

SimenB · 2018-03-25T16:35:36Z

integration-tests/timeouts/__tests__/a-banana.js

@@ -0,0 +1,9 @@
+


this directory shouldn't be checked in (it's created by integration tests)

packages/jest-config/src/normalize.js

cpojer · 2018-03-26T10:19:30Z

Thanks for the fix! I'm a bit worried that this diff will only fix this issue for uses of the multi project runner where the config objects are passed in but it won't work when specifying multiple paths to various configuration files.

cbelsole · 2018-03-26T14:39:30Z

@cpojer I think I can see what you are getting at. I spiked combining https://github.com/facebook/jest/blob/master/packages/jest-cli/src/cli/index.js#L228 and https://github.com/facebook/jest/blob/master/packages/jest-cli/src/cli/index.js#L248 to something like:

  if (projects.length > 0) {
    const parsedConfigs = projects
      .filter(root => {
        if (
          typeof root === 'string' &&
          fs.existsSync(root) &&
          !fs.lstatSync(root).isDirectory() &&
          !root.endsWith('.js') &&
          !root.endsWith('.json')
        ) {
          return false;
        }

        return true;
      })
      // readConfig recursively finds project paths by looking in `parsedConfig.projects`  `globalConfig.projects` and parses them as usual
      .map(root => readConfig(argv, root, true, configPath))
      .flatten();
...
  }

but I could not find a solution that I was happy with.

cbelsole · 2018-03-26T14:57:52Z

It looks like the only failing test is a test that is failing in master.

SimenB · 2018-03-26T15:56:04Z

Failing test tracked in #5871

cpojer · 2018-03-26T18:56:39Z

@cbelsole you are right, I think that's the right location to make this change. What exactly were you not sure about?

cbelsole · 2018-03-26T20:01:00Z

@cpojer I needed more background on how the config path was being passed in. It was not being set for me when I needed it to. So I backed that change out as to not blow up the scope of this pr.

SimenB · 2018-08-09T14:00:57Z

@cbelsole do you think we could land this? Maybe @paularmstrong has some thoughts on Christoph's suggestion?

paularmstrong · 2018-08-09T22:08:02Z

I'm not sure I understand @cpojer's request. For me as an end-user: as long as it works, I'm happy :) Currently working around this by manually specifying project name

cbelsole · 2018-08-10T23:21:08Z

@SimenB I merged master back into the branch to resolve the merge conflicts. This PR is ready to go since it fixes the issue we set out to fix. @cpojer was talking about merging all the ways one could set configs which this PR does not address.

packages/jest-config/src/normalize.js

CHANGELOG.md

SimenB · 2018-12-25T14:19:36Z

@cbelsole ping 🙂

…mpty

packages/jest-config/src/normalize.js

thymikee · 2019-01-03T20:42:40Z

packages/jest-config/src/normalize.js

  if (!options.name) {
    options.name = crypto
      .createHash('md5')
      .update(options.rootDir)
      .digest('hex');
  }

+  if (Array.isArray(options.projects)) {
+    options.projects = options.projects.map((project, index) => {


normalize is run on every project config, once per run, data is not persisted. I don't think it's necessary to double loop like this. Seems like simply adding a timestamp to the equation fixes the problem (or maybe I'm missing a bigger picture @SimenB):

diff --git a/packages/jest-config/src/normalize.js b/packages/jest-config/src/normalize.js index 713c8b0da..11d61e080 100644 --- a/packages/jest-config/src/normalize.js +++ b/packages/jest-config/src/normalize.js @@ -270,6 +270,7 @@ const normalizeMissingOptions = (options: InitialOptions): InitialOptions => { options.name = crypto .createHash('md5') .update(options.rootDir) + .update(Date.now().toString()) .digest('hex'); }

Math.random() seems safer in case the loop completes in less than 1ms, but I agree that should probably solve it as well

Agreed. I'd go with uuid/v4 for generating random number though, as Math.random is not so random after all and I'd hate to debug issue like this 😅.
Tested it locally and it fixes the issue (on Node 8, this particular example is not reproducible on Node 10)

@thymikee which double loop do you mean? It's just a single loop through projects, isn't it?

@thymikee which double loop do you mean? It's just a single loop through projects, isn't it?

normalize is run for every project config, so essentially we loop through projects once again here, hence double loop. And the solution I proposed doesn't have this issue.

Push it? 😀 I fixed my Set comment, would be nice to get this fix into Jest 24

Done. I checked the repro and it still works as expected. I should probably add an e2e test to be sure.

thymikee · 2019-01-20T19:21:48Z

e2e/__tests__/multiProjectRunner.test.js

@@ -399,3 +399,41 @@ test('Does transform files with the corresponding project transformer', () => {
  expect(stderr).toMatch('PASS project1/__tests__/project1.test.js');
  expect(stderr).toMatch('PASS project2/__tests__/project2.test.js');
 });
+
+test("doesn't bleed module file extensions resolution with multiple workers", () => {


While running a repro again, I discovered this only happens when we run multiple workers, so added this test. It fails without uuid.

github-actions · 2021-05-12T03:12:37Z

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.

sets project name if empty fixes jestjs#5597

5756129

facebook-github-bot added the cla signed label Mar 23, 2018

added to changelog

50d9933

SimenB reviewed Mar 23, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

SimenB reviewed Mar 23, 2018

View reviewed changes

types/Config.js Outdated Show resolved Hide resolved

SimenB reviewed Mar 23, 2018

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

SimenB reviewed Mar 23, 2018

View reviewed changes

thymikee requested changes Mar 23, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

types/Config.js Outdated Show resolved Hide resolved

Chris Belsole and others added 5 commits March 23, 2018 10:15

cr comments

6036dfa

removed unused import

be1cc18

fixed tests

6054f0b

use inital options instead of project config

feb9751

fix unintentional type change

0bf6d8e

SimenB reviewed Mar 25, 2018

View reviewed changes

cpojer reviewed Mar 26, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

cpojer reviewed Mar 26, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

Chris Belsole added 4 commits March 26, 2018 09:51

fixed path string and deleted unneeded test files

4f72619

Merge branch 'master' into set_project_name_if_empty

b7c061f

merged in master

0713d0f

updated snapshots and tests

65c64d9

removed updated snapshots put change under fixes

a856fb1

Merge branch 'master' into set_project_name_if_empty

04b0bb6

Merge branch 'master' into set_project_name_if_empty

30fecea

Merge remote-tracking branch 'origin' into set_project_name_if_empty

54302cf

SimenB reviewed Aug 13, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

SimenB reviewed Sep 17, 2018

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

cbelsole and others added 4 commits December 29, 2018 14:06

Merge remote-tracking branch 'root/master' into set_project_name_if_e…

be40277

…mpty

unique names across config and projects

8c8761d

fixed changelog

78801aa

Update CHANGELOG.md

9bbbd61

SimenB reviewed Dec 29, 2018

View reviewed changes

packages/jest-config/src/normalize.js Outdated Show resolved Hide resolved

thymikee reviewed Jan 3, 2019

View reviewed changes

SimenB and others added 6 commits January 20, 2019 17:05

Merge branch 'master' into set_project_name_if_empty

61523b0

use Set

44aa91d

use random uuid for assigning project name

a370c21

remove unnecessary test

1ab0bac

fix lint

691339f

add e2e test

6fcab16

thymikee reviewed Jan 20, 2019

View reviewed changes

SimenB approved these changes Jan 20, 2019

View reviewed changes

SimenB merged commit 388942f into jestjs:master Jan 20, 2019

This was referenced Jan 29, 2019

Jest 24 is 67% slower than Jest 23.6.0 on the same test suite #7732

Closed

fix: make sure generated name in config is stable across runs of Jest #7746

Merged

This was referenced Feb 6, 2019

Multi-project runner can't find some mocks #4119

Closed

browser field is not respected in concurrent mode w/ multi-project config #6887

Closed

captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019

sets project name if empty fixes jestjs#5597 (jestjs#5862)

23d1405

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sets project name if empty fixes #5597 #5862

sets project name if empty fixes #5597 #5862

cbelsole commented Mar 23, 2018

cbelsole commented Mar 23, 2018

SimenB commented Mar 23, 2018

SimenB Mar 23, 2018

SimenB Mar 25, 2018 •

edited

Loading

cpojer commented Mar 26, 2018

cbelsole commented Mar 26, 2018

cbelsole commented Mar 26, 2018

SimenB commented Mar 26, 2018

cpojer commented Mar 26, 2018

cbelsole commented Mar 26, 2018

SimenB commented Aug 9, 2018

paularmstrong commented Aug 9, 2018

cbelsole commented Aug 10, 2018

SimenB commented Dec 25, 2018

thymikee Jan 3, 2019

SimenB Jan 3, 2019 •

edited

Loading

thymikee Jan 3, 2019

SimenB Jan 20, 2019

thymikee Jan 20, 2019

SimenB Jan 20, 2019

thymikee Jan 20, 2019

thymikee Jan 20, 2019

github-actions bot commented May 12, 2021

sets project name if empty fixes #5597 #5862

sets project name if empty fixes #5597 #5862

Conversation

cbelsole commented Mar 23, 2018

Summary

Test plan

cbelsole commented Mar 23, 2018

SimenB commented Mar 23, 2018

Choose a reason for hiding this comment

SimenB Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

cpojer commented Mar 26, 2018

cbelsole commented Mar 26, 2018

cbelsole commented Mar 26, 2018

SimenB commented Mar 26, 2018

cpojer commented Mar 26, 2018

cbelsole commented Mar 26, 2018

SimenB commented Aug 9, 2018

paularmstrong commented Aug 9, 2018

cbelsole commented Aug 10, 2018

SimenB commented Dec 25, 2018

Choose a reason for hiding this comment

SimenB Jan 3, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

SimenB Mar 25, 2018 •

edited

Loading

SimenB Jan 3, 2019 •

edited

Loading