-
Notifications
You must be signed in to change notification settings - Fork 197
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
Rename all snapshots to "light" #4914
Conversation
2567791
to
bfa076e
Compare
1b312d1
to
07284b8
Compare
@@ -21,6 +21,8 @@ const pwCommand = process.env.FASTSTART !== "false" ? "dev" : "prod:playwright" | |||
|
|||
const config: PlaywrightTestConfig = { | |||
forbidOnly: !!process.env.CI, | |||
snapshotPathTemplate: |
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, this is '{snapshotDir}/{testFileDir}/{testFileName}-snapshots/{arg}{-projectName}{-snapshotSuffix}{ext}'
snapshotSuffix
is the platform (linux
), project
is a logical group of tests running in the same browser (context). Since we only have one project, we don't need this.
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.
Removed this in 6395b2c
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.
Add light
to the snapshot names makes sense to me 👍
However, I do not believe we should remove the platform indication from the filename. You mention in the PR body that they make it hard to compare snapshots generated across platforms... but that is intentional, it's the reason the platform is indicated. All the major browsers behave differently between platforms, especially and importantly in font rendering. Indicating the platform the snapshot was generated under ensures that when comparisons do happen, there is no possibility for platform-specific variations to cause noise in the diff.
We run playwright tests inside the Docker container for that reason, to eliminate that noise. We really should not compare screenshots from different OSs. If there are devex issues with the Playwright container that make it necessary to run the visual regression tests outside the Playwright container such that the platform indication would not be linux, then we need to fix those issues. Making it easy to do a faulty comparison (screenshots across OSs) is not an appropriate solution to that problem.
This is a blocker because while we can easily revert the change and add the platform name back, doing this at all sets an incorrect precedence about what kinds of comparisons are valid, and could very easily cause significant confusion, and even introduce flakiness in CI. For example, if a screenshot is generated outside the container, with a different platform and browser version, and then were committed, small font rendering and layout variations could cause difficult to understand flakiness in the screenshots.
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.
Just reviewing the diffs of screenshots. Some storybook ones are actually changed, rather than just renamed. Almost all of these with changes have visible differences, and some seem incorrect, so I wanted to confirm they are intentional changes.
The button screenshots have a variety of differences. Some have significantly different colours, like frontend/test/storybook/visual-regression/v-button.spec.ts-snapshots/filled-pink-8-resting-linux.png
:
Some have the same colours, but show ~2px of difference in the width of the button, like frontend/test/storybook/visual-regression/v-button.spec.ts-snapshots/filled-dark-resting-linux.png
and frontend/test/storybook/visual-regression/v-button.spec.ts-snapshots/filled-pink-8-hovered-linux.png
(although the latter also has the colour difference issue).
By the name frontend/test/storybook/visual-regression/v-button.spec.ts-snapshots/filled-white-hovered-linux.png
and frontend/test/storybook/visual-regression/v-button.spec.ts-snapshots/transparent-dark-hovered-linux.png
would appear to be white and dark-transparent, but both don't match my expectations of what a white or transparent button would be (including in their versions on main
, so something is wrong here overall, maybe).
frontend/test/storybook/visual-regression/v-icon-button.spec.ts-snapshots/v-icon-button-sizes-linux.png
is completely broken and doesn't show the components anymore:
4bf0ffb
to
eba4a38
Compare
eba4a38
to
f120c95
Compare
bb6b148
to
466755e
Compare
f120c95
to
5d6d66e
Compare
Sorry for clicking "rerequest review" too early. I noticed that the storybook snapshots are off, and I extracted #4919 out of this PR. Since the Storybook tests had |
466755e
to
5615d29
Compare
5d6d66e
to
d11fbb0
Compare
d11fbb0
to
1db05ad
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.
This looks good. Every snapshot is renamed without changes, with the (correct) exception of the additions:
- frontend/test/storybook/visual-regression/v-icon-button.spec.ts-snapshots/v-icon-button-sizes-light-linux.png
- frontend/test/storybook/visual-regression/focus.spec.ts-snapshots/focus-colored-light-linux.png
Sounds good! LGTM |
7e1245e
to
718a383
Compare
1db05ad
to
1dbc832
Compare
b18fbaf
to
91b9295
Compare
2b2a017
to
85a0666
Compare
91b9295
to
c60ba27
Compare
c635aa1
to
7e5228a
Compare
7e5228a
to
c829801
Compare
Fixes
Related to #4305 by @zackkrida
Description
Most of the changes in this PR are renaming of the snapshots: from "...-linux.png" to "...-light.png". In the follow-up PR, the "-dark.png" snapshots will be added.
This PR removes thethis change was removed to prevent flakiness in the future, as per @sarayourfriend's comment below.-linux
platform suffix from the snapshot names. This will enable someone on a different system to easily compare the snapshots to the linux ones in the repository, instead of creating new snapshots that cannot be compared.Testing Instructions
The code changes should make sense, and the snapshots should only be renamed, not changed.
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