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

Allow passing options to react-test-renderer .create() method #896

Closed
wants to merge 16 commits into from

Conversation

jrdrg
Copy link

@jrdrg jrdrg commented Apr 15, 2017

Issue:
When running storyshots with a story for a component that uses refs, it will throw a null reference error (see facebook/react#7371)

What I did

This PR adds the ability to pass options to the react-test-renderer create method, for example to allow mocking refs as per https://facebook.github.io/react/blog/2016/11/16/react-v15.4.0.html#mocking-refs-for-snapshot-testing

How to test

Run yarn test - ComponentWithRef.stories.js should use the createNodeMock function defined in storyshots.test.js to mock its ref and not fail with a null reference error. The story for ComponentWithRef attempts to get the scrollWidth of a div when the component mounts.

@jrdrg
Copy link
Author

jrdrg commented Apr 15, 2017

@shilman - reopening storybook-eol/storyshots#76 here

@jrdrg jrdrg force-pushed the test-renderer-options branch 5 times, most recently from b083f2a to f27db8b Compare April 17, 2017 15:13
@JaKXz
Copy link

JaKXz commented Apr 26, 2017

@jrdrg thanks for doing this work! Is there anything I can do to help get this merged? :)

I've been trying to run this branch locally but not having any luck with installing dependencies (using the latest versions of yarn, node etc) and getting the tests to run / pass.

@jrdrg
Copy link
Author

jrdrg commented Apr 26, 2017

@JaKXz I'm seeing this when running the tests from the storybook root dir after doing an install via lerna, which seems to be why the build is failing.

Summary of all failing tests
 FAIL  packages/storyshots/stories/__test__/storyshots.test.js
  ● Storyshots › Component with ref › on mount

    TypeError: Cannot read property 'emit' of null

If I yarn install directly in the storyshots directory and then run jest from there, it works fine - from some other github issues elsewhere I assume this has to do with storybook-addons somehow but haven't really gotten a chance to dig into it super deeply yet.

@theinterned
Copy link
Member

I wanted to get a PR in for this issue, but I can't run tests in the StoryShots package in the mono repo, so I made a working PR for the deprecated StoryShots repo instead:

storybook-eol/storyshots#98

@jrdrg jrdrg force-pushed the test-renderer-options branch 3 times, most recently from 0437617 to 072f7ed Compare April 28, 2017 17:39
@codecov
Copy link

codecov bot commented Apr 28, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@3732bcd). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #896   +/-   ##
=======================================
  Coverage          ?     0%           
=======================================
  Files             ?      2           
  Lines             ?     16           
  Branches          ?      3           
=======================================
  Hits              ?      0           
  Misses            ?     13           
  Partials          ?      3
Impacted Files Coverage Δ
.../stories/required_with_context/ComponentWithRef.js 0% <0%> (ø)
.../required_with_context/ComponentWithRef.stories.js 0% <0%> (ø)

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 3732bcd...136d62d. Read the comment docs.

@jrdrg
Copy link
Author

jrdrg commented Apr 28, 2017

@JaKXz @ndelangen @theinterned I fixed up the stuff that was preventing the tests from passing, looks like the build succeeded this time

.jestrc Outdated
@@ -3,7 +3,8 @@
"clearMocks": true,
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/__mocks__/fileMock.js",
"\\.(css|scss)$": "<rootDir>/__mocks__/styleMock.js"
"\\.(css|scss)$": "<rootDir>/__mocks__/styleMock.js",
"@kadira/storybook-addons": "<rootDir>/packages/addons/dist/index.js"
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

Copy link
Author

Choose a reason for hiding this comment

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

I was running into this issue storybook-eol/storybook-addons#1 when trying to run yarn test from the storybook root directory. So I added some console.logs while debugging and determined that this constructor https://github.com/storybooks/storybook/blob/master/packages/addons/src/index.js#L2 was getting called 3 times during the storyshots test, when it seemed to me it should be a singleton. Running the tests in the package itself (i.e. not using lerna and doing a yarn install directly from /packages/storyshots) worked fine, so I figured it might have something to do with jest resolving the symlinks so adding this line makes sure that all references to storybook-addons use the same absolute path.

@@ -0,0 +1,20 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

why are there 2 pretty much identical .jestrc files?

Copy link
Author

Choose a reason for hiding this comment

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

The second .jestrc file is for the test-examples script. Running yarn test uses the .jestrc in the root directory, but yarn test-examples uses lerna to run npm test in each package's directory. If I used "test": "jest --config ../../.jestrc" in the storyshots directory to reference the jestrc in the root, I ran into this error:

lerna ERR! test Errored while running script in 'storyshots'
lerna ERR! execute Error: Command failed: npm run test
lerna ERR! execute ● Validation Error:
lerna ERR! execute
lerna ERR! execute   Module ./node_modules/jest-enzyme/lib/index.js in the setupTestFrameworkScriptFile option was not found.
lerna ERR! execute
lerna ERR! execute   Configuration Documentation:
lerna ERR! execute   https://facebook.github.io/jest/docs/configuration.html

The rootDir is also different for yarn test and yarn test-examples, so I copied the .jestrc from the root dir here and made a few minor changes to the paths so I could do "test": "jest --config ./.jestrc" in the storyshots package.json.

Copy link
Author

Choose a reason for hiding this comment

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

@ndelangen is there a better way to handle this?

Copy link

Choose a reason for hiding this comment

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

I think Jest v20 takes care of a lot of the multi-project test running stuff.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like test-examples was removed, so I removed this extra .jestrc

@@ -14,10 +14,13 @@ const babel = require('babel-core');
const pkg = readPkgUp.sync().pkg;
const isStorybook =
(pkg.devDependencies && pkg.devDependencies['@kadira/storybook']) ||
Copy link
Member

Choose a reason for hiding this comment

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

Looks like, this could be extracted into a function?

@jrdrg jrdrg force-pushed the test-renderer-options branch 3 times, most recently from 8bd1551 to 93bfda3 Compare May 11, 2017 20:44
@jrdrg jrdrg force-pushed the test-renderer-options branch from 9fa5a1d to 01f480a Compare May 18, 2017 18:24
@jrdrg jrdrg force-pushed the test-renderer-options branch from ad6fdf2 to bd7b71f Compare May 18, 2017 18:43
@jrdrg jrdrg force-pushed the test-renderer-options branch from e276422 to 7156191 Compare May 18, 2017 19:09
@ndelangen
Copy link
Member

Ouch this is taking long, sorry!, I can see you're taking the effort to get this merged.

@ndelangen ndelangen mentioned this pull request May 21, 2017
@ndelangen
Copy link
Member

@jrdrg I can't push to your fork, so to rebase I had to push a branch to our repo and create a new PR: #1085

Thanks for the work! I'll ask @tmeasday to review.

@ndelangen ndelangen closed this May 21, 2017
tmeasday added a commit that referenced this pull request May 22, 2017
See for instance #896 -- it's a common problem to need mocked nodes. 

This solution is probably a bit of a bandaid, as large projects would probably want to set options per-story, this is a lot better than nothing in the meantime.
ndelangen pushed a commit that referenced this pull request May 24, 2017
See for instance #896 -- it's a common problem to need mocked nodes. 

This solution is probably a bit of a bandaid, as large projects would probably want to set options per-story, this is a lot better than nothing in the meantime.
@shilman shilman added the misc label May 27, 2017
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

Successfully merging this pull request may close these issues.

5 participants