fix screenshots tests & add getScreenshotOption to storyshots #3102

thomasbertet · 2018-02-26T13:50:04Z

Issue: none

What I did

Image snapshot were not run since this edit.
Added a default option to take a screenshot of the full page. It seems reasonnable default to me at least, so if it doesn't for you, let's remove that ❓
Added a new method getScreenshotOptions to be able to configure puppeteer screenshot() behavior.
Update package.json to be able to run Jest from the root node_modules, since this commit removes the Jest deps from official-storybook.

Also, I wanted to draw your attention that if we specify a threshold higher than 0 we might end-up with some bad visual regression. More info below.

How to test

At root: run yarn run test --image.
At examples/official-storybook: yarn run image-snapshots

Is this testable with jest or storyshots?

already did.

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

nope

Does this need an update to the documentation?

Yep, done.

Threshold for jest-image-snapshot.

I ran into the following issue:

This is from the addon-a11-default story.

This diff is ~2.5 % as for jest-image-snapshot. It looks bad to me, so I believe we will not want to have this kind of changes to pass, are we ? Just a question though, if you are fine with it, let it be and I will re-add the threshold to 4%.

thomasbertet · 2018-02-26T13:52:14Z

addons/storyshots/src/test-body-image-snapshot.js

  };

-  testFn.beforeEach = () =>
+  testFn.beforeAll = () =>


Moving this to beforeAll is a perf booster. No need to re-run Chrome for each tests.

Hypnosphi · 2018-02-26T16:26:59Z

Added a default option to take a screenshot of the full page

Is it a breaking change?

Hypnosphi · 2018-02-26T16:35:06Z

Looks like there are a lot more failures than just one addon-a11y story:
https://circleci.com/gh/storybooks/storybook/40645

BTW, for some reason diffs are not uploaded as artifacts
https://circleci.com/gh/storybooks/storybook/40645#artifacts/containers/0

thomasbertet · 2018-02-26T17:43:55Z

yay @Hypnosphi I believe I broke everything :D The diffs were not even generated in the first place 👎
I will check to understand why. You have to trust my word for now ahah :)

thomasbertet · 2018-02-26T17:46:29Z

Oh and yes this is breaking. Since the 3.4 is not out yet (still in alpha if I'm correct) I was thinking maybe that's not too bad to throw it now.
I feel now that it may be not necessary to generate a full page each time ... don't know what you think about this ?

codecov · 2018-02-26T18:11:26Z

Codecov Report

Merging #3102 into master will decrease coverage by 56.51%.
The diff coverage is 50%.

@@             Coverage Diff             @@
##           master    #3102       +/-   ##
===========================================
- Coverage    92.5%   35.98%   -56.52%     
===========================================
  Files           6      437      +431     
  Lines          40     9474     +9434     
  Branches        2      899      +897     
===========================================
+ Hits           37     3409     +3372     
- Misses          2     5506     +5504     
- Partials        1      559      +558

Impacted Files	Coverage Δ
addons/storyshots/src/test-body-image-snapshot.js	`13.79% <33.33%> (ø)`
addons/storyshots/src/index.js	`85.71% <75%> (ø)`
addons/graphql/src/components/FullScreen/style.js	`0% <0%> (ø)`
addons/links/register.js	`0% <0%> (ø)`
...dons/actions/src/lib/types/object/getObjectName.js	`62.5% <0%> (ø)`
...ct-native/src/server/config/webpack.config.prod.js	`0% <0%> (ø)`
addons/storyshots/src/test-bodies.js	`89.47% <0%> (ø)`
addons/actions/src/manager.js	`0% <0%> (ø)`
...codemod/src/transforms/update-organisation-name.js	`40.62% <0%> (ø)`
...p/angular/src/server/config/webpack.config.prod.js	`0% <0%> (ø)`
... and 423 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 c9080fb...d6b0eb5. Read the comment docs.

Hypnosphi · 2018-02-26T18:50:48Z

Ok I just forgot it's an alpha feature

thomasbertet · 2018-02-28T14:42:49Z

I re-added the workaround for puppeteer as I suspect this is linked to Chrome headless not willing to run properly.

It was previously removed in #3052, and not noticed it was used by the official-storybook storyshot tests because tests were not run anymore.

Hypnosphi · 2018-02-28T15:55:56Z

Still lots of errors =(
But now one can see that it's because of different fonts on different OS. That's why I raised the threshold:
https://40938-54173593-gh.circle-artifacts.com/0/official_storybook_image_snapshots/__diff_output__/storyshots-image-runner-js-image-snapshots-addons-a-11-y-disabled-1-diff.png

thomasbertet · 2018-02-28T17:54:07Z

Yep, I see that too.. I was actually trying to determine a good threshold ... not an easy thing to do...
4% is high due to having large image with a lot of white pixels...
1% might be enough, don't you think ?

Hypnosphi · 2018-02-28T19:05:31Z

1% might be enough, don't you think ?

Sure, if you can make current tests pass with it in a way that can be reproduced by other contributors. I mean, examples may change, and we need a way to update screenshots that would work in any environment (e.g., some people use Windows, others use Mac, and on CI we have Linux)

thomasbertet · 2018-03-01T08:04:14Z

Yep I get it, can you point me to contributors willing to help test these screenshots for Mac & Windows ? I've only got Linux.

Hypnosphi · 2018-03-01T09:34:09Z

I use Mac, @igor-dv uses Windows

thomasbertet · 2018-03-01T09:35:30Z

Allright, thanks. Could you test this then ? As well as @igor-dv ?
Just build the static storybook & run yarn test --image :) Thanks !

Hypnosphi · 2018-03-01T09:51:34Z

Sure, but some of the tests are still failing on CI: https://circleci.com/gh/storybooks/storybook/41017#artifacts/containers/0

thomasbertet · 2018-03-01T09:53:21Z

Yep .. argh not sure what to do, just raise the threshold and let some image still pass even if incorrect ? :(

Hypnosphi · 2018-03-01T10:03:03Z

I think a good solution would involve taking screenshot and verifying them in the same environment (e.g. in docker)

Without that, you'll always have a number of false positives or negatives depending on threshould. To me, personally, false positives (failures without actual error) are much worse, because they block development.

In the case of storybook, we won't miss any visual changes, because Chromatic will catch them for us. But for external storyshots users, with current realization it always will be a tough compromise. That's basically what @tobilen was talking about when you opened the first PR:
#2413 (comment)

thomasbertet · 2018-03-01T14:01:26Z

I think a good solution would involve taking screenshot and verifying them in the same environment (e.g. in docker)

Are we doing this anywhere in this repo ?

Hypnosphi · 2018-03-01T15:50:00Z

No. It requires users to have docker installed on their side

thomasbertet · 2018-03-02T07:54:20Z

I will just raise the threshold then.

Hypnosphi · 2018-03-02T18:14:33Z

Ok, I'll check how it works on my machine

Hypnosphi · 2018-03-02T19:27:35Z

examples/official-storybook/image-snapshots/storyshots-image.runner.js

@@ -21,13 +21,22 @@ if (!fs.existsSync(pathToStorybookStatic)) {
    suite: 'Image snapshots',
    framework: 'react',
    configPath: path.join(__dirname, '..'),
-    storyNameRegex: /^((?!tweaks static values with debounce delay|Inlines component inside story).)$/,


What would be the correct way to ignore those stories?

That's the way to ignore... But adding a proper ignore list to the api will definitely simplify everything...

Looks like it didn't work: it ignored all the tests instead

UPD: I just missed * in the end, it had to be this:

/^((?!tweaks static values with debounce delay|Inlines component inside story).)*$/

adding a proper ignore list

#2679 should help a lot here. We could just introduce skipSnapshots story parameter (name can be configurable)

Hypnosphi · 2018-03-04T10:15:33Z

I have a local failure:

● Image snapshots › Addons|Info.Options.inline › Inlines component inside story

    toMatchImageSnapshot(): Received image size must match baseline snapshot size in order to make comparison.

      69 |       return page.screenshot(getScreenshotOptions({ context: context, url: url }));
      70 |     }).then(function (image) {
    > 71 |       expect(image).toMatchImageSnapshot(getMatchOptions({ context: context, url: url }));
      72 |     });
      73 |   };
      74 | 
      
      at diffImageToSnapshot (node_modules/jest-image-snapshot/src/diff-snapshot.js:50:13)
      at Object.toMatchImageSnapshot (node_modules/jest-image-snapshot/src/index.js:50:20)
      at Object.throwingMatcher [as toMatchImageSnapshot] (node_modules/expect/build/index.js:214:24)
      at addons/storyshots/dist/test-body-image-snapshot.js:71:21

  ● Image snapshots › Addons|Info.Options.inline › Inlines component inside story

    expect.assertions(1)
    
    Expected one assertion to be called but received zero assertion calls.
      
      at extractExpectedAssertionsErrors (node_modules/expect/build/extract_expected_assertions_errors.js:46:19)

I guess it's because different fonts lead to different line wrapping, which affects page height

thomasbertet · 2018-03-05T08:52:37Z

Thanks @Hypnosphi for helping with this 💌 .
Yep I believe your assertion is correct. I was talking about this with the maintainer of jest-image-snapshot here and there
The discussion is about at least generating a simple diff when the images are different in sizes instead of just throwing and see nothing.

This won't fix the main issue we got here though. I may specify a greater viewport height to fix this issues, meaning these stories will be taller than the real viewport. WDYT ?

Hypnosphi · 2018-03-05T09:01:33Z

I think we should just ignore the problematic stories

Hypnosphi · 2018-03-08T23:22:57Z

examples/official-storybook/image-snapshots/storyshots-image.runner.js

        failureThresholdType: 'percent',
      }),
+      beforeScreenshot: (page, { context }) => {
+        // Screenshots for this story have to be tweaked.
+        if (context.kind === 'ui/Layout') {


Looks like it's not needed anymore

Hypnosphi · 2018-03-08T23:24:09Z

Thanks, I hope I'll be able to do the local testing again tomorrow

Hypnosphi · 2018-03-08T23:41:54Z

examples/official-storybook/stories/addon-storyshots.stories.js

+const text = 'Testing the storyshots addon';
+
+storiesOf('Addons|Storyshots', module)
+  .add('Default', () => <BaseButton label="" />)


Using an an unstyled button for those stories isn't a really good idea, because it uses appearance of system UI button, which means the differences between OS are huge

E. g., this is how storyshots story looks like on mac

Oh so true ! I'll change the test, did a bit too quickly.

Hypnosphi · 2018-03-08T23:44:15Z

Please take a look at screenshots before commiting them:
https://github.com/thomasbertet/storybook/blob/b71e110cc6fb5882526b7cdae0547bfc542621b0/examples/official-storybook/image-snapshots/__image_snapshots__/storyshots-image-runner-js-image-snapshots-addons-storyshots-default-1-snap.png

thomasbertet · 2018-03-09T16:18:31Z

My bad, did this a bit too quickly, will be extra careful !

Hypnosphi

Works OK on macOS. @igor-dv please test on Windows

igor-dv · 2018-03-11T09:41:59Z

I am getting this:

Not sure if it's a bug or my environment 🤷‍♂️

thomasbertet · 2018-03-12T13:27:26Z

Oh @igor-dv seems a weird issue .... :(
Does it fails every time ? I mean did you try multiple times ?
Assuming that it worked before ... not sure what to do, maybe you can try to upgrade puppeteer to check if its better ?

igor-dv · 2018-03-12T14:33:21Z

Yeah, tried it several times, I'll investigate it a bit more

igor-dv

My bad. Tried it before with an unstable version of Node. Works with the latest stable.

thomasbertet · 2018-03-13T10:22:59Z

Great, thanks @Hypnosphi & @igor-dv !

thomasbertet changed the title ~~fix sceenshots tests & update storyshots behavior~~ fix screenshots tests & update storyshots behavior Feb 26, 2018

thomasbertet changed the title ~~fix screenshots tests & update storyshots behavior~~ fix screenshots tests & add getScreenshotOption to storyshots Feb 26, 2018

thomasbertet added the addon: storyshots label Feb 26, 2018

thomasbertet commented Feb 26, 2018

View reviewed changes

thomasbertet added the in progress label Feb 26, 2018

thomasbertet force-pushed the feat-storyshots-screenshots-options branch from a9eb76a to 37f0564 Compare February 28, 2018 14:31

Hypnosphi reviewed Mar 2, 2018

View reviewed changes

Hypnosphi reviewed Mar 8, 2018

View reviewed changes

Hypnosphi added this to the v3.4.0 milestone Mar 8, 2018

Hypnosphi reviewed Mar 8, 2018

View reviewed changes

Update snapshots & screenshots

d4aa7c9

thomasbertet removed the in progress label Mar 9, 2018

thomasbertet and others added 2 commits March 9, 2018 17:41

Merge branch 'master' into feat-storyshots-screenshots-options

77b15f4

Merge branch 'master' into feat-storyshots-screenshots-options

ec2fd12

Hypnosphi approved these changes Mar 10, 2018

View reviewed changes

Merge branch 'master' into feat-storyshots-screenshots-options

ccdffad

igor-dv approved these changes Mar 13, 2018

View reviewed changes

Hypnosphi added feature request maintenance User-facing maintenance tasks ready labels Mar 13, 2018

Merge branch 'master' into feat-storyshots-screenshots-options

d6b0eb5

Hypnosphi merged commit 4808845 into storybookjs:master Mar 13, 2018

thomasbertet deleted the feat-storyshots-screenshots-options branch March 13, 2018 10:21

dependencies bot mentioned this pull request Jan 15, 2021

Update 13 dependencies from npm #2555

Merged

This was referenced Dec 7, 2021

[Snyk] Security upgrade node-sass from 4.14.1 to 7.0.0 aviadatsnyk/storybook#303

Open

[Snyk] Security upgrade node-sass from 4.14.1 to 7.0.0 adrukh/storybook#244

Open

This was referenced Dec 27, 2021

[Snyk] Security upgrade node-sass from 4.14.1 to 7.0.1 aviadatsnyk/storybook#306

Open

[Snyk] Security upgrade node-sass from 4.14.1 to 7.0.1 adrukh/storybook#250

Open

adrukh mentioned this pull request May 14, 2022

[Snyk] Fix for 1 vulnerabilities adrukh/storybook#285

Open

lili2311 mentioned this pull request Oct 16, 2023

[Snyk-dev] Security upgrade node-sass from 4.14.1 to 7.0.1 lili2311/storybook#184

Open

fix screenshots tests & add getScreenshotOption to storyshots #3102

fix screenshots tests & add getScreenshotOption to storyshots #3102

Conversation

thomasbertet commented Feb 26, 2018 • edited Loading

What I did

How to test

Threshold for jest-image-snapshot.

Choose a reason for hiding this comment

Hypnosphi commented Feb 26, 2018

Hypnosphi commented Feb 26, 2018

thomasbertet commented Feb 26, 2018 • edited Loading

thomasbertet commented Feb 26, 2018

codecov bot commented Feb 26, 2018 • edited Loading

Codecov Report

Hypnosphi commented Feb 26, 2018

thomasbertet commented Feb 28, 2018 • edited Loading

Hypnosphi commented Feb 28, 2018

thomasbertet commented Feb 28, 2018

Hypnosphi commented Feb 28, 2018 • edited Loading

thomasbertet commented Mar 1, 2018

Hypnosphi commented Mar 1, 2018

thomasbertet commented Mar 1, 2018

Hypnosphi commented Mar 1, 2018

thomasbertet commented Mar 1, 2018

Hypnosphi commented Mar 1, 2018

thomasbertet commented Mar 1, 2018

Hypnosphi commented Mar 1, 2018 • edited Loading

thomasbertet commented Mar 2, 2018

Hypnosphi commented Mar 2, 2018

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

Hypnosphi commented Mar 4, 2018

thomasbertet commented Mar 5, 2018 • edited Loading

Hypnosphi commented Mar 5, 2018

Choose a reason for hiding this comment

Hypnosphi commented Mar 8, 2018

Hypnosphi Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Mar 8, 2018

thomasbertet commented Mar 9, 2018

Hypnosphi left a comment

Choose a reason for hiding this comment

igor-dv commented Mar 11, 2018

thomasbertet commented Mar 12, 2018

igor-dv commented Mar 12, 2018

igor-dv left a comment

Choose a reason for hiding this comment

thomasbertet commented Mar 13, 2018

thomasbertet commented Feb 26, 2018 •

edited

Loading

thomasbertet commented Feb 26, 2018 •

edited

Loading

codecov bot commented Feb 26, 2018 •

edited

Loading

thomasbertet commented Feb 28, 2018 •

edited

Loading

Hypnosphi commented Feb 28, 2018 •

edited

Loading

Hypnosphi commented Mar 1, 2018 •

edited

Loading

thomasbertet commented Mar 5, 2018 •

edited

Loading

Hypnosphi Mar 8, 2018 •

edited

Loading