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
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ jobs:
name: "Restore core dist cache"
keys:
- core-dist-{{ .Revision }}
- run:
name: Workaround for https://github.com/GoogleChrome/puppeteer/issues/290
command: sh ./scripts/workaround-puppeteer-issue-290.sh
- run:
name: "Build react kitchen-sink"
command: |
Expand Down
16 changes: 16 additions & 0 deletions addons/storyshots/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,22 @@ initStoryshots({suite: 'Image storyshots', test: imageSnapshot({storybookUrl: 'h

`beforeScreenshot` receives the [Puppeteer page instance](https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#class-page) and an object: `{ context: {kind, story}, url}`. _kind_ is the kind of the story and the _story_ its name. _url_ is the URL the browser will use to screenshot. `beforeScreenshot` is part of the promise chain and is called after the browser navigation is completed but before the screenshot is taken. It allows for triggering events on the page elements and delaying the screenshot and can be used avoid regressions due to mounting animations.

### Specifying options to _screenshot()_

You might use `getScreenshotOptions` to specify options for screenshot. Will be passed to [Puppeteer .screenshot() fn](https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagescreenshotoptions)

```js
import initStoryshots, { imageSnapshot } from '@storybook/addon-storyshots';
const getScreenshotOptions = ({context, url}) {
return {
fullPage: false // Do not take the full page screenshot. Default is 'true' in Storyshots.
}
}
initStoryshots({suite: 'Image storyshots', test: imageSnapshot({storybookUrl: 'http://localhost:6006', getScreenshotOptions})});
```

`getScreenshotOptions` receives an object `{ context: {kind, story}, url}`. _kind_ is the kind of the story and the _story_ its name. _url_ is the URL the browser will use to screenshot.

### Integrate image storyshots with regular app

You may want to use another Jest project to run your image snapshots as they require more resources: Chrome and Storybook built/served.
Expand Down
24 changes: 9 additions & 15 deletions addons/storyshots/src/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import fs from 'fs';
import glob from 'glob';
import global, { describe, it, beforeEach, afterEach } from 'global';
import global, { describe, it } from 'global';
import addons from '@storybook/addons';
import loadFramework from './frameworkLoader';
import createChannel from './storybook-channel-mock';
Expand All @@ -27,6 +27,8 @@ export {
imageSnapshot,
};

const methods = ['beforeAll', 'beforeEach', 'afterEach', 'afterAll'];

export default function testStorySnapshots(options = {}) {
if (typeof describe !== 'function') {
throw new Error('testStorySnapshots is intended only to be used inside jest');
Expand All @@ -53,6 +55,12 @@ export default function testStorySnapshots(options = {}) {

const testMethod = options.test || snapshotWithOptions({ options: snapshotOptions });

methods.forEach(method => {
if (typeof testMethod[method] === 'function') {
global[method](testMethod[method]);
}
});

// eslint-disable-next-line
for (const group of stories) {
const { fileName, kind } = group;
Expand All @@ -63,20 +71,6 @@ export default function testStorySnapshots(options = {}) {
}

describe(suite, () => {
beforeEach(() => {
if (typeof testMethod.beforeEach === 'function') {
return testMethod.beforeEach();
}
return Promise.resolve();
});

afterEach(() => {
if (typeof testMethod.afterEach === 'function') {
return testMethod.afterEach();
}
return Promise.resolve();
});

describe(kind, () => {
// eslint-disable-next-line
for (const story of group.stories) {
Expand Down
23 changes: 14 additions & 9 deletions addons/storyshots/src/test-body-image-snapshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,16 @@ import { logger } from '@storybook/node-logger';

expect.extend({ toMatchImageSnapshot });

// We consider taking the full page is a reasonnable default.
const defaultScreenshotOptions = () => ({ fullPage: true });

const noop = () => {};

export const imageSnapshot = ({
storybookUrl = 'http://localhost:6006',
getMatchOptions = () => {},
beforeScreenshot = () => {},
getMatchOptions = noop,
getScreenshotOptions = defaultScreenshotOptions,
beforeScreenshot = noop,
}) => {
let browser; // holds ref to browser. (ie. Chrome)
let page; // Hold ref to the page to screenshot.
Expand Down Expand Up @@ -45,14 +51,13 @@ export const imageSnapshot = ({
throw e;
})
.then(() => beforeScreenshot(page, { context, url }))
.then(() =>
page.screenshot().then(image => {
expect(image).toMatchImageSnapshot(getMatchOptions({ context, url }));
})
);
.then(() => page.screenshot(getScreenshotOptions({ context, url })))
.then(image => {
expect(image).toMatchImageSnapshot(getMatchOptions({ context, url }));
});
};

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.

puppeteer
// add some options "no-sandbox" to make it work properly on some Linux systems as proposed here: https://github.com/Googlechrome/puppeteer/issues/290#issuecomment-322851507
.launch({ args: ['--no-sandbox ', '--disable-setuid-sandbox'] })
Expand All @@ -64,7 +69,7 @@ export const imageSnapshot = ({
page = p;
});

testFn.afterEach = () => browser.close();
testFn.afterAll = () => browser.close();

return testFn;
};
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,25 @@ if (!fs.existsSync(pathToStorybookStatic)) {
} else {
initStoryshots({
suite: 'Image snapshots',
storyKindRegex: /^Addons\|Storyshots/,
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 👍

test: imageSnapshot({
storybookUrl: `file://${pathToStorybookStatic}`,
getMatchOptions: () => ({
failureThreshold: 0.04, // 4% threshold,
failureThreshold: 0.02, // 2% threshold,
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

// Some weird stuff are done inside Layout component, so we have to wait some to get a viable screenshot.
// Tried some values here, seems 200 is long enough, not too short either :)
// Not sure why this is required but seems better than to avoid taking screenshots for these.
return new Promise(resolve => setTimeout(resolve, 200));
}
return Promise.resolve();
},
}),
});
}
3 changes: 2 additions & 1 deletion examples/official-storybook/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"build-storybook": "build-storybook -c ./ -s built-storybooks",
"chromatic": "chromatic test --storybook-addon --exit-zero-on-changes --app-code ab7m45tp9p",
"generate-addon-jest-testresults": "jest --config=tests/addon-jest.config.json --json --outputFile=stories/addon-jest.testresults.json",
"image-snapshots": "yarn run build-storybook && jest --projects=./image-snapshots",
"image-snapshots": "yarn run build-storybook && yarn run do-image-snapshots",
"do-image-snapshots": "../../node_modules/.bin/jest --projects=./image-snapshots",
"storybook": "start-storybook -p 9011 -c ./ -s built-storybooks"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Storyshots Addons|Storyshots Default 1`] = `<button />`;

exports[`Storyshots Addons|Storyshots Disabled 1`] = `
<button
disabled=""
>
Testing the storyshots addon
</button>
`;

exports[`Storyshots Addons|Storyshots Label 1`] = `
<button>
Testing the storyshots addon
</button>
`;
10 changes: 10 additions & 0 deletions examples/official-storybook/stories/addon-storyshots.stories.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import React from 'react';
import { storiesOf } from '@storybook/react';
import BaseButton from '../components/BaseButton';

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.

.add('Label', () => <BaseButton label={text} />)
.add('Disabled', () => <BaseButton disabled label={text} />);
9 changes: 9 additions & 0 deletions scripts/workaround-puppeteer-issue-290.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/bin/sh
# Workaround for https://github.com/GoogleChrome/puppeteer/issues/290

sudo apt-get update
sudo apt-get install -yq gconf-service libasound2 libatk1.0-0 libc6 libcairo2 libcups2 libdbus-1-3 \
libexpat1 libfontconfig1 libgcc1 libgconf-2-4 libgdk-pixbuf2.0-0 libglib2.0-0 libgtk-3-0 libnspr4 \
libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 \
libxcursor1 libxdamage1 libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 \
ca-certificates fonts-liberation libappindicator1 libnss3 lsb-release xdg-utils wget