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

HasteFS file size #7580

Merged
merged 3 commits into from
Jan 9, 2019
Merged

HasteFS file size #7580

merged 3 commits into from
Jan 9, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Jan 5, 2019

Summary

This PR adds a file size field to the haste map file metadata and exposes hasteFS.getSize(path).
It also updates TestSequencer to use the file size data from the hasteFS instead of statSyncing on its own.
This will become more important if the number of file size checks increases because of #7553.
cc @SimenB @cpojer @rubennorte

The "low-level" internals like HasteMap are pretty new to me, I'd appreciate a review from someone familiar with it who knows what implications this change could have :) e.g. what happens with existing serialized haste maps? Is this breaking?
And this will certainly need approval from someone from FB.

Test plan

Added e2e test haste_map_size similar to haste_map_sha1. TestSequencer still works.

@codecov-io
Copy link

Codecov Report

Merging #7580 into master will decrease coverage by <.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7580      +/-   ##
==========================================
- Coverage   67.99%   67.99%   -0.01%     
==========================================
  Files         248      248              
  Lines        9490     9494       +4     
  Branches        6        5       -1     
==========================================
+ Hits         6453     6455       +2     
- Misses       3035     3037       +2     
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-haste-map/src/HasteFS.js 50% <0%> (-3.58%) ⬇️
packages/jest-cli/src/TestSequencer.js 100% <100%> (ø) ⬆️
packages/jest-haste-map/src/crawlers/node.js 94.73% <100%> (+0.07%) ⬆️
packages/jest-haste-map/src/index.js 93.56% <100%> (ø) ⬆️
packages/jest-haste-map/src/crawlers/watchman.js 95.55% <100%> (+0.04%) ⬆️

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 b2ffe3d...d98d7e7. Read the comment docs.

Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. I noticed the watchman integration is also done, so I'm happy with it. We can release an alpha with this once merged and then test it internally (cc @rubennorte).

packages/jest-haste-map/src/constants.js Show resolved Hide resolved
@jeysal
Copy link
Contributor Author

jeysal commented Jan 6, 2019

Rebased on master

types/HasteMap.js Show resolved Hide resolved
@jeysal
Copy link
Contributor Author

jeysal commented Jan 8, 2019

Alright, should be ready for merge then :)

@mjesun mjesun merged commit 750c172 into jestjs:master Jan 9, 2019
@jeysal jeysal mentioned this pull request Jan 9, 2019
@cpojer
Copy link
Member

cpojer commented Jan 15, 2019

This is great work, thanks for doing this and going for the proper, long fix!

@jeysal jeysal deleted the hastefs-file-size branch January 15, 2019 10:28
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@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.

7 participants