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

fix(watchman): Fix watchman checks on Windows #5553

Merged
merged 1 commit into from
Feb 13, 2018
Merged

Conversation

BYK
Copy link
Contributor

@BYK BYK commented Feb 13, 2018

Summary

This patch fixes a long-standing issue with the watchman crawler. Watchman always returns POSIX-style paths with forward-slashes in it and Jest uses the "raw" roots array which has Windows-style paths in it. This makes the fast but naive isDescendant check that simply does a .startsWith check and causes failures in listening folders or checks.

Test plan

Run the test suite on Windows. If you keep the updated tests and undo the changes in the watchman crawler, the tests fail on Windows. With the fix in place, tests pass.

@codecov-io
Copy link

Codecov Report

Merging #5553 into master will decrease coverage by <.01%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5553      +/-   ##
=========================================
- Coverage   61.71%   61.7%   -0.01%     
=========================================
  Files         213     213              
  Lines        7149    7150       +1     
  Branches        3       3              
=========================================
  Hits         4412    4412              
- Misses       2736    2737       +1     
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-haste-map/src/crawlers/watchman.js 89.09% <94.11%> (-1.65%) ⬇️

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 2df9e4c...790b700. Read the comment docs.

@cpojer cpojer merged commit d9b4f0c into master Feb 13, 2018
@cpojer
Copy link
Member

cpojer commented Feb 13, 2018

Awesome, thank you so much for fixing this!

@SimenB SimenB deleted the fix-watchman-win branch February 14, 2018 09:30
jessecarfb pushed a commit to jessecarfb/jest that referenced this pull request Feb 14, 2018
BYK added a commit that referenced this pull request Feb 19, 2018
…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 added a commit that referenced this pull request Feb 19, 2018
…5615)

**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.
@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.

4 participants