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 diff test to designkit #6707

Merged
merged 8 commits into from
May 8, 2023

Conversation

ashtonG
Copy link
Contributor

@ashtonG ashtonG commented May 3, 2023

Description

This adds a visual diff test to designkit. We generate screenshots of each section of the designkit on the branch in question using playwright and then again on the main branch. We then use playwright's built in image comparison facilities to generate a diff between the screenshots. If differences are found, both screenshots and the diff are output to a flat html file that displays the results and CI will report in github.

Test Plan

CI Only -- no testing required

Commentary (optional)

we're using undici for our node fetch needs because it's the source of the experimental fetch library that landed in node 18, so when we upgrade, we should be able to just remove the import and library and everything will be the same.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

This adds a visual diff test to designkit. We generate screenshots of
each section of the designkit on the branch in question using playwright
and then again on the main branch. We then use playwright's built in
image comparison facilities to generate a diff between the screenshots.
If differences are found, both screenshots and the diff are output to a
flat html file that displays the results and CI will report in github.
@cla-bot cla-bot bot added the cla-signed label May 3, 2023
@netlify
Copy link

netlify bot commented May 3, 2023

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 2ae013b
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/645922d35887940008c06a97
😎 Deploy Preview https://deploy-preview-6707--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@ClaireNeveu ClaireNeveu left a comment

Choose a reason for hiding this comment

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

If there are no changes from the previous version this looks great to me. I liked it when you PRed it before. I didn't do a deep dive into the code but I can if you want me to. I could also ask somebody in devops if you want someone to look over your circleCI stuff.

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

thanks for working. I have some questions

  • Don't we wanna test it on different browsers as well as Chrome?
  • Is this test available on local too?
  • Why not using Playwright visual testing directly? Playwright offers rich image diff reports.

example:

Screen.Recording.2023-05-04.at.10.24.36.AM.mov

if these are already considered, i think its good to go.

.circleci/config.yml Show resolved Hide resolved
@ashtonG
Copy link
Contributor Author

ashtonG commented May 4, 2023

We're not using playwright's image comparison stuff directly because we don't want this test to block. Playwright's workflow would require the user to notice the test failing and then generate new screenshots. While we could fake this by running a screenshot test on main to generate the expected image and then again on the branch to generate the diffs, we'd still need a way to define which components to look at. Doing it statically introduces the possibility of the components list drifting out of sync, and doing it dynamically means that we either don't get screenshots of added components or removed components.

@ClaireNeveu
Copy link
Contributor

Noticed the latest version doesn't just display components with a diff.

@ashtonG ashtonG merged commit b0c2fbc into main May 8, 2023
@ashtonG ashtonG deleted the build/WEB-1043/visual-regression-redux branch May 8, 2023 17:24
jerryharrow pushed a commit to jerryharrow/determined that referenced this pull request May 15, 2023
@dannysauer dannysauer added this to the 0.22.2 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants