fix: absolute paths in `moduleDirectories` are invalid in Windows OS #5398

warren-bank · 2018-01-26T04:08:27Z

closes: Issue #5396

facebook-github-bot · 2018-01-26T04:08:51Z

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!

codecov-io · 2018-01-26T04:19:05Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #5398      +/-   ##
==========================================
+ Coverage   62.24%   62.24%   +<.01%     
==========================================
  Files         205      205              
  Lines        6931     6932       +1     
  Branches        3        3              
==========================================
+ Hits         4314     4315       +1     
  Misses       2616     2616              
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-resolve/src/node_modules_paths.js	`100% <100%> (ø)`	⬆️

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 ffb3be8...f7f91bb. Read the comment docs.

warren-bank · 2018-01-26T04:28:45Z

CLA (Contributor License Agreement) has now been signed.

facebook-github-bot · 2018-01-26T04:31:36Z

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

SimenB

Can you include a test for this? Also, please update the changelog.

Don't worry about multiple commits, btw. We squash on merge 🙂

SimenB · 2018-01-26T07:40:21Z

packages/jest-resolve/src/__tests__/resolve.test.js

+  });
+
+  it('can resolve node modules relative to absolute paths in "moduleDirectories"', () => {
+    const cwd = 'D:\\project';


this won't pass on a mac or linux. Can you switch on os.platform === 'win32' and use unix style paths? We run CI on appveyor, so it should still guard against regression

warren-bank · 2018-01-26T09:39:13Z

@SimenB ..mind if I ask you a quick question?
I'm pretty much brand new to Jest, so maybe you'll know the answer without digging.

As you saw in the first iteration of the unit tests, I wanted to test how path methods would be used to generate the dirs Array on a win32 platform.. but the test was running in a Linux environment.. and things went haywire.

I actually didn't notice your comment until after I'd iterated and written a 2nd version. Your suggestion to use a switch statement to run one test that's dependent on the platform of the machine executing the test runner is one way to go. But it does leave a lot of things untested.

The idea I had was to mock the global path module. Since I had no idea how the right way to do that in Jest would be.. I simply wrote beforeAll and afterAll hooks to backup and restore the correct path attributes.. and during each of the test cases, they're temporarily replaced by the platform-specific implementations.. which path makes available in path.win32 and path.posix.

The methodology is simple and sound. The problem that I find myself needing to work around.. is that the it() test cases are being run in parallel, rather than sequentially one after the next. This is causing my bit of trickery to behave unpredictably.. as the tests are interfering with each other. I see that there's a command-line switch --runInBand that causes all tests to run sequentially. But obviously the command is being issued by an automated script.. and besides.. I only want to run 2 tests in order.. not the entire suite.

Is there any way to chain two tests together.. programmatically.. in code?

If not, should I combine the two tests into a single it() function? It's not ideal.. since it violates the rule of one expect per test case. But, it seems like it may be the only way for the tests to run properly.

Any ideas?

SimenB · 2018-01-26T09:51:37Z

I'd use a combination of jest.doMock and jest.resetModules, that way you can mock path with either path.unix or path.win32 dependent on which behaviour you want.

An alternative is to write an integration testing your change on a higher level, instead of the actual paths being used. Not sure if that's viable here, though.

warren-bank · 2018-01-26T10:00:50Z

if I was to call jest.doMock('path').. would other modules that already have a reference to the global path library somehow automagically begin using the new mocked object until it gets reset by jest.resetModules?

and if somehow that was possible.. what would the behavior be within those other modules when multiple tests running in parallel each attempt to mock this same library?

warren-bank · 2018-01-26T10:07:27Z

more concretely:

resolve.test.js contains:
const Resolver = require('../');
../index.js contains:
import nodeModulesPaths from './node_modules_paths';
../node_modules_paths.js contains:
import path from 'path';
- and its nodeModulesPaths method uses path to output an array of platform-dependent filepaths

so..
if resolve.test.js contains 2 (or more) it() test cases that each want to temporarily modify the value of the global path library.. it doesn't seem like a mock Object could work. Hmm..

warren-bank · 2018-01-26T10:09:46Z

Would it be terrible of me if I just combine the 2 tests into a single function.. so they can run as one sequential block?

warren-bank · 2018-01-26T23:23:51Z

quick status update..

branch dedicated to debugging the problem

includes some minimal node scripts to debug the real culprit
turns out.. the issue is actually with Node's path module
when an attribute is assigned to path, its new value is also transparently assigned to the same attribute in path[platform]
this observation quickly led to a working test case..

passing unit tests

first iteration, based on most my recently failed test
returns a chained Promise
overall, bit of a shit-show..
I'll revert the structure of the unit tests back from a single Promise chain to 2 distinct it() methods.
At this point, I'm not so sure that the two it() methods run concurrently..

observation:

it's kind of amusing that the changes to the actual library took about 5 minutes to write
and writing a unit test that doesn't break the internet took about 5 hours (argh..)

warren-bank · 2018-01-26T23:26:20Z

PS: sorry that there's a commit that's floating toward the end of this thread. I pushed it from a virtual machine that had its clock set without a timezone. My bad.

warren-bank · 2018-01-26T23:36:01Z

reverting to this set of tests
and applying workaround for path

warren-bank · 2018-01-26T23:55:13Z

2nd iteration of passing unit tests

reverts tests to 534014e
applies same methodology to workaround the problem with re-assignment of attributes to path

observation:

this diff illustrates the workaround methodology

warren-bank · 2018-01-27T00:14:07Z

feels done to me..
what do you think?

SimenB

Thanks!

Sorry it took so long writing the tests for it 🙁

SimenB · 2018-01-27T09:44:06Z

@warren-bank I've pushed an update to the illustrating how to use resetModules and doMock to simplify the tests.

I'm sorry I wasn't clearer in my original comment, and that it meant you spent some time needlessly fighting the path module.

doMock docs, contains some prose around it.

Thank you so much for the fix though, looks really clean.

warren-bank · 2018-01-27T09:54:49Z

no worries.. I'm glad to contribute and if I can learn something new in the process, then all the better. I'm interested to see how you did it.

warren-bank · 2018-01-27T10:23:45Z

that's very interesting..

so you..

jest.doMock('path', foo)
- where foo is a function that returns an Object to mock path
Resolver = require('../')
- where Resolver is scoped at a higher level
- the CommonJS module had already been imported
  - I was under the impression that require() caches modules so a singleton is shared between all consumers
- the CommonJS module is imported again after the mock is created
  - somehow, require() returns a different Object which contains its own scoped closure
  - this scoped closure contains a reference to path
  - when this CommonJS module is being re-evaluated.. and require('path') is called.. and the return value is stored into its scoped closure.. this value is now the mock Object

observation:

jest.resetModules() is being called in beforeEach

question:

should jest.resetModules() be moved to afterEach?
- that way, the mock will be unset after the final test case

question:

does calling jest.resetModules() have any impact on userResolver, which is mocked at a higher scope?

question:

what happens to Resolver after this describe() block of tests?
- its value was replaced by each it() test
- the only difference between them is the value of the mocked Object
- after the mock is reset, does path within the Resolver scoped closure revert back to node's path module?

warren-bank · 2018-01-27T10:35:53Z

sorry.. I really need to stop asking dumb questions.. and just RTFM!

I hate it when people do that :)

SimenB · 2018-01-27T10:36:31Z

I was under the impression that require() caches modules so a singleton is shared between all consumers

jest.resetModules(); clear this cache.

should jest.resetModules(); be moved to afterEach?

It needs to happen before we re-require the module under test, but it should maybe be in beforeAll and afterEach instead of beforeEach.

what happens to Resolver after this describe() block of tests?

No magic happening - it's not cleaned up.

We could add a jest.unmock('path'); in afterAll in addition to a re-require of Resolver. This is not currently needed as it's the last test in the file (and every test file is completely isolated), but it should probably be done to avoid confusion if and when future tests are added in this file (such as the open #5403). Feel free to push another commit 🙂

I really need to stop asking dumb questions..

No worries!

warren-bank · 2018-01-27T10:46:05Z

ohhh.. now it's all making sense..

so jest.resetModules() is just calling require.cache={}
..it doesn't magically modify pre-existing references to imported modules
..it just allows modules to be re-imported so mocks will be used by them

I still need to read the manual.. but yeah.. that's making sense to me. Thanks!

warren-bank · 2018-01-27T10:58:20Z

would it be fair to assume that the way jest is creating a mock:
jest.doMock('path', foo)

would look something like:
require.cache[require.resolve('path')] = foo()

SimenB · 2018-01-27T11:02:28Z

Conceptually, yeah. Jest implements require on its own, so require.cache is not populated or messed with, but as a mental model it's close enough.

warren-bank · 2018-01-27T11:06:49Z

cool... thanks again for the lesson.
I'm literally brand new to using mocks in jest (obviously).. so I really do appreciate the info.

ok.. I'm calling it a night :)

SimenB · 2018-01-27T12:29:23Z

Pushed the additional cleanup

SimenB · 2018-01-27T22:58:46Z

@warren-bank mind rebasing?

warren-bank · 2018-01-27T23:14:48Z

not at all..

warren-bank · 2018-01-28T00:10:01Z

oops..

I don't have much (any) experience using pull requests on github..
didn't realize that the following series of commands would close the pull request..

git clone "https://github.com/warren-bank/jest.git"
cd jest

git checkout -b "issue-5396"
git push -u origin "issue-5396"

git checkout master
git reset --hard "c243f67efbae70bd416a61b0c899485bde3a702b"

git remote add facebook https://github.com/facebook/jest.git
git pull --ff-only facebook master
git push -f origin master

git checkout -b "issue-5396-final"
# merge HEAD of "issue-5396" into "master"
git add --all .
git commit -m 'fix #5396: preserve absolute paths in `moduleDirectories`'
git push -u origin "issue-5396-final"

git checkout master
git merge "issue-5396-final"
git push -u origin master

I'll make a new pull request..
this time I'll use a branch ("issue-5396-final")..
so pulling from your repo will be simpler.

warren-bank · 2018-01-28T00:14:21Z

oh.. re-opening it works too

warren-bank · 2018-01-28T00:26:56Z

hope you don't mind..
I refactored the before/after logic for how path is mocked.

Figured I'd leave the higher scope Resolver Object alone..
and just store the per-test instances into locally scoped variables.
..less cleanup needed

just waiting to see if all the validation checks pass..

warren-bank · 2018-01-28T00:50:15Z

made another change..
since we're mocking path in each test case..
may as well store a reference to the mock and use it..
which simplifies the code (fewer things to remember)

warren-bank · 2018-01-28T01:25:32Z

cool..
sorry for the workflow..
but it's all set for you now :)

warren-bank · 2018-01-28T01:57:09Z

hmm.. that's weird

my final commit was a minor tweak to the CHANGELOG..
when it re-ran the test suite, something unrelated to the changes made in the pull request timed out.

is there a way for you to manually re-run the suite?
..or should I just --amend the last commit and hope that triggers it?

update: i pushed an amended commit.. it worked.. the tests are running now..

update: all good..

github-actions · 2021-05-12T22:07:33Z

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.

facebook-github-bot added the CLA Signed ✔️ label Jan 26, 2018

SimenB requested changes Jan 26, 2018

View reviewed changes

SimenB reviewed Jan 26, 2018

View reviewed changes

warren-bank mentioned this pull request Jan 27, 2018

absolute paths in moduleDirectories are invalid in Windows OS #5396

Closed

SimenB approved these changes Jan 27, 2018

View reviewed changes

SimenB force-pushed the master branch from 6bbfdd3 to 6148662 Compare January 27, 2018 12:30

SimenB force-pushed the master branch from 6148662 to eb6f2e0 Compare January 27, 2018 12:31

warren-bank closed this Jan 27, 2018

warren-bank force-pushed the master branch from eb6f2e0 to 0110101 Compare January 27, 2018 23:34

fix jestjs#5396: preserve absolute paths in moduleDirectories

c2b74a7

warren-bank mentioned this pull request Jan 28, 2018

fix #5396: preserve absolute paths in moduleDirectories #5408

Closed

warren-bank reopened this Jan 28, 2018

refactor unit tests

28de870

warren-bank added 2 commits January 27, 2018 16:28

pretty

7b3db12

refactor: use mock

a8ee87d

bug fix

139ffd8

CHANGELOG

af8edc2

warren-bank force-pushed the master branch from 62a7270 to af8edc2 Compare January 28, 2018 02:00

warren-bank added 2 commits January 27, 2018 18:28

adjust sample data in unit tests

6a5e1a2

adjust sample data in unit tests

2276746

warren-bank force-pushed the master branch from 5ba25b0 to 2276746 Compare January 28, 2018 02:56

SimenB approved these changes Jan 28, 2018

View reviewed changes

Merge branch 'master' into meep

f7f91bb

cpojer merged commit 1f6e35c into jestjs:master Jan 29, 2018

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

fix: absolute paths in moduleDirectories are invalid in Windows OS #5398

fix: absolute paths in moduleDirectories are invalid in Windows OS #5398

Conversation

warren-bank commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

codecov-io commented Jan 26, 2018 • edited Loading

Codecov Report

warren-bank commented Jan 26, 2018

facebook-github-bot commented Jan 26, 2018

SimenB left a comment

Choose a reason for hiding this comment

SimenB Jan 26, 2018

Choose a reason for hiding this comment

warren-bank commented Jan 26, 2018 • edited Loading

SimenB commented Jan 26, 2018

warren-bank commented Jan 26, 2018

warren-bank commented Jan 26, 2018

warren-bank commented Jan 26, 2018

warren-bank commented Jan 26, 2018 • edited Loading

observation:

warren-bank commented Jan 26, 2018

warren-bank commented Jan 26, 2018

warren-bank commented Jan 26, 2018 • edited Loading

observation:

warren-bank commented Jan 27, 2018

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Jan 27, 2018 • edited Loading

warren-bank commented Jan 27, 2018

warren-bank commented Jan 27, 2018 • edited Loading

warren-bank commented Jan 27, 2018

SimenB commented Jan 27, 2018 • edited Loading

warren-bank commented Jan 27, 2018 • edited Loading

warren-bank commented Jan 27, 2018 • edited Loading

SimenB commented Jan 27, 2018

warren-bank commented Jan 27, 2018 • edited Loading

SimenB commented Jan 27, 2018

SimenB commented Jan 27, 2018

warren-bank commented Jan 27, 2018

warren-bank commented Jan 28, 2018

warren-bank commented Jan 28, 2018

warren-bank commented Jan 28, 2018

warren-bank commented Jan 28, 2018

warren-bank commented Jan 28, 2018

warren-bank commented Jan 28, 2018 • edited Loading

github-actions bot commented May 12, 2021

fix: absolute paths in `moduleDirectories` are invalid in Windows OS #5398

fix: absolute paths in `moduleDirectories` are invalid in Windows OS #5398

codecov-io commented Jan 26, 2018 •

edited

Loading

warren-bank commented Jan 26, 2018 •

edited

Loading

warren-bank commented Jan 26, 2018 •

edited

Loading

warren-bank commented Jan 26, 2018 •

edited

Loading

SimenB commented Jan 27, 2018 •

edited

Loading

warren-bank commented Jan 27, 2018 •

edited

Loading

SimenB commented Jan 27, 2018 •

edited

Loading

warren-bank commented Jan 27, 2018 •

edited

Loading

warren-bank commented Jan 27, 2018 •

edited

Loading

warren-bank commented Jan 27, 2018 •

edited

Loading

warren-bank commented Jan 28, 2018 •

edited

Loading