-
Notifications
You must be signed in to change notification settings - Fork 583
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
tests: add missing snapshot and axe coverage for ActionMenu #4538
Conversation
|
size-limit report 📦
|
fdf4f7d
to
08e63e1
Compare
@@ -36,30 +37,29 @@ test.describe('ActionMenu', () => { | |||
test.describe(theme, () => { |
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 noticed that this test is under Inactive items
but it's visiting the wrong ID, -links-and-actions
which is actually a duplicate of another test. Updated!
c5cda68
to
65e5b1e
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 snapshots changed for ActionList bcuz Jonathan's profile picture changed 😅
I'm assuming the failing tests are expected because the snapshots changed, and will need to be merged in for it to pass. |
@khiga8 that's a great point about the CI failing, ideally with |
@@ -46,7 +46,9 @@ test.describe('ActionMenu', () => { | |||
// Open state | |||
await page.locator('button', {hasText: 'Open menu'}).waitFor() | |||
await page.getByRole('button', {name: 'Open menu'}).click() | |||
expect(await page.screenshot()).toMatchSnapshot(`ActionMenu.Inactive Items.${theme}.png`) | |||
expect(await page.screenshot({animations: 'disabled'})).toMatchSnapshot( |
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.
Aha! Thank you @joshblack! 🌟
bump @mperrotti when you have a sec to review 👀 |
4931bfa
to
c15d0f9
Compare
* tests: add missing axe coverage for open ActionMenu * Update test * test(vrt): update snapshots * test(vrt): update snapshots * test(vrt): update snapshots * test(ActionMenu): disable animations for snapshot test --------- Co-authored-by: khiga8 <[email protected]> Co-authored-by: Josh Black <[email protected]>
Fixes: #4530
Changelog
I noticed that most of the snapshots test and axe scan coverage for ActionMenu are for the default closed state, which is just a button.
This PR adds an action to open the ActionMenu to ensure we're getting adequate visual regression and axe testing coverage, and regenerates the snapshots.
Rollout strategy
Testing & Reviewing
Merge checklist