-
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
Enable running the snapshot tests for both color modes #4926
Conversation
eecfabd
to
9cfa522
Compare
9c89d08
to
201f53f
Compare
5a985d6
to
6a486ad
Compare
201f53f
to
56eb502
Compare
6a486ad
to
c635aa1
Compare
72b29c5
to
7d44ee8
Compare
c635aa1
to
7e5228a
Compare
8d08737
to
3becb32
Compare
Full-stack documentation: https://docs.openverse.org/_preview/4926 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
7e5228a
to
c829801
Compare
82e3c7a
to
6bcc141
Compare
6bcc141
to
bf04a21
Compare
bd11e2e
to
effb9d2
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.
👍 from me! This is great, the refactors are nice and all make sense to me in addition to the new expect.soft
usage. Maybe there is a way to get rid of the innerExpectSnapshot
and multiple getSnapshotName
layers, but I'm not too worried about it and didn't have any immediate ideas, that didn't just involve passing through more parameters and having a bigger final getSnapshotName
with more conditions, so it's all good to me!
Nice work 🚀 I left some questions and one whitespace nitpick but nothing blocking from my perspective.
onMounted(() => { | ||
document.body.classList.add("bg-default") | ||
|
||
watch( |
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 curiousity) What is the reason to have these watch
calls inside onMounted
? Does this effectively delay starting the watching until after the component is ready?
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 just used the same structure as the withRtl
decorator. After your comment, I went back and refactored this. Moved the watch
calls out of onMounted
. I also found a way of using useGlobals
(it didn't work when I try running it in the onMounted
, but worked when I moved it out of it), so now the global theme is updated correctly when the Openverse theme switcher value is changed.
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'll make an issue to also refactor the withRtl
decorator so that when dir
is changed using Openverse footer, the global languageDirection
is updated, similar to how with-theme
works now.
} | ||
export type ExpectSnapshot = <T extends Locator | Page>( |
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.
nit: Whitespace to help break up the file's components
} | |
export type ExpectSnapshot = <T extends Locator | Page>( | |
} | |
export type ExpectSnapshot = <T extends Locator | Page>( |
@@ -102,6 +102,7 @@ export const project: TSESLint.Linter.ConfigType = { | |||
"expectEventPayloadToMatch", | |||
// Shared assertion for visual regression tests | |||
"expectSnapshot", | |||
"expectScreenshotAreaSnapshot", |
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 couldn't find this used anywhere. Is it left over from a previous version, and do we still need to include it?
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 used in storybook visual regression tests, e.g. frontend/test/storybook/visual-regression/v-checkbox.spec.ts
. It makes testing the screenshot area (instead of the full page) easier.
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.
Ah! A diff must have been collapsed when I "ctrl + f" searched for it. Sorry!
07def70
to
14069a0
Compare
Fixes
Extracted from #4915
Related to #4305 by @zackkrida
Description
This is one idea of how we can implement testing both dark and light theme for visual regressions without running each test twice. It relies on the
expectSnapshot
function, which now runs 2 snapshot comparisons instead of one.First, the default snapshot is taken. Then, the theme switcher is used to change the color mode, and the second snapshot is taken. To make sure that we always run both snapshot checks, even if the first one fails, we use the
expect.soft
that runs the test to the end, but fails if one of theexpect
s in the test fails.We use the theme switcher to change the color mode. To do this, we add a
themeSwitcher
to all stories using a decorator, and use the footer switcher for the tests that use the Nuxt app.expectSnapshot
function is extracted from thebreakpoints.ts
file. It needs thepage
argument from Playwright test to allow locating the theme switcher. The optional (nullable) parameters were combined to a separateoptions
object, which allows passing only the necessary options instead of having to pass{}
blank object for the unused one.This PR also adds the theme switcher to the Storybook toolbar, so now you can view both color modes. The Storybook switcher's state is synced with the state of our own theme switcher in the story.
The image cell snapshots had to be updated. I made the width smaller to match the real size of images in the grid, and to make the snapshot files smaller, and to fit all of them into a single screen height. View updated image cell story
Testing Instructions
Go to Storybook and see check that the color theme switcher is working. Also, check the follow-up PRs that add the dark snapshots.
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