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

test: add visual regression support and spec all combinations #556

Merged
merged 18 commits into from
Apr 29, 2021

Conversation

rabelloo
Copy link
Contributor

@rabelloo rabelloo commented Apr 7, 2021

Purpose

Add support for visual regression tests.

Approach

Use Cypress to navigate the Storybook instance and screenshot All Combinations stories.

Testing

Locally, yarn e2e for headless or yarn cy for interactive.
In GitHub actions, there should be a new job inside the Test workflow.

Risks

Requires a new set of tools (mainly Cypress).
Couldn't get jest-image-snapshot alone to work correctly directly, something about how Storybook build stories and the whole interaction of Jest + Storybook + Webpack + TypeScript + Babel.
Cypress is pretty good though, we could easily spin up any browser we want.

@rabelloo rabelloo self-assigned this Apr 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2021

🎊 PR Preview 80650e9 has been successfully built and deployed to https://onfido-castor-preview-pr-556.surge.sh

🕐 Build time: 179.247s

🤖 By surge-preview

.github/workflows/test.yml Outdated Show resolved Hide resolved
@rabelloo rabelloo requested a review from josokinas April 7, 2021 17:54
@rabelloo
Copy link
Contributor Author

rabelloo commented Apr 7, 2021

The workflow seems to be failing on an image diff, but hard to see why until artifacts are retained (screenshot).

I suspect it's a browser difference between mine locally (where baseline screenshots were taken) and the one on CI.

Will investigate later.

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.storybook/main.ts Outdated Show resolved Hide resolved
packages/react/src/components/button/button.tsx Outdated Show resolved Hide resolved
e2e/visual-regression/all.e2e.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rabelloo rabelloo changed the title test: add visual regression support test: add visual regression support and spec all combinations Apr 12, 2021
Copy link
Contributor

@josokinas josokinas left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Mind also adding docs to CONTRIBUTING.md please?

packages/react/src/components/button/button.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@rabelloo
Copy link
Contributor Author

Mind also adding docs to CONTRIBUTING.md please?

Please do let me know how it looks.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@rabelloo rabelloo marked this pull request as ready for review April 29, 2021 12:01
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@rabelloo
Copy link
Contributor Author

Alright this is now ready 🎉

Environment differences (between local and CI for example) are enough for images not to match.
So I have added another job screenshot-baselines to provide us with those when they mismatch.

Artifacts are kept nicely. If e2e fails, we can download those images and update the baselines.

@josokinas
Copy link
Contributor

That's great!

One thing to note though, on a core screenshot is not of rendered UI but of code snippets instead, so essentially of this: https://onfido-castor-preview-pr-556.surge.sh/iframe.html?id=core-button--all-combinations&args=&viewMode=story

What do you reckon we should do, probably remove tests for core at least for now?

@rabelloo
Copy link
Contributor Author

Yea I noticed that, our decorator that transforms that string into a DOM node doesn't run in that scenario :/

I think keeping it is fine, we're essentially snapshoting (like, comparing markup) for now.

Making core stories work as iframes is something we want either way, and it's not too hard to fix I reckon.

@josokinas
Copy link
Contributor

OK, fine, good to go then 🚀

rabelloo and others added 12 commits April 29, 2021 13:20
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
rabelloo and others added 6 commits April 29, 2021 13:20
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
Co-authored-by: Julius Osokinas <20243687+josokinas@users.noreply.github.com>
@rabelloo rabelloo force-pushed the test/visual-regression branch from 48a7021 to 80650e9 Compare April 29, 2021 12:20
@rabelloo rabelloo merged commit bc003dd into main Apr 29, 2021
@rabelloo rabelloo deleted the test/visual-regression branch April 29, 2021 12:25
@rabelloo
Copy link
Contributor Author

Next step is to explore using CI matrixes for other browsers, or a 3rd party tool like BrowserStack.

We'll probably need baseline screenshots for each browser though, as they have subtle rendering differences.

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

Successfully merging this pull request may close these issues.

2 participants