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: absolute paths in moduleDirectories are invalid in Windows OS #5398

Merged
merged 9 commits into from
Jan 29, 2018

Conversation

warren-bank
Copy link
Contributor

closes: Issue #5396

@facebook-github-bot
Copy link
Contributor

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
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

CLA (Contributor License Agreement) has now been signed.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

warren-bank commented Jan 26, 2018

@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
Copy link
Member

SimenB commented Jan 26, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

warren-bank commented Jan 26, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

  • reverting to this set of tests
  • and applying workaround for path

@warren-bank
Copy link
Contributor Author

warren-bank commented Jan 26, 2018

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
Copy link
Contributor Author

feels done to me..
what do you think?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sorry it took so long writing the tests for it 🙁

@SimenB
Copy link
Member

SimenB commented Jan 27, 2018

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

warren-bank commented Jan 27, 2018

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
Copy link
Contributor Author

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

I hate it when people do that :)

@SimenB
Copy link
Member

SimenB commented Jan 27, 2018

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
Copy link
Contributor Author

warren-bank commented Jan 27, 2018

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
Copy link
Contributor Author

warren-bank commented Jan 27, 2018

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
Copy link
Member

SimenB commented Jan 27, 2018

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
Copy link
Contributor Author

warren-bank commented Jan 27, 2018

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
Copy link
Member

SimenB commented Jan 27, 2018

Pushed the additional cleanup

@SimenB
Copy link
Member

SimenB commented Jan 27, 2018

@warren-bank mind rebasing?

@warren-bank
Copy link
Contributor Author

not at all..

@warren-bank
Copy link
Contributor Author

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
Copy link
Contributor Author

oh.. re-opening it works too

@warren-bank
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@warren-bank
Copy link
Contributor Author

warren-bank commented Jan 28, 2018

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

5 participants