Add ability to use image snapshots to addon-storyshots #2413

thomasbertet · 2017-12-01T16:55:41Z

Issue: #1781

What I did

Ability to generate image snapshots from stories using addon-storyshots.

How to test

Start fresh & bootstrap everything
Go to cra-kitchen-sink
Run yarn run test-image-snapshots

Is this testable with jest or storyshots?

Not sure

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

Yep done.

Does this need an update to the documentation?

Maybe we should say something about it in the doc as it may persuade people to use storybook ? But I don't think there is a word about storyshots in the doc yet.

tobilen · 2017-12-01T18:09:18Z

just an fyi, since we added image-snapshot testing to our suites a couple of days ago: puppeteer will render text slightly different depending on the enviroment you run it on (especially mac vs linux based containers).

This means you either need to provide a big enough margin of error, or you need to record your snapshots locally from inside the container.

Either way, this change will mean that users who use container for their CI will need to modify those to include chrome. Definitely a breaking change here.

thomasbertet · 2017-12-04T08:51:20Z

@tobilen I do not understand why this is a breaking change as it just adds an option to generate image snapshots. If one is using the addon-storyshots, he must use this option to generate images and thus the concern about linux/mac is up to him to handle while doing this, isn't it ?

thomasbertet · 2017-12-05T09:14:48Z

@Hypnosphi could you give me a hint on what to do next to go on with this ?

thomasbertet · 2017-12-06T12:15:35Z

@Hypnosphi in the meantime, can you elaborate on why this is a breaking change ? 🤔

Hypnosphi · 2017-12-06T12:27:17Z

what to do next to go on with this

I'd like to hear from @ndelangen, he has set up visual tests for kitchen sink apps

tobilen · 2017-12-06T18:10:23Z

hey @thomasbertet i mistakenly assumed you'd want storyshots to do image snapshotting as a default. i now see that you're just providing a helper method. Sorry about that.

I would include something about that in the readme though.

danielduan · 2017-12-06T19:07:04Z

examples/cra-kitchen-sink/src/stories/image-diff.stories.js

+  </span>
+);
+
+storiesOf('Button - for-image-snapshot', module)


Can you please follow the current naming convention we have?

storiesOf('Addon Storyshots.Image Snapshot Button...')

I need a pattern to match to avoid generating all images.

WDYT about Addon Storyshots.Button.ImageSnapshot where ImageSnapshot is the pattern I'm matching ?

what's wrong with matching that entire string? is it because the entire kind name isn't available to you because of the hierarchy tree?

danielduan · 2017-12-06T19:10:08Z

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

+import path from 'path';
+
+initStoryshots({
+  suite: 'Image snapshots',


can you document all of these options and the possible inputs for these in README.md?

In fact I already documented inside the README the three params for the imageSnapshot function. (the function that you assign to the test param of initStoryshots.)

The "suite" option & all other initStoryshots params are already documented, nothing new from this PR.

danielduan · 2017-12-06T19:11:03Z

examples/cra-kitchen-sink/src/stories/image-diff.stories.js

+    )
+  );
+
+storiesOf('Some really long story kind description - for-image-snapshot', module)


please nest this under the previous Addon Storyshots section

danielduan · 2017-12-06T19:12:41Z

Thanks for the contribution, I'm pretty excited for this new feature!

igor-dv

To be honest. I feel a bit uncomfortable with adding this feature to StoryShots. Looks like it should be a separate plugin even from the dependencies' point of view, but I am not 100% sure about it (And I was not a part of the discussion at the beginning), so I don't want to be strict and picky.

igor-dv · 2017-12-07T07:05:54Z

addons/storyshots/src/index.js

@@ -16,6 +16,8 @@ export {
  snapshotWithOptions,
  shallowSnapshot,
  renderOnly,
+  imageSnapshot,
+  imageSnapshotWithOptions,


I think, since this is a new feature, you can provide just the imageSnapshot method with the relevant defaults, instead of extending the API with two methods. (IMO it's already confusing enough, and we need to start thinking of some refactoring here for the breaking version)

Fair enough, did it

igor-dv · 2017-12-07T07:14:01Z

addons/storyshots/src/test-bodies.js

@@ -39,3 +43,63 @@ export function renderOnly({ story, context }) {
  const storyElement = story.render(context);
  reactTestRenderer.create(storyElement);
 }
+
+export const imageSnapshotWithOptions = ({ port = 6006, host = 'localhost', scheme = 'http' }) => {


As I see there is no cohesion between other "bodies" in this module and the imageSnapshot (and it's a big one). I think worth extracting it into a separate file. index.js can just import it from there.

Ok, did it.

igor-dv · 2017-12-07T07:24:55Z

addons/storyshots/src/test-bodies.js

+    const baseUrl = `${scheme}://${host}:${port}`;
+    const encodedKind = encodeURIComponent(context.kind);
+    const encodedStoryName = encodeURIComponent(context.story);
+    const storyUrl = `/iframe.html?selectedKind=${encodedKind}&selectedStory=${encodedStoryName}`;


As I understand imageSnapshot won't work for RN. It makes this addon a bit inconsistent. This should be documented very clearly. You can even throw some error in case someone tries to run it for RN.

I added a message about it inside README & the test itself.

thomasbertet · 2017-12-07T09:53:28Z

Thanks for all your feedbacks ! I think I got every one of them dealt with. Please let me know :)

danielduan · 2017-12-07T21:49:50Z

We're gonna need to fix the unit test as well. Let me see if I can install Chrome on our CircleCI instance.

edit: just converted CI to use the docker image with browsers. we're gonna need to fix the test to serve storybook in the background.

codecov · 2017-12-07T21:56:48Z

Codecov Report

Merging #2413 into master will decrease coverage by 0.04%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master    #2413      +/-   ##
==========================================
- Coverage   34.35%   34.31%   -0.05%     
==========================================
  Files         390      391       +1     
  Lines        8772     8806      +34     
  Branches      903      899       -4     
==========================================
+ Hits         3014     3022       +8     
- Misses       5145     5187      +42     
+ Partials      613      597      -16

Impacted Files	Coverage Δ
addons/storyshots/src/test-body-image-snapshot.js	`7.69% <7.69%> (ø)`
addons/storyshots/src/index.js	`79.31% <80%> (-0.69%)`	⬇️
app/vue/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
addons/a11y/src/components/WrapStory.js	`72.09% <0%> (ø)`	⬆️
lib/components/src/table/cell.js	`65.21% <0%> (ø)`	⬆️
addons/links/src/react/components/link.js	`78.78% <0%> (ø)`	⬆️
addons/a11y/src/components/Report/index.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Array.js	`34.14% <0%> (ø)`	⬆️
...t-native/src/preview/components/StoryView/index.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/components/menu_item.js	`19.14% <0%> (ø)`	⬆️
... and 49 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 65a121f...797f2b7. Read the comment docs.

thomasbertet · 2017-12-08T13:42:36Z

@danielduan What we did internally at my company was just to "sleep" for some time before running the test. It sounds pretty hacky but hey it works ! 🍭

Our CicleCI config is as follow:

defaults: &defaults
  docker:
    - image: circleci/node:8.9.1-browsers
[...]
     - run:
          name: Storybook
          command: yarn storybook
          background: true
      - run:
          name: Sleep while running storybook
          command: sleep 60
      - run:
          name: Tests
          command: ./node_modules/.bin/lerna run test --stream

The last command runs the test which needs the storybook to be running.

Is this something we "could" imagine ? If you know a way to know when the storybook has launched, then we could possibly hook into this "event" ?

igor-dv · 2017-12-08T14:27:37Z

Why running it on ci? Why not just export the static version?

thomasbertet · 2017-12-08T15:43:31Z

@igor-dv what do you mean ? I'm not sure I get it.

danielduan · 2017-12-08T16:08:12Z

@igor-dv the CI is for running the test to make sure this does not regress.

@thomasbertet feel free to toy with the CircleCI settings on your branch to get it working.

Hypnosphi · 2017-12-08T16:33:24Z

I think @igor-dv meant that we can run build-storybook and then serve the output instead of running webpack dev server on CI

igor-dv · 2017-12-08T18:31:23Z

@Hypnosphi, exactly =)

Hypnosphi · 2017-12-11T11:52:58Z

Is there a way to exclude a story from screenshot testing, but still include in regular snapshots? This may be needed when using transitions and animations in components

thomasbertet · 2017-12-11T14:45:13Z

Yes you can use the same parameters as for other storyshots config. In fact, that's what I did, I created two storyshots instances, and each instance is running their proper stories.

Note that you cannot run within a single initStoryshot() the two snapshots mode (images & HTML).
If you wish to use image snapshot side-by-side with HTML snapshots, you need to call another initStoryshots() with the proper params (including regexp for name/kind exclusion/inclusion)

thomasbertet · 2017-12-11T14:47:04Z

@Hypnosphi any hint on how to start a node server to serve the static storybook on CircleCI ? Do you have any experience in this that can save me some time ?
Also, you would not accept this PR if I run the storybook in the CI using the webpack-dev-server, right ?
Thanks!

igor-dv · 2018-01-08T19:23:30Z

Is something missing here to go forward?

Hypnosphi · 2018-01-08T19:29:31Z

— Document what is needed to separate visual and regular tests in addons/storyshots/README.md

This, at least

thomasbertet · 2018-01-09T10:13:04Z

@Hypnosphi I'm not sure I follow. I mean there is nothing to do in particular to separate visual regressions (ie. image snapshots) from markup regressions.

The main issue I had was the integration with CRA, as it runs everything that follows the .test.js pattern,. At the time, I did not wanted them to be run automatically, because it requires you to build the static-storybook. This seemed to me a bit too long to be integrated as-is. That is why I had created a different approach in the CRA kitchen sink.

Now that everything is inside official-storybook, no need for two different approaches as we have full control over what test file is used.

I already updated the README of the storyshots addon to document everything image-snapshots related. I believe this is sufficient, WDYT ?

Please tell me in something is unclear. If you feel you need something else or not feel confident in something, please let me know.

Hypnosphi · 2018-01-09T10:16:51Z

there is nothing to do in particular to separate visual regressions (ie. image snapshots) from markup regressions.

Creating a separate jest project may seem obvious, but I wouldn't qualify it as "nothing"

integration with CRA

That's definitely worth documenting as well. There's quite a lot of CRA generated apps out there =)

thomasbertet · 2018-01-10T10:50:57Z

Well technically it's not required to create a separate Jest project, it eases the Jest configuration & especially for our use case with the test ran by /scripts/tests.js.

That said, I tried to document this as much as I can, please forgive my english, don't hesitate to correct what does not make sense / is not english-valid sentences :)

Thanks again for your help & patience on this, really appreciate it. Please let me know if anything needs more details!

Hypnosphi · 2018-01-10T11:45:50Z

Thanks @thomasbertet , great work!
Looks like there's only one small change left: #2413 (comment)

@danielduan Your concerns are addressed, aren't they?

thomasbertet · 2018-01-10T14:07:22Z

@Hypnosphi see my answer : #2413 (comment).

thomasbertet · 2018-01-11T13:44:38Z

So are we good ? Are we waiting for @danielduan check ❓

Hypnosphi · 2018-01-11T13:52:08Z

unit-test job does an unneeded work ("Build static React kitchen-sink"). Note that unit-test doesn't contain any image tests at all
https://circleci.com/gh/storybooks/storybook/32093?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Concerns seem to be addressed

thomasbertet · 2018-01-11T16:09:43Z

@Hypnosphi you were so right, sorry did not notice it before .. anyway thanks !

danielduan · 2018-01-11T17:41:31Z

My bad, been pretty busy recently. Looks good to me.

igor-dv · 2018-01-11T18:11:35Z

@thomasbertet 👏

ndelangen · 2018-01-11T19:37:46Z

👏

anescobar1991 · 2018-01-16T21:12:17Z

Should the css style testing portion of the docs be updated as well? By the way this is really great 👏

ndelangen · 2018-01-17T13:55:56Z

@anescobar1991 sounds like a plan! You want to open a PR adding it?

anescobar1991 · 2018-01-17T14:01:21Z

Sure!

Edit: opened #2767 with change

wenzowski · 2018-01-18T16:37:54Z

This will not work on node 9+
pngjs/pngjs#95

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

Hypnosphi added the BREAKING CHANGE label Dec 1, 2017

Hypnosphi changed the base branch from master to release/3.3 December 1, 2017 21:31

thomasbertet force-pushed the storyshots-add-image-snapshots branch 2 times, most recently from e4c45dc to a742d00 Compare December 4, 2017 14:02

Hypnosphi requested a review from ndelangen December 5, 2017 09:28

Hypnosphi removed the BREAKING CHANGE label Dec 6, 2017

danielduan previously requested changes Dec 6, 2017

View reviewed changes

igor-dv requested changes Dec 7, 2017

View reviewed changes

thomasbertet force-pushed the storyshots-add-image-snapshots branch from 1ff1ede to b154fc5 Compare December 11, 2017 15:32

thomasbertet and others added 2 commits January 10, 2018 11:51

Merge branch 'master' into storyshots-add-image-snapshots

c5a1fa1

Add more details in READMEfor image snapshots

96a5df5

Merge branch 'master' into storyshots-add-image-snapshots

1a4d34c

thomasbertet added 2 commits January 10, 2018 16:51

Merge branch 'master' into storyshots-add-image-snapshots

b104897

Merge branch 'master' into storyshots-add-image-snapshots

425937a

Thomas Bertet and others added 2 commits January 11, 2018 16:20

remove extra CI step to build static storybook on unit-test

57b06f8

Merge branch 'master' into storyshots-add-image-snapshots

797f2b7

Hypnosphi approved these changes Jan 11, 2018

View reviewed changes

Hypnosphi merged commit 5dc6e8f into storybookjs:master Jan 11, 2018

thomasbertet deleted the storyshots-add-image-snapshots branch January 11, 2018 16:49

anescobar1991 mentioned this pull request Jan 17, 2018

Updating css-style testing docs to mention StoryShots image snapshots feature #2767

Merged

Hypnosphi mentioned this pull request Mar 1, 2018

fix screenshots tests & add getScreenshotOption to storyshots #3102

Merged

Add ability to use image snapshots to addon-storyshots #2413

Add ability to use image snapshots to addon-storyshots #2413

Conversation

thomasbertet commented Dec 1, 2017 • edited Loading

What I did

How to test

tobilen commented Dec 1, 2017

thomasbertet commented Dec 4, 2017 • edited Loading

thomasbertet commented Dec 5, 2017

thomasbertet commented Dec 6, 2017

Hypnosphi commented Dec 6, 2017

tobilen commented Dec 6, 2017

danielduan Dec 6, 2017 • edited Loading

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

danielduan commented Dec 6, 2017

igor-dv 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

thomasbertet commented Dec 7, 2017

danielduan commented Dec 7, 2017 • edited Loading

codecov bot commented Dec 7, 2017 • edited Loading

Codecov Report

thomasbertet commented Dec 8, 2017 • edited Loading

igor-dv commented Dec 8, 2017

thomasbertet commented Dec 8, 2017

danielduan commented Dec 8, 2017 • edited Loading

Hypnosphi commented Dec 8, 2017

igor-dv commented Dec 8, 2017

Hypnosphi commented Dec 11, 2017 • edited Loading

thomasbertet commented Dec 11, 2017 • edited Loading

thomasbertet commented Dec 11, 2017

igor-dv commented Jan 8, 2018

Hypnosphi commented Jan 8, 2018

thomasbertet commented Jan 9, 2018

Hypnosphi commented Jan 9, 2018 • edited Loading

thomasbertet commented Jan 10, 2018

Hypnosphi commented Jan 10, 2018

thomasbertet commented Jan 10, 2018

thomasbertet commented Jan 11, 2018

Hypnosphi commented Jan 11, 2018

thomasbertet commented Jan 11, 2018

danielduan commented Jan 11, 2018

igor-dv commented Jan 11, 2018

ndelangen commented Jan 11, 2018

anescobar1991 commented Jan 16, 2018 • edited Loading

ndelangen commented Jan 17, 2018

anescobar1991 commented Jan 17, 2018 • edited Loading

wenzowski commented Jan 18, 2018

thomasbertet commented Dec 1, 2017 •

edited

Loading

thomasbertet commented Dec 4, 2017 •

edited

Loading

danielduan Dec 6, 2017 •

edited

Loading

danielduan commented Dec 7, 2017 •

edited

Loading

codecov bot commented Dec 7, 2017 •

edited

Loading

thomasbertet commented Dec 8, 2017 •

edited

Loading

danielduan commented Dec 8, 2017 •

edited

Loading

Hypnosphi commented Dec 11, 2017 •

edited

Loading

thomasbertet commented Dec 11, 2017 •

edited

Loading

Hypnosphi commented Jan 9, 2018 •

edited

Loading

anescobar1991 commented Jan 16, 2018 •

edited

Loading

anescobar1991 commented Jan 17, 2018 •

edited

Loading