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.

Sign up for GitHub

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

Jump to bottom

fix(haste-map): multiple resolutions on failure #12420

Merged
merged 2 commits into from Feb 18, 2022
Merged

fix(haste-map): multiple resolutions on failure #12420

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a24d99a
fix(haste-map): multiple resolutions on failure
oliversalzburg Feb 17, 2022
62cb293
Update CHANGELOG.md
SimenB Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@
- `[jest-environment-jsdom]` Make `jsdom` accessible to extending environments again ([#12232](https://github.com/facebook/jest/pull/12232))
- `[jest-jasmine2, jest-types]` [**BREAKING**] Move all `jasmine` specific types from `@jest/types` to its own package ([#12125](https://github.com/facebook/jest/pull/12125))
- `[jest-matcher-utils]` Pass maxWidth to `pretty-format` to avoid printing every element in arrays by default ([#12402](https://github.com/facebook/jest/pull/12402))
- `[jest-haste-map]` Don't use partial results if search errors ([#12420](https://github.com/facebook/jest/pull/12420))

### Chore & Maintenance

Expand Down
4 changes: 3 additions & 1 deletion packages/jest-haste-map/src/crawlers/node.ts
View file
Open in desktop
Expand Up @@ -71,7 +71,9 @@ function find(
fs.readdir(directory, {withFileTypes: true}, (err, entries) => {
activeCalls--;
if (err) {
callback(result);
if (activeCalls === 0) {
Copy link
Member

@SimenB SimenB Feb 18, 2022 •
edited

Choose a reason for hiding this comment

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

I think this makes sense. is the error you're seeing mitigated by this change, or is it an educated guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is mitigated. I was running Jest in a debugger in our monorepo to understand what was going on and this change reliably restored operations to give stable results.

Here is an example of 2 consecutive runs in our current monorepo with the code as-is:

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  6541 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 6541 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  5436 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 5436 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

Note that the number of files checked decreased. This is the result of the map being generated, hitting the error, and then persisting the partial map to disk. Operations then continue, the search finally completes and the full set of files is pushed into the map.
When I run the second time, I only get the files that were in the cache from the first run.

After making the change suggested in this PR, the file count will restore to the original, full count:

$ vi node_modules/jest-haste-map/build/crawlers/node.js

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  6541 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 6541 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

All runs after that contain the correct amount of files.

Copy link
Member

Choose a reason for hiding this comment

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

cool, good enough for me! 😀

callback(result);
}
return;
}
entries.forEach(entry => {
Expand Down