Add renderer option to storyshots #2414

tobilen · 2017-12-01T18:01:49Z

What I did

Added two options to the options passed to initStoryshots, renderer and serializer. Their usage is documented in the readme, but in short, you can pass custom renderer and serializer functions to the storyshot addon.
This is probably a bit easier to use than the test option, which requires you to pass an entire snapshotting test.

How to test

In addition to the storyshot tests currently in the addon, i added one where enzymes mount renderer and the serializer from enzyme-to-json are passed.

Does this need a new example in the kitchen sink apps?
No

Does this need an update to the documentation?
Yes, which has been added.

codecov · 2017-12-01T18:39:14Z

Codecov Report

Merging #2414 into release/3.3 will increase coverage by 0.05%.
The diff coverage is 58.33%.

@@               Coverage Diff               @@
##           release/3.3    #2414      +/-   ##
===============================================
+ Coverage        18.72%   18.77%   +0.05%     
===============================================
  Files              348      348              
  Lines             8198     8202       +4     
  Branches           906      917      +11     
===============================================
+ Hits              1535     1540       +5     
+ Misses            5995     5932      -63     
- Partials           668      730      +62

Impacted Files	Coverage Δ
addons/storyshots/src/index.js	`80% <100%> (+0.4%)`	⬆️
addons/storyshots/src/test-bodies.js	`61.9% <37.5%> (+11.9%)`	⬆️
app/react/src/server/utils.js	`0% <0%> (-53.58%)`	⬇️
lib/components/src/table/table.js	`0% <0%> (ø)`	⬆️
addons/a11y/src/components/Report/Rules.js	`0% <0%> (ø)`	⬆️
...ddons/actions/src/containers/ActionLogger/index.js	`2.89% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/configs/handle_routing.js	`24.21% <0%> (ø)`	⬆️
addons/knobs/src/components/PropForm.js	`9.3% <0%> (ø)`	⬆️
lib/ui/src/modules/api/actions/api.js	`51.85% <0%> (ø)`	⬆️
.../ui/src/modules/ui/components/addon_panel/index.js	`34.78% <0%> (ø)`	⬆️
... and 52 more

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 02b566a...11800f8. Read the comment docs.

Hypnosphi · 2017-12-01T21:36:56Z

addons/storyshots/.babelrc

@@ -1,5 +1,5 @@
 {
-  "presets": ["env", "react"],
+  "presets": ["env", "stage-0", "react"],


Why is it needed? Can we include the particular transform instead?

for the destructuring assignment here: https://github.com/storybooks/storybook/pull/2414/files#diff-75fdf0f2c6757366489b97c6898ea585R6

i would have used stage-2, but storybook itself pulls in stage-0, so i went with that to stay consistent

Oh I see. I wonder if we can remove this nested .babelrc to avoid confusion

@ndelangen what do you think?

Hypnosphi · 2017-12-01T21:41:12Z

Looks great! Let's use this feature for storyshots of example/cra-kitchen-sink

tobilen

i've extended the demo project to use it. i went for jest configuration in package.json over initializing everything in the initSnapshot file, since that is the cleaner way to do it imo

tobilen · 2017-12-01T22:23:07Z

addons/storyshots/.babelrc

@@ -1,5 +1,5 @@
 {
-  "presets": ["env", "react"],
+  "presets": ["env", "stage-0", "react"],


for the destructuring assignment here: https://github.com/storybooks/storybook/pull/2414/files#diff-75fdf0f2c6757366489b97c6898ea585R6

i would have used stage-2, but storybook itself pulls in stage-0, so i went with that to stay consistent

Hypnosphi · 2017-12-02T10:13:06Z

examples/cra-kitchen-sink/src/enzyme.js

+import Enzyme from 'enzyme';
+import Adapter from 'enzyme-adapter-react-16';
+
+Enzyme.configure({ adapter: new Adapter() });


We have this setup on root level, no need to duplicate it here

same as #2414 (comment)

Hypnosphi · 2017-12-02T10:15:07Z

examples/cra-kitchen-sink/package.json

@@ -39,6 +39,9 @@
    "@storybook/components": "^3.3.0-alpha.4",
    "@storybook/react": "^3.3.0-alpha.4",
    "babel-jest": "^21.2.0",
+    "enzyme": "^3.2.0",


We need to have a single enzyme instance across the codebase, so it shouldn’t be added to package dependencies

i know. but i figured an "example project" should include the necessary dependencies? its a lot more difficult to understand if i need to look outside of the project for its code to work. thats why i included enzyme setup there

OK, makes sense

Hypnosphi · 2017-12-02T10:21:26Z

examples/cra-kitchen-sink/src/storyshots.test.js

-  test: multiSnapshotWithOptions({
-    createNodeMock,
-  }),
+  renderer,


Can we support custom renderers for multisnapshots as well?

yeah sure. done

Looks like it still stores all the snapshots in one file

it does. i'm currently debugging it. looks like it did not work in the first place. the multisnapshot function doesnt seem to be called correctly

should work now. we cant use enzymes mount since that renders out the context prop (includes the file path, which is different on every machine). We also need to pass a serializer since jest-specific-snapshots doesnt use the projects serializer

mount includes context props, which means current file paths will be printed to snapshots. those are machine-dependant and differ on every enviroment. we dont want that.

Hypnosphi · 2017-12-02T15:45:51Z

examples/cra-kitchen-sink/src/stories/__snapshots__/addon-a11y.stories.storyshot

-  onClick={[Function]}
-  style={Object {}}
->
+<button>


Do you have an idea why Enzyme strips attributes?

its the renderer specified in the tests. you have shallow which shallowrenders, mount which more or less renders the entire vdom, and render (uses cheerio under the hood) which just prints out html. see https://github.com/airbnb/enzyme/blob/master/docs/api/render.md

like i said, we cant use mount with multisnapshot because the snapshots then include the context prop, and part of that is the file path. and the file path changes depending on what the absolute path on the system the snapshot is stored at is.

im not very familiar with the internals of jest-specific-snapshots, but if there was a way for it to pass a relative path in the context, that would eliminate the problem. its where i see the solution to the problem at least.
i wouldnt want to modify the output of the renderer (like replacing the filepath with an empty string or somesuch), which is the only alternative i can think of

tobilen added 7 commits December 1, 2017 16:17

Add option in snapshot generator to use renderer from options if passed

50a4b06

Use snapshot with options as default, pass on renderer to test-bodies

5ddb6b5

Add option to pass serializer to storyshots

8143264

Add enzyme-to-json for serializing, initStoryshots with custom renderer

b4c7098

Extend readme

f42b29a

Move enzyme-to-json dependency to storyshots, split initStoryshots tests

2b2b27d

Remove react-test-renderer storyshot snapshots, since they dont work yet

45755f9

Hypnosphi self-assigned this Dec 1, 2017

Hypnosphi self-requested a review December 1, 2017 21:35

Hypnosphi added addon: storyshots feature request labels Dec 1, 2017

Hypnosphi reviewed Dec 1, 2017

View reviewed changes

tobilen added 3 commits December 1, 2017 23:31

Add dependencies necessary to use enzyme in jest

beda473

Use storyshots renderer option to use enzyme

c78e169

Use storyshots renderer option to use enzyme, add snapshots to project

e3c2fc0

tobilen commented Dec 1, 2017

View reviewed changes

Hypnosphi requested changes Dec 2, 2017

View reviewed changes

Hypnosphi reviewed Dec 2, 2017

View reviewed changes

tobilen added 4 commits December 2, 2017 15:09

Use multisnapshots in test project

e762993

Remove stored snapshots, since multi snapshots are different on the CI

9b1e319

Use enzymes render renderer.

3bbbac5

mount includes context props, which means current file paths will be printed to snapshots. those are machine-dependant and differ on every enviroment. we dont want that.

Correctly use multiSnapshotTest in example project

71421d8

Hypnosphi reviewed Dec 2, 2017

View reviewed changes

Merge branch 'release/3.3' into add-renderer-option-to-storyshots

11800f8

Hypnosphi approved these changes Dec 2, 2017

View reviewed changes

Hypnosphi merged commit 16cd900 into storybookjs:release/3.3 Dec 2, 2017

igor-dv mentioned this pull request Dec 8, 2017

887 Generate snapshot per story file #1584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add renderer option to storyshots #2414

Add renderer option to storyshots #2414

tobilen commented Dec 1, 2017

codecov bot commented Dec 1, 2017 •

edited

Loading

Hypnosphi Dec 1, 2017

tobilen Dec 1, 2017

Hypnosphi Dec 2, 2017

Hypnosphi commented Dec 1, 2017

tobilen left a comment

tobilen Dec 1, 2017

Hypnosphi Dec 2, 2017

tobilen Dec 2, 2017

Hypnosphi Dec 2, 2017

tobilen Dec 2, 2017

Hypnosphi Dec 2, 2017

Hypnosphi Dec 2, 2017

tobilen Dec 2, 2017

Hypnosphi Dec 2, 2017

tobilen Dec 2, 2017 •

edited

Loading

tobilen Dec 2, 2017

Hypnosphi Dec 2, 2017

tobilen Dec 2, 2017 •

edited

Loading

tobilen Dec 2, 2017

Add renderer option to storyshots #2414

Add renderer option to storyshots #2414

Conversation

tobilen commented Dec 1, 2017

What I did

How to test

codecov bot commented Dec 1, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Dec 1, 2017

tobilen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobilen Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobilen Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2017 •

edited

Loading

tobilen Dec 2, 2017 •

edited

Loading

tobilen Dec 2, 2017 •

edited

Loading