-
Notifications
You must be signed in to change notification settings - Fork 194
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
Make Storybook visual regression tests more strict #4919
Conversation
bb6b148
to
466755e
Compare
466755e
to
5615d29
Compare
@obulat a lot of this looks good, but there are also very many snapshots with updates where I'm not sure what the actual change is. My concern with that is if it could be indicative of flake. Fortunately, they did all pass locally 😄 I'll review this in-depth by tomorrow at the latest! |
@@ -18,7 +18,7 @@ export const Default: Story = { | |||
setup() { | |||
return () => | |||
h("div", { id: "teleports" }, [ | |||
h("div", { class: "fixed inset-0 w-full h-full" }, [ | |||
h("div", { class: "fixed inset-0 w-full h-full bg-default" }, [ |
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.
By default, there is no background, and the header background comes from the page background. Adding the background here to make the header look correct in both light and dark snapshots.
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.
The difference is due to the updated background color (bg-surface
?)
91b9295
to
c60ba27
Compare
9e1019e
to
89ff2fb
Compare
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
89ff2fb
to
bab1575
Compare
bab1575
to
37cfc08
Compare
37cfc08
to
3d4c2ac
Compare
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.
The screenshots look great, @obulat!
To clarify the plan here, is maxDiffPixelRatio: 0.003, the final value, or are you incrementally reducing it to 0 so that the review process is easier?
The latter. I hope there's no flakiness, otherwise we might want to introduce |
Fixes
Related to #4305 by @zackkrida
Description
Originally, this PR completely removed
maxDiffPixelRatio
from Storybook Playwright config. However, that resulted in more than 200 changed snapshots. To make it easier, I decided to split that change into several ones. The first step here is to lower themaxDiffPixelRatio
from 0.01 to 0.003.I also added a wrapper to
VSearchTypes
story to make the snapshot better.Testing Instructions
The snapshot changes should be correct, and the CI should pass.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin