feat(jest-haste-map): Enable crawling for symlink test files

[mrmeku]

Summary

Jest haste maps test file crawler (see packages/jest-haste-map/src/crawlers/node.ts) uses the unix find command by default to find files whose name matches the testMatch pattern defined in jest.config.js. This works great for most cases, but works poorly when executing jest under bazel.

In bazel's execution environment, all source/test files are symlinked into a sandboxed directory rather than copied over directly. Jest's crawler is currently set up to use find's type flag to filter only for files. This excludes symlinked files

Under bazel I would expect my symlinked test files to be able to be crawled. This merely involves tweaking the command line args to find to include both files and symlinks --type f ---> (--type f -o --type=l)

Test plan

I have already tested my fix against multiple bazel environments to verify that the fix enables jest to work properly for that test runner. The only risk of this change is that my adding support for crawling symlinks hampers performance of file crawling. If that is the case this behavior might be better off to be enabled behind a flag. But I do not suspect that there will be a performance hit. If reviewers have a mechanism for running jest bench marks I'm happy to create a repo filled with both files and symlinks and benchmarking the change.

[facebook-github-bot]

Hi mrmeku! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[mrmeku]

CLA has been signed

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[jeysal]

Hi @mrmeku, thanks for the issue and PR! Unfortunately I think the fix is not as simple. This will break e.g. if ln -s /etc etc.test.js - EISDIR: illegal operation on a directory, read. There's a couple of issues/PRs for proper symlink support on the tracker but it's not a simple task.

[mrmeku]

@jeysal If I handled the case for filtering out symlinked directories would you be satisfied with this change?

For background:

• I've seen the other changes related to supporting symlinks (feat: opaque symlink support #7549) but they seem to have been abandoned. Looking at the approach of that existing PR I wasn't convinced it was correct.
• In general, the approach in the PR I linked to was to track files/symlinks separately so that symlinks could be resolved to real file paths at a later stage after crawling. However that seperation does not seem necessary to me. It adds complexity to the code base and the crawler is already equipped to resolve the symlinks as it immediately calls fs.stat on those files identified by calling find.

Given that I've identified that this change does solve a bug with executing jest under bazel and prior attempts aren't showing much traction, I'd like to continue this PR. Can we work together to identify the short comings of this approach so that I can resolve those short comings?

[jeysal]

With a check that the linked node is a file I couldn't come up with an example that breaks anymore, I'm just not sure that I wholly understand the implications of the change because I'm only partially familiar with the low level parts of Jest. Unfortunately, I don't know who else would, since there is no one from FB working on Jest atm, and @SimenB, who has spent the most time on Jest, didn't do much with haste-map afaik. I only know @cpojer looked at symlink handling more broadly for Metro and Jest some time ago - not sure what the outcome was?

[mrmeku]

@jeysal I'll update my PR to do the isFile check while we search for the right code maintainer to review the change. In the worst case, I can always hide this logic behind a flag so that it's not on by default. I'll defer to the opinions of @SimenB / @cpojer on that decision.

Just my two cents, so far as I can tell, I think that the majority of user's will not be impacted by a change allowing symlinks to be crawled since the majority of users do not use symlinks. Only special workspaces / runtime environments make use of symlinks directly in their source/test code. And at least for Jest execution in Bazel's runtime env, the current default behavior is a bug and this change eliminates that bug.

[codecov-io]

Codecov Report

Merging #9351 into master will decrease coverage by 0.38%.
The diff coverage is 26.31%.

@@            Coverage Diff             @@
##           master    #9351      +/-   ##
==========================================
- Coverage   65.09%   64.71%   -0.39%     
==========================================
  Files         287      280       -7     
  Lines       12145    11998     -147     
  Branches     3007     2961      -46     
==========================================
- Hits         7906     7764     -142     
+ Misses       3604     3601       -3     
+ Partials      635      633       -2

Impacted Files	Coverage Δ
packages/jest-haste-map/src/crawlers/node.ts	72.41% <26.31%> (-14.26%)	⬇️
packages/pretty-format/src/plugins/ReactElement.ts	86.48% <0%> (-6.02%)	⬇️
packages/jest-environment-node/src/index.ts	51.16% <0%> (-5.25%)	⬇️
packages/jest-matcher-utils/src/index.ts	82.69% <0%> (-4.13%)	⬇️
packages/jest-reporters/src/coverage_reporter.ts	51.21% <0%> (-2.68%)	⬇️
packages/jest-resolve/src/index.ts	45.25% <0%> (-2.46%)	⬇️
packages/jest-snapshot/src/inline_snapshots.ts	80.39% <0%> (-1.61%)	⬇️
...es/pretty-format/src/plugins/ReactTestComponent.ts	81.81% <0%> (-1.52%)	⬇️
packages/jest-transform/src/ScriptTransformer.ts	68.63% <0%> (-1.01%)	⬇️
packages/expect/src/jestMatchersObject.ts	75% <0%> (-0.87%)	⬇️
... and 60 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 2ee214d...24d8b03. Read the comment docs.

[SimenB]

Can we add a test (e2e I guess due to the nature of the change)?

[mrmeku]

@SimenB e2e test added successfully

[mrmeku]

@SimenB I've updated the commit to fix lint errors, include copywrite for the test file I added and also augmented the non-native file crawler to pick up symlink files as well.

[cpojer]

Thank you so much for adding this feature. Since you are using Bazel, is this work related to Google's adoption of Jest or is this separate?

The only concern I have is adding features to the node crawler that the watchman one does not support. If somebody has watchmen installed but falls back to node for some reason, Jest should be able to reconcile between the two. If we add this feature, different amounts of tests may be found depending on the current state of the machine or other environment factors.

[mrmeku]

@cpojer Sorry for the late reply, I went on a vacation last week :)

This isn't directly related to Google's adoption of a new test runner (I don't work there anymore so I'm a bit out of the loop). I got started on this project as a means of making the Angular CLI run under bazel more or less without any special customization. The Angular CLI is capable of running jest when using the @nrwl/jest package that my company I work for maintains. And in addition to that, I just like jest and want it to be the defacto test runner for running JS tests under bazel.

@cpojer One question. I'm willing to augment watchmen to have the same behavior if need be (it doesn't affect the bazel use case, but if its a requirement for getting the PR merged I could be persuaded to make a PR). So my question to you is, is that a hard requirement for getting this logic merged? Could it be made to be follow up work after merging this PR?

[cpojer]

@mrmeku thanks for the reply and hope you enjoyed your vacation! I'd much prefer if we could merge watchman support for this given the concerns I outlined above that will lead to inconsistency and a poor experience with Jest when the underlying system is flaky. I am happy if you add that in a separate PR and would prefer holding off on merging this one until most of the watchman support is available or once we at least get the biggest blockers out of the way – or otherwise you make a big promise that you will do that. Otherwise we may need to disable or unland this change to not cause issues in Jest.

[SimenB]

@mrmeku were you able to look into the watchman implementation? This'll also conflict with #9514, but that shouldn't really affect your implementation

[mrmeku]

@SimenB I'm taking a look into it tonight. Life got a little hectic so I had to delay things for a little while

[mrmeku]

@SimenB @cpojer It looks like watchman's symlink's support is off by default, but it does support a flag to enable crawling symlinks: https://github.com/facebook/watchman/blob/b3895be21e4502fc1fcaae75b146a703bc134fce/root/stat.cpp#L232

That being the case, I'm not exactly sure what the best change to make is. So far as I can tell, we don't have a way to override the settings within a user's watchmanrc (nor do I think we would want to). With all that context in mind, here are a couple of options that I've though of:

1. Print a warning if a symlink was followed and the project's watchmanrc file does not have "watch_symlinks": true
2. Make following symlinks opt-in via a flag to mirror the behavior of watchman

I think I'm partial to #2 since its the least complex option.

[SimenB]

I'm down with adding a symlink option. @cpojer @jeysal @thymikee thoughts?

[SimenB]

Thanks! I think this is finally good to go. To be safe I'll wait to land it until we start landing breaking changes (mostly because of the find changes, I've broken that before without knowing). That will start landing this week though, so not a long wait 🙂

Thanks for sticking with this, it's taken a while with much back and forth!

(I landed parts of the docs changes in 038d8be, btw)

[mrmeku]

Thanks for all your patience :)

[alexeagle]

Ping @SimenB could you find time to get this in? The community of Bazel users is all patching this change in, after tripping over the symlink problem. It's becoming a serious barrier to adoption since Jest is so popular :)

[alexeagle]

sorry @SimenB I know we're all busy OSS maintainers, but this one would be a big help for the Bazel community. it trips all novice users on their way in.

[SimenB]

Hey @alexeagle, sorry about the radio silence here!

We discussed this a bit, and got some pushback since it necessitates turning off watchman, which in theory leads to worse performance, especially during startup/initial crawl and watch mode re-runs. This led us down the rabbit hole of trying to benchmark the crawler implemented with node fs vs find vs watchman. And we actually found node crawler and watchman to be pretty similar, while find is way slower (at least on the macs and linuxes we tested), which is a change from when it was implemented (at the time find was about 10x faster than the crawl). But that means we should possibly not use find by default when disabling watchman since it's slower. We ended up agreeing that we should probably write an issue about this and get more people to test the hypothesis, then it just stalled since nobody had the energy to write up that post. Then we (or at least I) just forgot the whole thing 😅

---

Aaaaaanyway, I didn't know you were actually using a patch to get this. I assume that means that this change works for Bazel users?

[liamjones]

@SimenB thank you for the extra info. When you were talking about node fs vs find vs watchman is that the decision that's made here?: https://github.com/facebook/jest/blob/40d04d8aaca5f6add7398345f6821af2cad24e69/packages/jest-haste-map/src/index.ts#L779

E.g. is the selection of FSEventsWatcher the find you were talking about or am I misunderstanding?

[SimenB]

Both find and fs are nodeCrawl here: https://github.com/facebook/jest/blob/40d04d8aaca5f6add7398345f6821af2cad24e69/packages/jest-haste-map/src/index.ts#L723-L724, then https://github.com/facebook/jest/blob/40d04d8aaca5f6add7398345f6821af2cad24e69/packages/jest-haste-map/src/crawlers/node.ts#L207

[alexeagle]

I didn't know you were actually using a patch to get this. I assume that means that this change works for Bazel users?

yes, we have the patch in all our examples (e.g. https://github.com/bazelbuild/rules_nodejs/blob/stable/examples/jest/patches/jest-haste-map%2B24.9.0.patch) and have instructed all users to install it as well.

[SimenB]

#yolo

[SimenB]

27.0.0-next.7 is released with this 🙂

[github-actions]

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.