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

toMatchInlineSnapshot fails if prettier's native module deps are mocked through jest.mock #6702

Closed
tryggvigy opened this issue Jul 16, 2018 · 12 comments Β· Fixed by #6776
Closed

Comments

@tryggvigy
Copy link
Contributor

πŸ› Bug Report

toMatchInlineSnapshot fails if path module is mocked

jest.mock('path', () => ({}));

test('foo', () => {
  expect({}).toMatchInlineSnapshot();
});

results in

● Test suite failed to run

    TypeError: path.resolve is not a function

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: macOS High Sierra 10.13.5
    CPU: x64 Intel(R) Core(TM) i7-6567U CPU @ 3.30GHz
  Binaries:
    Node: 8.9.4 - ~/.nvm/versions/node/v8.9.4/bin/node
    Yarn: 1.7.0 - /usr/local/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v8.9.4/bin/npm
  npmPackages:
    jest: ^23.4.0 => 23.4.0
@SimenB
Copy link
Member

SimenB commented Jul 17, 2018

I wonder if this might be fixed by #6687?

@aaronabramov wdyt?

EDIT: Hmm, no. I tried applying that patch locally, and it still fails πŸ™

@aaronabramov
Copy link
Contributor

no, this seems like a separate issue, but comes from the same bug.
right now we're trying to do too much from within the VM. we need to take absolutely everything out of there except the snapshot state object at some point

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Jul 24, 2018

I'm trying to dig into this. I've got a test that reproduces the bug in e2e/__tests__/to_match_inline_snapshot.test.js.

The error is coming from the compiled node_modules/prettier/third-party.js and originally from this line in cosmiconfig dependency. Thats the code thats trying to run when TypeError: path.resolve is not a function happens.


node_modules/prettier/prettier-bin.js is right now using these native modules

var path = _interopDefault(require('path'));
var os = _interopDefault(require('os'));
var assert = _interopDefault(require('assert'));
var fs = _interopDefault(require('fs'));
var util = _interopDefault(require('util'));
var events = _interopDefault(require('events'));
var thirdParty = require('./third-party');
var thirdParty__default = thirdParty['default'];
var readline = _interopDefault(require('readline'));
// and these two through ./third-party
var stream = _interopDefault(require('stream'));
var module$1 = _interopDefault(require('module'));

I'm assuming jest.mock will overrider any of the above. I'm unsure how to actually fix this issue (looking at #6687 for inspiration) but I'll keep digging. I'd appreciate any input from folks with more context.

update jest.mock only causes failure on some of them

mock path -- TypeError: path.resolve is not a function
mock fs -- TypeError: fs.statSync is not a function
mock os -- TypeError: os.homedir is not a function
mock assert -- TypeError: assert is not a function
mock utils -- passes
mock readline -- passes
mock stream -- passes
mock module -- passes

@tryggvigy tryggvigy changed the title toMatchInlineSnapshot fails if path module is mocked toMatchInlineSnapshot fails if prettier native module dependencies are mocked through jest.mock Jul 24, 2018
@tryggvigy tryggvigy changed the title toMatchInlineSnapshot fails if prettier native module dependencies are mocked through jest.mock toMatchInlineSnapshot fails if prettier's native module dependencies are mocked through jest.mock Jul 24, 2018
@tryggvigy tryggvigy changed the title toMatchInlineSnapshot fails if prettier's native module dependencies are mocked through jest.mock toMatchInlineSnapshot fails if prettier's native module deps are mocked through jest.mock Jul 24, 2018
@aaronabramov
Copy link
Contributor

@tryggvigy hey! the issue comes from the fact that we have two environments we execute Jest code in:

  1. Outer environment, where all framework code is running
  2. Test environment inside a VM https://github.com/facebook/jest/blob/master/packages/jest-environment-node/src/index.js#L94

because most of our framework code is in the outer environment we're able to mock native/whatever modules from inside the VM and not affect the framework code.

unfortunately inline snapshots bring a lot of logic that is executed in the inner scope (inside the VM) and interacting with the environment makes it possible to break framework code.

the right fix for this is to take everything framework related to the outer scope and pass a pure/stateless function to the VM

@aaronabramov
Copy link
Contributor

@tryggvigy actually! since you found that the issue comes from inside prettier my PR might have fixed it (cause i did take prettier to the outer scope)
do you mind checking out master and trying to reproduce it?

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Jul 24, 2018

Thanks for explaining @aaronabramov! Also, unfortunately yes I can reproduce this on master.

Here is the test I added to e2e/__tests__/to_match_inline_snapshot.test.js and reproduces the bug on latest master.

test('fails', () => {
  const filename = 'mockFail.test.js';
  const test = `
    jest.mock('path', () => ({}));
    test('hæhæ!', () => {
      expect({}).toMatchInlineSnapshot();
    });
  `;

  writeFiles(TESTS_DIR, {[filename]: test});
  const {stderr, status} = runJest(DIR, ['-w=1', '--ci=false', filename]);
  expect(stderr).toMatch('1 snapshot written from 1 test suite.');
  expect(status).toBe(0);
});

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Jul 27, 2018

I noticed that this is not an issue in jest-circus.
When run with JEST_CIRCUS=1, this passes.

@aaronabramov
Copy link
Contributor

ugh.. that might be my fault :)
i did use require for babel-traverse, but kept localRequire for prettier
https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/setup_jest_globals.js#L105
i'm pretty sure that's the reason

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Jul 29, 2018

Yeah, thats it :) Test passes now, I'll make a PR

#6776

@tryggvigy
Copy link
Contributor Author

tryggvigy commented Jul 29, 2018

Closing the issue. It's fixed once #6776 is merged

thymikee pushed a commit that referenced this issue Jul 30, 2018
## Summary
This PR fixes #6702 by seizing to local require prettier in jest-jasmine2. Instead `require(config.prettierPath)` is used. This prevents the issue where mocking native modules like `path` and `fs` that prettier depends on would cause `toMatchInlineSnapshot` to fail.

## Test plan
Run the new unit test introduced in this PR.
@aaronabramov
Copy link
Contributor

@tryggvigy thanks for digging into it! :)

@github-actions
Copy link

This issue 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 a pull request may close this issue.

3 participants