jest-resolve: check if the path is a symbolic link before resolving realpath

[asapach]

Summary

Fixes #6880: as discussed in the issue, first check if the path is a symbolic link before resolving realpath.
I've also removed all the consequent realpath calls, because the first one should already resolve the canonical physical path.

Test plan

Updated the existing tests to make sure nothing breaks.

[facebook-github-bot]

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 up 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 the corporate CLA signed.

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

[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!

[thymikee]

Anybody willing to perf test that? @mjesun @SimenB @palmerj3

[codecov-io]

Codecov Report

Merging #6906 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6906      +/-   ##
==========================================
+ Coverage   66.98%   66.99%   +<.01%     
==========================================
  Files         250      250              
  Lines       10360    10361       +1     
  Branches        3        4       +1     
==========================================
+ Hits         6940     6941       +1     
  Misses       3419     3419              
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-resolve/src/node_modules_paths.js	100% <100%> (ø)	⬆️
packages/expect/src/jest_matchers_object.js	84.61% <0%> (-1.1%)	⬇️

---

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 0b1e8c3...87e4d4b. Read the comment docs.

[palmerj3]

@thymikee I can run this build against some large jest test suites if you want and post the results

[thymikee]

Would be lovely @palmerj3!

[thymikee]

Bonus points for running on Windows

[SimenB]

This needs a changelog entry 😀 Excited about this fix, seems really reasonable.

@nickpape-msft thoughts?

[asapach]

@SimenB, changelog updated.

[palmerj3]

So I ran a large apps unit test suites against this PR, jest master, and jest 23.4.2 for comparison and here are the results. These were all run on MacOS. I'm not currently setup locally to test this on windows but if that's crucial I'll see what I can do. All test runs were with the same jest configuration and using the --no-cache flag.

Details for my local environment:

  System:
    OS: macOS Sierra 10.12.6
    CPU: x64 Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
  Binaries:
    Node: 9.11.2 - ~/.nvm/v9.11.2/bin/node
    Yarn: 1.9.2 - /usr/local/bin/yarn
    npm: 5.6.0 - ~/.nvm/v9.11.2/bin/npm

Test results:

23.4.2 - 10,691 tests - 144.828s
Master - 10,691 tests - 119.249s
This PR - 10,691 tests - 114.725s

Hope this helps!

cc @thymikee @SimenB @asapach

[SimenB]

That's awesome, thanks @palmerj3!

We've had reports of slowdown on large suites for 23 vs previous, this might be part of the issue (#5085 landed in 23 (thanks changelog!))?

[asapach]

Here's my comparison:

  System:
    OS: Windows 8.1
    CPU: x64 Intel(R) Core(TM) i5-4200M CPU @ 2.50GHz
  Binaries:
    Node: 8.11.4

Test Suites: 274
Tests:       1201
Snapshots:   564

Jest version	Cold start	Warm run
22.1.4	528s	502s
23.5.0	327s	265s
This PR	270s	214s

[palmerj3]

@thymikee think we have some spam here

[thymikee]

Not sure how to prevent spam like this, deleted for now.

[SimenB]

@mjesun any thoughts on this one?

[mjesun]

Probably @arcanis is a better one for this one.

[arcanis]

Just my opinion, but I don't think this change is a good idea.

Using realpath only when a condition is met just to support older Windows drivers seems more like a hack to me than a real fix, and since this implementation will affect all platforms (which in the large majority of cases won't even affected by the bug) I'm a bit concerned.

Something in particular that worries me is that symlinks can be anywhere in the path, there's no reason why only the last path component would be a symlink, and I'm pretty sure this change would break that (what if node_modules was the symlink itself, rather than the individual package folders?).

My suggestion if you really need to add support for this would be to 1/ guard the change so that it only applies on Windows (and ideally only on systems that suffer from this), and 2/ mimic the full realpath behavior by using fs.readlink on each component of the path.

---

@asapach You mentioned in #6880 that realpath was using GetFinalPathNameByHandleA; are you sure? Looking at the node source code, it seems to be a 'pure' js implementation that only uses lstat and readlink (vs fs.realpath.native which uses the native function).

[asapach]

@arcanis, realpath-native is being used there, so it's calling the native function: https://github.com/facebook/jest/blob/e9350f6121d4bdb378d53b53a07c6d6fba5618c2/packages/jest-resolve/src/node_modules_paths.js#L14

Something in particular that worries me is that symlinks can be anywhere in the path, there's no reason why only the last path component would be a symlink, and I'm pretty sure this change would break that

Yes, sorry, I didn't test it thoroughly enough. isSymbolicLink() will return true only on the immediate symlink, but not its children. I'll see what can be done about it.

...mimic the full realpath behavior by using fs.readlink on each component of the path.

I still think that one call to realpath should be enough, since it traverses the full path: https://github.com/nodejs/node/blob/master/lib/fs.js#L1434-L1437

Here's how I tested it - create the following directory structure

/foo/bar → /bar
/bar/baz → /baz

For /foo/bar/baz/qux/ realpath returns /baz/qux, so no matter how deep your folder structure is realpath will resolve it to canonical path. That's why I don't see the point of doing it repeatedly in the loop: https://github.com/facebook/jest/blob/e9350f6121d4bdb378d53b53a07c6d6fba5618c2/packages/jest-resolve/src/node_modules_paths.js#L48-L52

[asapach]

OK, what if we keep using native realpath (once), but wrap it in try-catch like in e.g. jest-runtime: https://github.com/facebook/jest/blob/6d3da9be06350bfa7414b609807ebd0cbdf0c416/packages/jest-runtime/src/script_transformer.js#L174-L180
I think this should also address #6880.

[asapach]

Here's the proposed solution: master...asapach:realpath
I can create a PR if that's OK.

[arcanis]

Looks much better imo 😃 I'd recommend a comment inside the catch branch to quickly explain the need for this fallback.

[asapach]

Thanks, @arcanis! Closing this PR in favor of #6925

[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.