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

fix screenshots tests & add getScreenshotOption to storyshots #3102

Merged

Conversation

thomasbertet
Copy link
Contributor

@thomasbertet thomasbertet commented Feb 26, 2018

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:

storyshots-image-runner-js-image-snapshots-addons-a-11-y-default-1-diff
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 thomasbertet changed the title fix sceenshots tests & update storyshots behavior fix screenshots tests & update storyshots behavior Feb 26, 2018
@thomasbertet thomasbertet changed the title fix screenshots tests & update storyshots behavior fix screenshots tests & add getScreenshotOption to storyshots Feb 26, 2018
};

testFn.beforeEach = () =>
testFn.beforeAll = () =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Hypnosphi
Copy link
Member

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

Is it a breaking change?

@Hypnosphi
Copy link
Member

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
Copy link
Contributor Author

thomasbertet commented Feb 26, 2018

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
Copy link
Contributor Author

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
Copy link

codecov bot commented Feb 26, 2018

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Member

Ok I just forgot it's an alpha feature

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

thomasbertet commented Feb 28, 2018

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Hypnosphi commented Feb 28, 2018

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
Copy link
Contributor Author

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
Copy link
Member

I use Mac, @igor-dv uses Windows

@thomasbertet
Copy link
Contributor Author

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

@Hypnosphi
Copy link
Member

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

@thomasbertet
Copy link
Contributor Author

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

@Hypnosphi
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Hypnosphi commented Mar 1, 2018

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

@thomasbertet
Copy link
Contributor Author

I will just raise the threshold then.

@Hypnosphi
Copy link
Member

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

@@ -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).)$/,
Copy link
Member

Choose a reason for hiding this comment

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

What would be the correct way to ignore those stories?

Copy link
Member

Choose a reason for hiding this comment

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

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

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 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).)*$/

Copy link
Member

Choose a reason for hiding this comment

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

adding a proper ignore list

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah 👍

@Hypnosphi
Copy link
Member

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
Copy link
Contributor Author

thomasbertet commented Mar 5, 2018

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
Copy link
Member

I think we should just ignore the problematic stories

failureThresholdType: 'percent',
}),
beforeScreenshot: (page, { context }) => {
// Screenshots for this story have to be tweaked.
if (context.kind === 'ui/Layout') {
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 it's not needed anymore

@Hypnosphi Hypnosphi added this to the v3.4.0 milestone Mar 8, 2018
@Hypnosphi
Copy link
Member

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

const text = 'Testing the storyshots addon';

storiesOf('Addons|Storyshots', module)
.add('Default', () => <BaseButton label="" />)
Copy link
Member

@Hypnosphi Hypnosphi Mar 8, 2018

Choose a reason for hiding this comment

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

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
screen shot 2018-03-09 at 01 41 03

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@thomasbertet
Copy link
Contributor Author

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

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

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

@igor-dv
Copy link
Member

igor-dv commented Mar 11, 2018

I am getting this:

image

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

@thomasbertet
Copy link
Contributor Author

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
Copy link
Member

igor-dv commented Mar 12, 2018

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

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

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

@Hypnosphi Hypnosphi added feature request maintenance User-facing maintenance tasks ready labels Mar 13, 2018
@Hypnosphi Hypnosphi merged commit 4808845 into storybookjs:master Mar 13, 2018
@thomasbertet thomasbertet deleted the feat-storyshots-screenshots-options branch March 13, 2018 10:21
@thomasbertet
Copy link
Contributor Author

Great, thanks @Hypnosphi & @igor-dv !

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

Successfully merging this pull request may close these issues.

3 participants