fix(watchman): Overhauls how Watchman crawler works fixing Windows #5615

BYK · 2018-02-19T16:19:16Z

Summary

Watchman crawler was ignoring the relative_path field in the response of
a watch-project call, requiring it to match watch roots with the actual
project roots afterward. Not only this was inefficient, it was also faulty
due to the naive .startsWith() check in isDescendant(). This was causing
issues both with Windows file paths (#5553) and after that with case-insensitive
file systems where the names from Watchman did not match the casing of the passed
roots.

This patch replaces all that logic by taking the relative_path field into account
and does some consolidation along with using async/await instead of promises.

Test plan

Run the updated test suite on all platforms and make sure it passes. I've also
verified this on some internal Windows repos by manually patching the built module
and making sure there are no warnings regarding duplicated haste names due to incorrect
crawling of project root siblings.

…sues **Summary** Watchman crawler was ignoring the `relative_path` field in the response of a `watch-project` call, requiring it to match watch roots with the actual project roots afterwards. Not only this was inefficient, it was also faulty due to the naive `.startsWith()` check in `isDescendant()`. This was causing issues both with Windows file paths (#5553) and after that with case-insensitive file systems where the names from Watchman did not match the casing of the passed roots. This patch replaces all that logic by taking the `relative_path` field into account and does some consolidation along with using `async`/`await` instead of promises. **Test plan** Run the updated test suite on all platforms and make sure it passes. I've also verified this on some internal Windows repos by manually patching the built module and making sure there are no warnings regarding duplicated haste names due to incorrect crawling of project root siblings.

BYK · 2018-02-19T16:24:11Z

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

+ `Watchman error: ${error.message.trim()}. Make sure watchman ` +
+ `is running for this project. See ${watchmanURL}.`,
+ );
+ const stack = error.stack;


I guess I can merge stack traces instead since error.stack should never be empty.

BYK · 2018-02-19T16:24:38Z

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

@@ -18,126 +18,127 @@ import H from '../constants';
 const watchmanURL =
 'https://facebook.github.io/watchman/docs/troubleshooting.html';

-function isDescendant(root: string, child: string): boolean {
- return child.startsWith(root);
+class WatchmanError extends Error {


Reason for this overhaul was to make stack traces useful since otherwise I always ended up in the helper function which was far from being helpful.

Not a fan of overriding the stack field. Can you just mutate the message of the original error instance instead?

BYK · 2018-02-19T16:25:17Z

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

 const ROOTS = [FRUITS, VEGETABLES];
 const BANANA = path.join(FRUITS, 'banana.js');
 const STRAWBERRY = path.join(FRUITS, 'strawberry.js');
 const KIWI = path.join(FRUITS, 'kiwi.js');
 const TOMATO = path.join(FRUITS, 'tomato.js');
 const MELON = path.join(VEGETABLES, 'melon.json');
+const WATCH_PROJECT_MOCK = {


This actually makes the common test scenarios more similar to real-world cases and increases coverage a bit.

BYK · 2018-02-19T16:25:48Z

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

- reject(WatchmanError(error));
- });
- });
+ if (clientError) {


Hoping this approach works and is OKAY. Otherwise I'd need to wrap the whole thing in a new Promise() block :(

codecov-io · 2018-02-19T16:36:24Z

Codecov Report

Merging #5615 into master will increase coverage by 0.02%.
The diff coverage is 91.37%.

@@            Coverage Diff             @@
##           master    #5615      +/-   ##
==========================================
+ Coverage   60.65%   60.67%   +0.02%     
==========================================
  Files         214      213       -1     
  Lines        7314     7316       +2     
  Branches        4        4              
==========================================
+ Hits         4436     4439       +3     
+ Misses       2877     2876       -1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-haste-map/src/crawlers/watchman.js	`91.66% <91.37%> (+2.57%)`	⬆️
packages/jest-haste-map/src/worker.js	`96% <0%> (ø)`	⬆️
packages/jest-haste-map/src/blacklist.js

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 bea889c...69607dd. Read the comment docs.

jeanlauliac · 2018-02-19T16:36:42Z

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

 expect(client.end).toBeCalled();
 });
 });

- it('updates the file object when the clock is given', () => {
+ test('updates the file object when the clock is given', () => {


why did you switch to test instead of it?

it is not directly documented and seems like the preference is to use test instead.

jeanlauliac · 2018-02-19T16:39:46Z

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

@@ -18,126 +18,127 @@ import H from '../constants';
 const watchmanURL =
 'https://facebook.github.io/watchman/docs/troubleshooting.html';

-function isDescendant(root: string, child: string): boolean {
- return child.startsWith(root);
+class WatchmanError extends Error {


Not a fan of overriding the stack field. Can you just mutate the message of the original error instance instead?

jeanlauliac

Okay that sounds reasonnable

davidaurelio

Thanks for fixing windows woes :-) that’s awesome!

Replacing .then() with async functions is a good idea, but readability is not as good as it could be.

You also sacrificed the parallelism that we had previously.

I think a short try/catch block with await Promise.all(...) would be a good improvement in both regards.

davidaurelio · 2018-02-21T11:06:41Z

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

+const WATCH_PROJECT_MOCK = {
+ [FRUITS]: {
+ relative_path: 'fruits',
+ watch: forcePOSIXPaths(ROOT_MOCK),


why are you forcing posix paths here? Wouldn’t this be a result of something like path.join(__dirname, 'ab/cd/ef')?

Watchman always returns forward slashes and path.join() uses \ as path separator on Windows. To correctly mimic Watchman responses, we need this. Am I missing something?

no, I just missed that information in this context. All good :-)

path.posix.join?

I don't think that replaces existing backslashes.

> path.posix.join('M:\\lol') 'M:\\lol'

Ah, good point!

Could use slash, not sure if better, though.

> const slash = require('slash'); undefined > slash('M:\\lol') 'M:/lol'

Not sure if it is worth the effort or the extra devDependency only for tests only to support UNC paths but if you think this would be an improvement, I can submit another PR?

Nah, it's fine (we have slash already, fwiw)

we have slash already, fwiw

Didn't see it in jest-haste-map, that's why :D

davidaurelio · 2018-02-22T15:42:53Z

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

+ try {
+ const watchmanRoots = new Map();
+ for (const root of roots) {
+ const response = await cmd('watch-project', root);


this is effectively serialising watch-project commands, did you check whether this incurs a speed penalty?

I didn't notice any slowdowns but you are right. For some reason I thought await in a for loop would not be blocking. I'll submit a follow-up to make this parallel again. Great catch!

davidaurelio · 2018-02-22T15:49:57Z

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


- const clocks = data.clocks;
- let files = data.files;
+ try {


this is a gigantic try/catch, and really hard to read. Was it infeasible to break it up into functions?

Well, it is because I tried to mimic the earlier behavior which was not very visible. This is also mostly to wrap the error in that custom text we have. I can try narrowing this down at the risk of not being fully backward compatible regarding error handling and error message customization.

davidaurelio · 2018-02-22T15:53:02Z

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

- }
+ let shouldReset = false;
+ const watchmanFileResults = new Map();
+ for (const [root, directoryFilters] of watchmanRoots) {


not parallelizing means that you have to wait for all watch-project commands before you can send the queries.

davidaurelio · 2018-02-22T15:53:28Z

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

+ : // Otherwise use the `suffix` generator
+ {expression, fields, suffix: extensions};
+
+ const response = await cmd('query', root, query);


... and all queries are serialised, too

BYK · 2018-02-22T16:08:58Z

Will make things parallel again, thanks for the review!

This is a follow up to #5615 where it made all async watchman commands serialized when converting them from promises to `async` functions. Existing Watchman crawler tests should pass, maybe slightly faster.

github-actions · 2021-05-12T20:06: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.

BYK added 🐛 Bug 🚀 Enhancement Hi-Pri Windows labels Feb 19, 2018

BYK assigned cpojer Feb 19, 2018

BYK requested review from cpojer and jeanlauliac February 19, 2018 16:19

facebook-github-bot added the CLA Signed ✔️ label Feb 19, 2018

BYK commented Feb 19, 2018

View reviewed changes

jeanlauliac reviewed Feb 19, 2018

View reviewed changes

jeanlauliac approved these changes Feb 19, 2018

View reviewed changes

BYK closed this Feb 19, 2018

BYK reopened this Feb 19, 2018

Better error handling

69607dd

BYK force-pushed the watchman-crawler-overhaul branch from 2fe9780 to 69607dd Compare February 19, 2018 17:27

BYK merged commit adbd927 into master Feb 19, 2018

BYK deleted the watchman-crawler-overhaul branch February 19, 2018 18:13

SimenB added Needs Changelog and removed Needs Changelog labels Feb 19, 2018

This was referenced Feb 20, 2018

Update jest in / from 22.3.0 to 22.4.0 opencomponents/storage-adapters#48

Closed

Update 14 dependencies from npm storybookjs/storybook#3041

Merged

davidaurelio reviewed Feb 22, 2018

View reviewed changes

BYK mentioned this pull request Feb 22, 2018

fix(watchman): Parallelize Watchman calls in crawler again #5640

Merged

This was referenced Feb 26, 2018

jest versions available: 22.4.2, 22.4.1, 22.4.0 serlo/serlo-abc#269

Closed

Update jest in / from 22.0.4 to 22.4.2 dailybruin/sources#140

Closed

Update jest in / from 22.3.0 to 22.4.2 severest/retrobot#44

Merged

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

fix(watchman): Overhauls how Watchman crawler works fixing Windows #5615

fix(watchman): Overhauls how Watchman crawler works fixing Windows #5615

BYK commented Feb 19, 2018

BYK Feb 19, 2018

BYK Feb 19, 2018

jeanlauliac Feb 19, 2018

BYK Feb 19, 2018

BYK Feb 19, 2018

codecov-io commented Feb 19, 2018 •

edited

Loading

jeanlauliac Feb 19, 2018

BYK Feb 19, 2018

jeanlauliac Feb 19, 2018

jeanlauliac left a comment

davidaurelio left a comment

davidaurelio Feb 21, 2018

BYK Feb 22, 2018

davidaurelio Feb 22, 2018

SimenB Feb 23, 2018

BYK Feb 23, 2018

SimenB Feb 23, 2018 •

edited

Loading

BYK Feb 23, 2018 •

edited

Loading

SimenB Feb 23, 2018

BYK Feb 23, 2018

davidaurelio Feb 22, 2018

BYK Feb 22, 2018

davidaurelio Feb 22, 2018

BYK Feb 22, 2018 •

edited

Loading

davidaurelio Feb 22, 2018

davidaurelio Feb 22, 2018

BYK commented Feb 22, 2018

github-actions bot commented May 12, 2021

fix(watchman): Overhauls how Watchman crawler works fixing Windows #5615

fix(watchman): Overhauls how Watchman crawler works fixing Windows #5615

Conversation

BYK commented Feb 19, 2018

Summary

Test plan

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

codecov-io commented Feb 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanlauliac left a comment

Choose a reason for hiding this comment

davidaurelio left a 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

SimenB Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

BYK Feb 23, 2018 • 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

BYK Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Feb 22, 2018

github-actions bot commented May 12, 2021

codecov-io commented Feb 19, 2018 •

edited

Loading

SimenB Feb 23, 2018 •

edited

Loading

BYK Feb 23, 2018 •

edited

Loading

BYK Feb 22, 2018 •

edited

Loading