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

React/Jest Mocking issue with keymirror #5183

Closed
blainekasten opened this issue Oct 15, 2015 · 15 comments
Closed

React/Jest Mocking issue with keymirror #5183

blainekasten opened this issue Oct 15, 2015 · 15 comments

Comments

@blainekasten
Copy link
Contributor

I've ran into an issue with 0.14 when testing with jest & npm 3. Ultimately I've realized that since npm 3 installs everything as siblings, packages that react depends on will still get automatically mocked and therefore break.

My example was an error like this:

● Runtime Error
TypeError: /Users/blaine.kasten/Sites/videochrome/src/ProgressBar/ProgressBarBackground/__tests__/ProgressBarBackground--tests.js: /Users/blaine.kasten/Sites/videochrome/node_modules/react/react.js: /Users/blaine.kasten/Sites/videochrome/node_modules/react/lib/React.js: /Users/blaine.kasten/Sites/videochrome/node_modules/react/lib/ReactDOM.js: /Users/blaine.kasten/Sites/videochrome/node_modules/react/lib/ReactDefaultInjection.js: /Users/blaine.kasten/Sites/videochrome/node_modules/react/lib/BeforeInputEventPlugin.js: Cannot read property 'topCompositionEnd' of undefined

Looking into it, in the lib/BeforeInputEventPlugin.js file, it referenced ./EventConstants, in that file it was using fbjs/lib/keymirror to create it's EventConstants. Since that was being mocked, the return from ./EventConstants.js was undefined. Thus causing an error.

This might be part of a bigger problem with jest and supporting npm3. I'm not sure if this issue should live in their repo. Or more immediately react could not depend on fbjs and just use it's own keymirror implementation.

@zpao
Copy link
Member

zpao commented Oct 15, 2015

Yea, this automocking stuff is now more annoying… yayyyy. I tend to have more issues with automocking than gains from not using it, so my first move is pretty much always to just disable it for everything. This isn't going to be React specific - anybody who had disabled mocking for a dependency with dependencies will have problems - but will definitely be part of the story of testing React. It's probably worth bringing up in the jest repo so it's on people's radar. I'll bug @cpojer about it too :P

Or more immediately react could not depend on fbjs and just use it's own keymirror implementation.

keyMirror is only 1 of the things that got moved to fbjs. We'd have to move a bunch of things back into the react repo and that's really unlikely.

@cpojer
Copy link
Contributor

cpojer commented Oct 15, 2015

Can we just kill keyMirror? We don't need it at all.

@zpao
Copy link
Member

zpao commented Oct 15, 2015

That can be a separate discussion. keyMirror is a symptom here but the root of this issue is jest + npm.

@blainekasten
Copy link
Contributor Author

This is getting really bad. I ran into this again (sort of my own fault). I set my unmock to "fbjs/lib/keymirror" which I should have just done "fbjs".

This just popped up again with a deep issue:

In react/lib/Danger you required fbjs/lib/createNodesFromMarkup which is called on L89 then looped over. Unfortunately the mock made that function return undefined, which errored on the .length in L92.

So, @cpojer like @zpao said, there are symptoms here. But this is seeming to be a deep jest issue. Should I migrate this issue to the jest repo? do you have one already?

@cpojer
Copy link
Contributor

cpojer commented Oct 15, 2015

I agree this situation is really annoying. I don't have a good solution for this yet and I would recommend for the React team to provide a list of modules that should not be mocked until we come up with something good. Sorry for the trouble.

@zpao
Copy link
Member

zpao commented Oct 15, 2015

Yea, we should make this a jest issue but with a React specific work-around - unmock react and fbjs, and hopefully don't use any dependencies of fbjs. Basically anytime you unmock something you need to walk down it's deps and unmock all of those or you're in potential trouble.

@justinwyllie
Copy link

Is there any kind of a resolution for this?
I am using: npm 3.3.6 and Jest 0.7.1
I am trying to not mock react-bootstrap. My test fails with either 'TypeError' or 'SyntaxError'. I added some of the modules suggested in the comments above to unmockedModulePathPatterns in my package.json. i also followed through all the dependencies of react-bootstrap and unmocked them in my test. I had to upgrade node to support Jest and that installed npm 3.3.6.

@zpao
Copy link
Member

zpao commented Nov 17, 2015

@justinwyllie Personally I suggest either unmocking everything or downgrading to npm2. Unfortunately we don't have the time to play whack-a-mole.

@cpojer
Copy link
Contributor

cpojer commented Nov 17, 2015

This will be fixed in jest 0.8.0 or 0.8.1 towards the end of the year. I have a clear solution in mind but I need to make sure it will work out fine.

@cpojer
Copy link
Contributor

cpojer commented Nov 17, 2015

I recommend adding all of the modules from here: https://github.com/react-bootstrap/react-bootstrap/blob/master/package.json#L114 to your unmock patterns.

@blainekasten
Copy link
Contributor Author

Is this supposed to be fixed in jest-cli@0.9.0? @cpojer

@cpojer
Copy link
Contributor

cpojer commented Feb 19, 2016

Not yet! I have a fix that I need to add tests for and tests for performance. It will definitely be fixed with the final 0.9 though :)

@cpojer
Copy link
Contributor

cpojer commented Feb 22, 2016

I'll publish a new version of jest-cli@next (0.9 pre-release) that fixes this issue today.

@cpojer cpojer closed this as completed Feb 22, 2016
ghost pushed a commit to jestjs/jest that referenced this issue Feb 22, 2016
Summary:This fixes unmock resolution for node_modules when using npm3. The way this is solved is by checking whether the parent dependency is unmocked and both modules are within a `node_modules` folder.

This has taken a while, mainly because it required #599 to be merged. I also added some more caching that makes test runs even faster. The react-native test suite now completes in about 6s (down from 7.5-8s). There might be more in the near future ;)

This fixes #554, #730, facebook/react#5183, facebook/relay#832 and will be part of 0.9.0. I'll publish 0.9.0-fb3 today with this fix.
Closes #732

Differential Revision: D2959610

fb-gh-sync-id: c374b7a2bcdfddf768905356a08948d9156eb028
shipit-source-id: c374b7a2bcdfddf768905356a08948d9156eb028
@blainekasten
Copy link
Contributor Author

MAY THE GODS BE PRAISED

On Sun, Feb 21, 2016 at 6:18 PM, Christoph Pojer notifications@github.com
wrote:

I'll publish a new version of jest-cli@next (0.9 pre-release) that fixes
this issue today.


Reply to this email directly or view it on GitHub
#5183 (comment).

@cpojer
Copy link
Contributor

cpojer commented Mar 6, 2016

Jest 0.9.0 was published along with a complete overhaul of the website: facebook.github.io/jest/ This problem should not be an issue again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants