[Chore] VRT fixes#9721
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates EUI’s Storybook visual regression test runner to snapshot each story across multiple “variants” (currently desktop + mobile viewports), and documents how variant baselines are named and how to opt out per-story.
Changes:
- Run VRT screenshots in a loop over defined variants (desktop/mobile) and suffix baseline names with the variant (e.g.
…-desktop.png,…-mobile.png). - Remove
packages/eui/playwright.config.ts, which is not used by the currenttest-storybook-based VRT runner. - Expand the VRT wiki docs with variant behavior and
parameters.vrt.skipVariants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wiki/contributing-to-eui/testing/visual-regression-testing.md | Documents variant baselines and how to skip variants per story |
| packages/eui/playwright.config.ts | Removes unused Playwright config file |
| packages/eui/.storybook/test-runner.ts | Implements per-variant viewport iteration and per-variant snapshot IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d4ef603 to
1fb106d
Compare
e26d9ec to
237fa42
Compare
63173bf to
99f4f96
Compare
316dbb0 to
02c55cb
Compare
a46e54b to
40f47ac
Compare
d944a28 to
da05bff
Compare
2 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuicomment (1 difference)
euidatagrid (1 difference)
|
|
buildkite test this 1 (after green) |
|
buildkite test this 2 (after green) |
|
buildkite test this 3 (after green) |
|
buildkite test this 4 (after green) |
| # encounters a story without one. Commit those net-new baselines now, | ||
| # *regardless* of whether VRT overall passed or failed. | ||
| new_files="$(git ls-files --others --exclude-standard -- "${REF_DIR}")" | ||
| if [[ -n "${new_files}" ]]; then |
There was a problem hiding this comment.
Good thinking, this is much nicer 🎉
| @@ -159,7 +159,7 @@ export const VirtualizedCodeBlockScrolling: Story = { | |||
| tags: ['vrt-only'], | |||
| parameters: { | |||
| vrt: { | |||
| selector: VRT_SELECTORS.portal, | |||
| skip: true, | |||
There was a problem hiding this comment.
❓ I understand that the purpose is to skip these stories for now and get back to them.
Do you think it's valid to add a comment stating this anyway according to our convention, or will this be done in the very near future and not meaningful anyway?
There was a problem hiding this comment.
I don't think it's necessary because my plan is to immediately try to stabilize these after this PR is merged. So skips are temporary.
6 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuisuperdatepicker (6 differences)
|
6 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuisuperdatepicker (6 differences)
|
💚 Build Succeeded
History
|
💚 Build Succeeded
History
|
























Summary
Several issues after having merged #9594.
Still a couple of flakes
I think some of the flakes like
EuiPopoverPlayground might be caused by font subpixel positioning and there's a Chromium flag to disable it:--disable-font-subpixel-positioning.For now, I want to skip these flakes so that they are not blocking PRs:
EuiPopover > PlaygroundEuiDataGrid > Column selector(similar as ☝🏻)EuiDataGrid > CompactEuiCodeBlock > Virtualized code block scrollingLack of
mobilesnapshotsTurns out
playwright.config.tsisn't picked up by the test-runner and is dead code but what's more curious is there's conditional logic for the viewport in the test runner. Not sure what I was thinking at the time.Added an iteration through "variants". Called them generically because although it's only viewport for now, we may extend it (dark mode, high contrast mode etc.).
No add commit on failed VRT step
If the VRT step fails, we do not add
chore(eui): add VRT baseline screenshots, we fold the new snapshots underchore(eui): update VRT baseline screenshots.So PRs that mixed new stories with diffs on existing ones OR encountered flakes silently lost the new baselines (e.g.
PaginationExampleon #9726). Now we commit new baselines regardless of VRT outcome.Cancel intermediate builds
You can read more about it here. The idea is to cancel the original build which keeps running in parallel and will e.g. comment stale visual diff.
QA instructions for reviewer
skip: trueskip: ['mobile']