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

Make jest-haste-map compute SHA-1s for excluded files too #6264

Merged
merged 1 commit into from
May 30, 2018
Merged

Make jest-haste-map compute SHA-1s for excluded files too #6264

merged 1 commit into from
May 30, 2018

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented May 25, 2018

Although Jest itself does not use SHA-1s yet, this part of the code is used by https://github.com/facebook/metro, where provided SHA-1s are used to compute cache keys.

When using watchman as a crawler, everything works well, but when using the Node crawler, Metro breaks on the first node_modules file it finds. This is because the Node crawler does not compute SHA-1s, and leaves this duty to the worker. Because node_modules are not processed to extract dependencies, their SHA-1 was never computed and Metro crashed.

This diff introduces a new, lightweight method in worker.js, which sole mission is to compute the SHA-1. It is called instead of worker when the file will be dropped from heavy processing, and only when SHA-1s are requested.

Note: all tests pass, but I haven't tested this internally yet, I'll comment once I know it fully works.

let threw = false;

try {
await getSha1({computeSha1: true, filePath: '/i/dont/exist.js'});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use

await expect(getSha1({computeSha1: true, filePath: '/i/dont/exist.js'})).rejects.toThrow()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😍

}

// $FlowFixMe: checking error code is OK if error comes from "fs".
if (['ENOENT', 'EACCES'].indexOf(error.code) < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

.includes instead of .indexOf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That code was moved, but ok :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just seen it a minute later :D But still why not used explicit API :)

@mjesun
Copy link
Contributor Author

mjesun commented May 25, 2018

I added an integration test too, forcing the Node crawler, which always returns null on SHA-1s, and verified they get computed, even for node_modules files.

const SkipOnWindows = require('../../scripts/SkipOnWindows');
const DIR = path.resolve(os.tmpdir(), 'haste_map_sha1');

SkipOnWindows.suite();
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe let's try without skipping windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@thymikee
Copy link
Collaborator

Needs a yarn lint:md pass

@thymikee
Copy link
Collaborator

Rebase + changelog and I think we're good 👍

@SimenB
Copy link
Member

SimenB commented May 29, 2018

Rebase should fix CI.

This also should have a changelog entry 😀

@mjesun
Copy link
Contributor Author

mjesun commented May 30, 2018

Rebased over master and added a CHANGELOG entry. I'll wait for the build to pass in order to merge.

@mjesun
Copy link
Contributor Author

mjesun commented May 30, 2018

:| WTF did we move the integration tests away?

@rickhanlonii
Copy link
Member

@mjesun moved them to e2e since they're e2e tests 👌

@mjesun
Copy link
Contributor Author

mjesun commented May 30, 2018

Yeah found that 😄 I also moved mine ;)

@codecov-io
Copy link

Codecov Report

Merging #6264 into master will increase coverage by 0.09%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6264      +/-   ##
==========================================
+ Coverage   63.63%   63.72%   +0.09%     
==========================================
  Files         226      226              
  Lines        8648     8665      +17     
  Branches        4        3       -1     
==========================================
+ Hits         5503     5522      +19     
+ Misses       3144     3142       -2     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.js 96.33% <100%> (+0.04%) ⬆️
packages/jest-haste-map/src/worker.js 73.97% <50%> (-3.31%) ⬇️
packages/jest-worker/src/worker.js 92.18% <0%> (-6.12%) ⬇️
packages/jest-worker/src/types.js 100% <0%> (ø) ⬆️
packages/jest-haste-map/src/crawlers/node.js 100% <0%> (+3.79%) ⬆️
packages/jest-worker/src/index.js 95.12% <0%> (+8.53%) ⬆️

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 2307643...fb9ef0d. Read the comment docs.

@github-actions
Copy link

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.

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

Successfully merging this pull request may close these issues.

None yet

6 participants