Used relative paths in haste map #7020

rubennorte · 2018-09-21T19:40:08Z

Summary

At the moment all paths in the haste map are absolute, which is convenient as all the operations in Jest use absolute paths. This has a problem though: haste maps can't be remotely cached.

This PR makes all paths in the haste map relative so they can be cached and reused in different servers (remote caching).

The approach used is to have relative paths in the haste map but make it transparent for its users (making them absolute in all the operations).

I also tried making all paths relative during serialization/deserialization, but it has a significant impact in startup time. With this approach the cost is distributed across the whole test run and the impact isn't significant overall.

Test plan

I've updated all tests and tested it in FB-infra (both single and watch mode).

SimenB · 2018-09-22T07:06:39Z

Seems to have completely broken Windows? See CI:

$ node ./packages/jest-cli/bin/jest.js --color
No tests found
In C:\projects\jest
  1894 files checked across 14 projects. Run with `--verbose` for more details.
Pattern:  - 0 matches
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

EDIT: Huh, same output on travis, so it's not an OS thing

rubennorte · 2018-09-24T15:28:32Z

@SimenB I'm still investigating those. I ran the same command in a docker container with Linux and Node 10.11.0 and it works fine :S

rubennorte · 2018-09-24T15:36:51Z

I just saw the issue after rebasing onto master. I'll fix it now.

codecov-io · 2018-09-24T16:07:31Z

Codecov Report

Merging #7020 into master will increase coverage by 0.06%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #7020      +/-   ##
==========================================
+ Coverage   66.86%   66.92%   +0.06%     
==========================================
  Files         250      250              
  Lines       10460    10489      +29     
  Branches        3        4       +1     
==========================================
+ Hits         6994     7020      +26     
- Misses       3465     3468       +3     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-runtime/src/index.js	`68.01% <ø> (ø)`	⬆️
packages/jest-haste-map/src/worker.js	`77.77% <100%> (+0.85%)`	⬆️
packages/jest-haste-map/src/crawlers/watchman.js	`95.12% <100%> (+0.18%)`	⬆️
packages/jest-haste-map/src/module_map.js	`86.44% <100%> (-1.8%)`	⬇️
packages/jest-haste-map/src/crawlers/node.js	`98.8% <100%> (+0.02%)`	⬆️
packages/jest-haste-map/src/haste_fs.js	`65.71% <66.66%> (+7.09%)`	⬆️
packages/jest-haste-map/src/index.js	`89.71% <85.71%> (-0.09%)`	⬇️
... and 3 more

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 0cd757b...2536b04. Read the comment docs.

SimenB · 2018-09-25T07:11:29Z

packages/jest-haste-map/src/crawlers/node.js


  return new Promise(resolve => {
    const callback = list => {
      const files = new Map();
      list.forEach(fileData => {
-        const name = fileData[0];
+        const filePath = fileData[0];
+        const relativeFilePath = path.relative(rootDir, filePath);


should we do any OS normalization? If the point of this is remote caching, this won't work between posix and windows, will it?

The way it's implemented now it wouldn't work between posix and windows. I guess we should normalize the paths, if I can do it without significant perf impact

Just doing path.posix.relative instead of path.relative should work, not sure if it would actually break in edge-cases for windows, though

Just doing path.posix.relative instead of path.relative should work

Yes but you'd have to normalize the path every time you access the haste map, which seems too much.

I considered making the paths relative only during serialization (which would've made this easier for windows) but that seriously impacts startup time. Doing it in runtime, as it is done now, removes that impact but requires doing normalization in runtime, which makes it harder to maintain compatibility between posix and windows. This is a tricky one and I'm not sure it's worth the effort.

Maybe we should only support remote caching when the haste map is created using the same path format.

More on that, since it'll be an undocumented feature I think we can go with this version for now. If someone needs it in the future we can revisit it.

Yeah, sounds good!👍

cpojer · 2018-09-25T18:38:47Z

packages/jest-haste-map/src/crawlers/watchman.js

@@ -159,12 +160,15 @@ module.exports = async function watchmanCrawl(

  for (const [watchRoot, response] of watchmanFiles) {
    const fsRoot = normalizePathSep(watchRoot);
-    clocks.set(fsRoot, response.clock);
+    const relativeFsRoot = path.relative(rootDir, fsRoot);


You are adding two path.relative calls here. How does this affect performance of the initial crawl?

I'm still running some tests, but it basically cancels the benefits of the Map change. I'm trying to optimize it at least for the most common cases.

I managed to make the calls to resolve and relative almost negligible. See fast_path.js.

cpojer · 2018-09-25T18:39:11Z

packages/jest-haste-map/src/haste_fs.js

  }

-  getFileIterator(): Iterator<string> {
-    return this._files.keys();
+  *getFileIterator(): Iterator<string> {


woah, fancy.

How many times can we use iterators, right? 😄

rubennorte · 2018-10-01T15:17:56Z

packages/jest-haste-map/src/index.js

    this._cachePath = HasteMap.getCacheFilePath(
      this._options.cacheDirectory,
-      `haste-map-${this._options.name}`,
+      `haste-map-${this._options.name}-${rootDirHash}`,


I removed the rootDir from the roots when generating the hash for the haste map name in the cache, but that can cause conflicts between different projects. Using the hash of the rootDir as a separate fragment of the name prevents this conflicts and allows us to easily reconstruct the cache from a file generated remotely using a different rootDir.

github-actions · 2021-05-12T08:07:57Z

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.

rubennorte requested review from cpojer and mjesun September 21, 2018 19:40

facebook-github-bot added the cla signed label Sep 21, 2018

rubennorte requested a review from SimenB September 21, 2018 19:41

rubennorte added 3 commits September 24, 2018 16:27

Used relative paths in haste map

3181917

Update changelog

a455f1c

Use relative roots for the name of the cached haste map

a10745b

rubennorte force-pushed the hastemap-with-relative-paths branch from 99bd508 to a10745b Compare September 24, 2018 15:27

Remove duplicated method

2536b04

SimenB reviewed Sep 25, 2018

View reviewed changes

SimenB approved these changes Sep 25, 2018

View reviewed changes

cpojer reviewed Sep 25, 2018

View reviewed changes

rubennorte force-pushed the hastemap-with-relative-paths branch from 64a2a5a to f5df606 Compare October 1, 2018 13:46

Improved performance of path manipulations

9a22225

rubennorte force-pushed the hastemap-with-relative-paths branch from f5df606 to 9a22225 Compare October 1, 2018 13:54

rubennorte commented Oct 1, 2018

View reviewed changes

Use rootDir in the haste map cache name

2d84704

rubennorte force-pushed the hastemap-with-relative-paths branch from 0a96100 to 2d84704 Compare October 1, 2018 15:20

rubennorte merged commit 659b048 into jestjs:master Oct 2, 2018

rubennorte deleted the hastemap-with-relative-paths branch October 2, 2018 08:56

Robinfr mentioned this pull request Jun 6, 2019

Unable to have a root on a separate drive on Windows #8536

Closed

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

Used relative paths in haste map #7020

Used relative paths in haste map #7020

rubennorte commented Sep 21, 2018

SimenB commented Sep 22, 2018 •

edited

Loading

rubennorte commented Sep 24, 2018

rubennorte commented Sep 24, 2018

codecov-io commented Sep 24, 2018

SimenB Sep 25, 2018

rubennorte Oct 1, 2018

SimenB Oct 1, 2018

rubennorte Oct 1, 2018

rubennorte Oct 1, 2018

SimenB Oct 1, 2018

cpojer Sep 25, 2018

rubennorte Oct 1, 2018

rubennorte Oct 1, 2018 •

edited

Loading

cpojer Sep 25, 2018

rubennorte Oct 1, 2018 •

edited

Loading

rubennorte Oct 1, 2018

github-actions bot commented May 12, 2021

Used relative paths in haste map #7020

Used relative paths in haste map #7020

Conversation

rubennorte commented Sep 21, 2018

Summary

Test plan

SimenB commented Sep 22, 2018 • edited Loading

rubennorte commented Sep 24, 2018

rubennorte commented Sep 24, 2018

codecov-io commented Sep 24, 2018

Codecov Report

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

Choose a reason for hiding this comment

rubennorte Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubennorte Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

SimenB commented Sep 22, 2018 •

edited

Loading

rubennorte Oct 1, 2018 •

edited

Loading

rubennorte Oct 1, 2018 •

edited

Loading