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

Add chromatic visual tests #2505

Merged
merged 17 commits into from
Dec 27, 2017
Merged

Add chromatic visual tests #2505

merged 17 commits into from
Dec 27, 2017

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Dec 18, 2017

Issue: We didn't have visual regression tests for our stories

Note: this is built on #2504

What I did

Added chromatic to our CI

How to test

Make a UI change, check out the build at https://chromaticqa.com

NOTE: this is not currently accessible to members of the storybooks org, just sorting out org permissions.

Anyone in the storybooks org on GH should be able to access the project here: https://www.chromaticqa.com/builds?appId=5a375b97f4b14f0020b0cda3

@tmeasday
Copy link
Member Author

@ndelangen do you have the permission to grant access from the storybooks GH org?

@Hypnosphi
Copy link
Member

Note that there's an ongoing work on adding visual testing functionality to storyshots: #2413

@tmeasday
Copy link
Member Author

tmeasday commented Dec 18, 2017

@Hypnosphi

Note that there's an ongoing work on adding visual testing functionality to storyshots

Yes! I think this is great! I think however for a larger project like this one Chromatic can offer a little more on top of that, namely parallelized / cloud screenshots and an review flow UI. Interested to hear your thoughts once you've given it a go.

package.json Outdated
"repo-dirty-check": "node ./scripts/repo-dirty-check"
"repo-dirty-check": "node ./scripts/repo-dirty-check",
"storybook": "start-storybook -p 6006",
"chromatic": "chromatic test --storybook-addon --app-code sm3026v658i"
Copy link
Member

Choose a reason for hiding this comment

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

If it's not a public key, probably better hide it with the CI parameter

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, see the note on the commit ;)

Copy link
Member

Choose a reason for hiding this comment

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

Hah, 😄

@Hypnosphi
Copy link
Member

Oh, that's actually pretty cool. I think there's a value in adding this for all the cra-kitchen-sink stories

@tmeasday
Copy link
Member Author

I think there's a value in adding this for all the cra-kitchen-sink stories

Yes, I'll get this going once we sort out #2504

@tmeasday tmeasday force-pushed the tmeasday/chromatic branch 2 times, most recently from f71e104 to cacfcf5 Compare December 20, 2017 04:34
@ndelangen
Copy link
Member

Please rebase, so i can better judge this PR, looks all right.

Different storybook app layers install the `start-storybook` command on top of each other.

We won't leave the storybook here, though.
@tmeasday
Copy link
Member Author

@ndelangen done.

- run:
name: "Visually test storybook"
command: |
cd examples/official-storybook; yarn chromatic
Copy link
Member

Choose a reason for hiding this comment

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

Please add a root-level shortcut script for that

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be even made an option in scripts/test.js

Copy link
Member

Choose a reason for hiding this comment

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

Is this something we want to be able to run locally?

Copy link
Member

Choose a reason for hiding this comment

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

I believe so, at least to update the refs

Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression chromatic provided a review system to decide if a change is OK or NOT_OK ?

Copy link
Member

Choose a reason for hiding this comment

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

If the tests are run by each dev, how does chromatic overcome the rendering differences per system?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIU they actually run in cloud even when launched locally

Copy link
Member

Choose a reason for hiding this comment

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

chromatic provided a review system to decide if a change is OK or NOT_OK

That's cool that they do that! And I feel convenient doing that even before submitting a PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you can run it locally and it does take the snapshots in the cloud still.

The only hiccup with this is the CHROMATIC_APP_CODE environment variable -- it will need to be set for the test to run locally. Any thoughts on how to make that easier for devs (you can find it in the settings page at https://www.chromaticqa.com/settings?appId=5a375b97f4b14f0020b0cda3)

Copy link
Member Author

Choose a reason for hiding this comment

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

@Hypnosphi -- not sure how to best add it to the scripts/test.js command; it seems the way that is written a command either needs to be a jest project or a npm test command for some subdirectory. I'm not sure we want to make it npm test for examples/official-storybook though.

Should I change the script to support a full custom command? Perhaps in a separate PR?

@ndelangen ndelangen changed the base branch from release/3.3 to master December 23, 2017 22:56
@storybookjs storybookjs deleted a comment from codecov bot Dec 24, 2017
@codecov
Copy link

codecov bot commented Dec 26, 2017

Codecov Report

Merging #2505 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2505   +/-   ##
=======================================
  Coverage   32.68%   32.68%           
=======================================
  Files         398      398           
  Lines        8838     8838           
  Branches      953      949    -4     
=======================================
  Hits         2889     2889           
+ Misses       5313     5294   -19     
- Partials      636      655   +19
Impacted Files Coverage Δ
...-native/src/preview/components/OnDeviceUI/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 50.74% <0%> (ø) ⬆️
addons/actions/src/lib/retrocycle.js 54.54% <0%> (ø) ⬆️
addons/actions/src/lib/reviver.js 58.33% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.63% <0%> (ø) ⬆️
lib/codemod/src/transforms/update-addon-info.js 50% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Rules.js 0% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
addons/actions/src/lib/util/getPropertiesList.js 60% <0%> (ø) ⬆️
... and 48 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 a13f39b...e521ade. Read the comment docs.

@tmeasday tmeasday added the ready label Dec 27, 2017
@tmeasday
Copy link
Member Author

This is ready to merge when the 3.3 release process is complete.

@Hypnosphi
Copy link
Member

Hypnosphi commented Dec 27, 2017

How does it affect release process? It doesn't change any external packages

@tmeasday
Copy link
Member Author

It shouldn't, but @shilman asked me to hold off merging into master until the release is out...

@Hypnosphi Hypnosphi added the maintenance User-facing maintenance tasks label Dec 27, 2017
@ndelangen
Copy link
Member

Merge when ready, if you ask me

@Hypnosphi Hypnosphi merged commit 7e4e6b8 into master Dec 27, 2017
@Hypnosphi Hypnosphi deleted the tmeasday/chromatic branch December 27, 2017 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants