-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(docs): add visual regression testing #3290
Conversation
<Label> | ||
<Icon name='mail' /> 23 | ||
</Label> | ||
) | ||
|
||
export default LabelExampleBasic | ||
export default LabelExampleLabel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to change this because it turns out there's another component with the exact same name so sometimes https://react.semantic-ui.com/maximize/label-example-basic/ would render one component first, other times the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an eslint rule we could add to catch issues like this. I'd have to go look and see what we used in another project but it was helpful. Especially in tests.
|
||
import App from './App' | ||
|
||
export default App | ||
|
||
if (process.env.FAKER_SEED) { | ||
faker.seed(parseInt(process.env.FAKER_SEED, 10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that the "random" values are deterministic for each run of the test suite. The normal docs outside of running the visual tests still won't use a seed because it's not provided in that case.
Awesome stuff @jayphelps. I have been aware of some of the visual regression testing tooling out there but I have not looked into implementing and pros/cons yet. This will likely be a massive help, especially until we get more control over the styles themselves when v2 lands. |
7db3723
to
609f844
Compare
Thanks @jayphelps. Over at https://github.com/stardust-ui/react (a fork we're playing with), we've also setup visual regression tests but using https://screener.io. We used the same methodology you've used here, testing the maximized path for each example. I'll try to look more throughly into your post and this PR soon. Again, thanks much for the work and for loving Semantic ❤️ |
609f844
to
8a9a9f8
Compare
1eefffd
to
e073c05
Compare
@@ -95,6 +98,7 @@ | |||
"@babel/standalone": "^7.1.0", | |||
"@mdx-js/loader": "^0.15.5", | |||
"@types/react": "^16.4.14", | |||
"@types/sinon": "5.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added to workaround an upstream issue with the typings DefinitelyTyped/DefinitelyTyped#30657
Cypress pulls in @types/sinon-chai which pulls in whatever the latest version of @types/sinon is, which causes these issues:
error TS2370: A rest parameter must be of an array type.
63 calledWithMatch(...args: TArgs): boolean;
@levithomason right on! Dunno if you've had a chance to compare Screener with Applitools, but here are some cliff-notes on Applitools's unique benefits:
There's a lot of similarities too--they both have a Storybook SDK that might someday interest you. |
Hey @levithomason! Any thoughts on this PR? Would love to help this along if I can! |
Hi @myspivey 👋 Thanks much for the offer of assistance. I just haven't had time to grok it fully. Visual regression testing is something we absolutely do need, so this will get merged in one form or another. Here are some of my thoughts. The bigger picture here is that Semantic UI React is just one of many implementations in the Semantic Org. We consume the CSS from Semantic-UI-CSS right now, which is published out of Semantic-UI (jQuery implementation). It would be most ideal if we could get the visual regression tests down to a lower level in the eco system. This way, when CSS changes happen at Semantic-UI and break something (happens), it is caught there before release. The issues we catch here in the React port are helpful, but the core CSS is not published here. The difficulty with the CSS workflow has driven us to consider maintaining a separate port of the styles here in our own repo. However, we don't want to further fracture the eco system. Not sure where we'll land on this. I'm trying to prove out a single style solution that will produce CSS, LESS, SASS, and CSS-in-JS all from a single build. More on that later. As mentioned we have also implemented visual regression tests over at Stardust UI (a fork) using Screener.io. This has gone pretty well so far. The Screener team is extremely responsive and has shipped features and fixes just for us. I'm not sure what the best tech is for this at the moment. It might be a good idea to test Applitools here and Screener over at Stardust UI and see how it goes. Conclusion, let's fix the conflicts in this PR updated and merge it. @jayphelps thanks for the great work here! Apologies for the delay, we just popped baby number 3 👶 👶 👶 ... 😅 |
@jayphelps account created, API key added, email me@levithomason.com. |
e073c05
to
0aa606b
Compare
@levithomason thank you for the feedback! Very exciting! I will work on getting you setup under the OSS license. PR should be current now. Will keep an eye on this space! |
@levithomason just a quick follow up, your API key has been updated to the OSS license! Cheers and let me know if you have any issues! |
Hey @levithomason, anything else I can help with or clarify? Hope the testing is going great! Cheers! |
There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one. However, PRs for this issue will of course be accepted and welcome! If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks! |
I've merged master and fixed conflicts as well as updated the deps here. We'll see if we can finally get this in... |
👍 Lmk if there’s anything I can do. |
🔥 this is awesome to finally get this in! let me know if we can support! |
We went with Percy for now #4093, however there is already an issue related to @jayphelps I based my PR mainly on your changes, thanks for rising this. |
Hey folks! I'm a big fan of this project from when I worked at Netflix. I've since started another company called This Dot and been experimenting with visual regression testing. Applitools was kind enough to sponsor me to help out and make some PRs to add visual regression tests to some open source projects and I immediately thought of Semantic.
If you're not familiar with visual regression testing there's a pretty good summary in the Storybook docs (most of the page is unrelated to Storybook) but the tl;dr it involves rendering components individually in the DOM and taking a snapshot of the pixels and doing comparisons of it against baselines.
Most solutions do normal 1:1 pixel comparing, but this often results in false positives because the exact pixels can vary. e.g. different GPUs render things differently, so your local tests will fail against baselines created on another computer, or if the CI changes its GPU or there's a browser update, etc, etc, etc. Applitools on the other hand tries to visually compare like a human would, ignoring differences that a human wouldn't perceive or care about. The whole AI thing and such.
Maintaining a separate visual tests can sometimes be a pain on such a large project so what I did instead was take advantage of the wonderful existing docs setup you have. Since you already had the ability to render the component examples all by themselves e.g. https://react.semantic-ui.com/maximize/label-example-label/ This made it pretty dang easy.
So I chose to use Cypress as the browser test harness, which Applitools has an SDK for. I started off with just one test for each type of component so it's sort of a "smoke test" but you can add more if you'd like--it'll take longer to run though.
What happens is the docs site is served, then Cypress launches a headless Chrome where it runs the tests. Applitool's SDK then takes DOM snapshots w/ styles (not screenshots) from that headless Chrome and sends them to Applitool's servers where it renders those results in Chrome and Firefox and then takes a screenshot of that result and does its intelligent comparisons against the baseline. The baseline is created the first time you run it. If there's an issue, it'll be reported in CircleCI with an included link directly to Applitool's web app where you can see the visual differences and either accept or reject them or do other cool things like set ignored regions or tweak the matching behavior.
You'll need to create an Applitools account. and
export APPLITOOLS_API_KEY=valuehere
your API key in your local environment if you run it locally.I also included the necessary changes for it to run in your CircleCI setup, all you need to do is add your APPLITOOLS_API_KEY to your CircleCI project environment variables in their UI.
While the default Applitools account has decent limits they also offer free unlimited accounts to open source projects. In addition to higher limits, it will also check your screenshots concurrently so it runs faster. If you'd like to unlock that you can either contact them directly or if you shoot me your account email I can have them hook you up. If you don't want to post your email here, you can Twitter DM it to me or find my email on my profile.