Skip to content
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.

Sign up for GitHub

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

Add e2e tests for keyboard interactions in DataViews ListView #61648

Merged
merged 4 commits into from
May 14, 2024
Merged

Add e2e tests for keyboard interactions in DataViews ListView #61648

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7764d8f
Implement e2e test for keyboard interactions on dataviews list view
oandregal May 14, 2024
21a87ee
Update test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js
oandregal May 14, 2024
f4b9d6d
Use toBeVisible instead of waitFor
oandregal May 14, 2024
90b54b9
Create pages for e2e tests
Mamaduka May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 189 additions & 0 deletions test/e2e/specs/site-editor/dataviews-list-layout-keyboard.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Dataviews List Layout', () => {
test.beforeAll( async ( { requestUtils } ) => {
// Activate a theme with permissions to access the site editor.
await requestUtils.activateTheme( 'emptytheme' );
} );

test.afterAll( async ( { requestUtils } ) => {
// Go back to the default theme.
await requestUtils.activateTheme( 'twentytwentyone' );
} );

test.beforeEach( async ( { admin, page } ) => {
// Go to the pages page, as it has the list layout enabled by default.
await admin.visitSiteEditor();
await page.getByRole( 'button', { name: 'Pages' } ).click();
} );

test( 'Items list is reachable via TAB', async ( { page } ) => {
// Start the sequence on the search component.
await page.getByRole( 'searchbox', { name: 'Search' } ).click();

// Tab until reaching the items list.
await page.keyboard.press( 'Tab' );
await expect(
page.getByRole( 'button', { name: 'Add filter' } )
).toBeFocused();

await page.keyboard.press( 'Tab' );
await expect(
page.getByRole( 'button', { name: 'Reset' } )
).toBeFocused();

await page.keyboard.press( 'Tab' );
await expect(
page.getByRole( 'button', { name: 'View options' } )
).toBeFocused();

// Make sure the items have loaded before reaching for the 1st item in the list.
await page.getByRole( 'grid' ).waitFor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The waitFor is "anti-pattern" in PW tests, and we should try to avoid it when possible. Can we swap similar calls with toBeVisible() assertions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to learn more about this and update it. I followed what we're doing in multiple other places: block directory, autosave, block bindings, multi-entity saving, editing widgets, etc. Could you expand on why is it an anti-pattern and in which situations should we avoid it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those are all copy pasted from the other tests or util methods. I should probably better to update those before it spreads even further :D

The waitFor isn't explicit, and it's unclear at a glance that the method also waits for the element to be visible. I think it's OK to use in test utils, where test flow or readability isn't essential.

The following examples do the same thing:

await page.getByRole( 'grid' ).waitFor()

// Explicit asertion. It's clear without a comment what the test is doing.
await expect( page.geByRole( 'grid' ) ).toBeVisible()

This is also highlighter in the e2e test best practices - https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/e2e/README.md#make-explicit-assertions.

cc @WunderBart, @kevin940726, in case I missed something :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an opinion on a way or the other, happy to change it if that's preferred f4b9d6d

It sounds like we should make a lint rule to prevent people from using what our existing tests do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, waitFor should be really rare, mostly in test utils. In Playwright, if we want to explicitly test the element is visible we should just use toBeVisible(). If we just want to wait for the element to load for whatever actions below, we shouldn't need to, Playwright auto-waits for us.

I don't think this is a major anti-pattern though, just a good-to-know tip and helps readability in the long term 😄. (Thank you @Mamaduka!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WunderBart, thanks for sharing the case.

I personally like the idea. Should we continue discussions in an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like about the approach is that you no longer can easily run the e2e tests by just providing a URL, you need access to CI and that database which makes running the e2e tests against Core (with the plugin disabled) for instance harder or remote urls or things like that. Thought one solution could be to provide this "reset DB" step as a REST API or something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka, should I handle creating the issue? 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don’t mind, @WunderBart 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#61719

await page.keyboard.press( 'Tab' );
await expect( page.getByLabel( 'Privacy Policy' ) ).toBeFocused();
} );

test( 'Navigates from items list to preview via TAB, and vice versa', async ( {
page,
} ) => {
// Start the sequence on the search component.
await page.getByRole( 'searchbox', { name: 'Search' } ).click();

// Tab until reaching the items list.
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );

// Make sure the items have loaded before reaching for the 1st item in the list.
await page.getByRole( 'grid' ).waitFor();
await page.keyboard.press( 'Tab' );
await expect( page.getByLabel( 'Privacy Policy' ) ).toBeFocused();

// Go to the preview.
await page.keyboard.press( 'Tab' );
await expect(
page
.getByRole( 'region', { name: 'Editor content' } )
.getByRole( 'button', { name: 'Edit' } )
).toBeFocused();

// Go back to the items list using SHIFT+TAB.
await page.keyboard.press( 'Shift+Tab' );
await expect( page.getByLabel( 'Privacy Policy' ) ).toBeFocused();
} );

test( 'Navigates the items list via UP/DOWN arrow keys', async ( {
page,
} ) => {
// Start the sequence on the search component.
await page.getByRole( 'searchbox', { name: 'Search' } ).click();

// Tab until reaching the items list.
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );

// Make sure the items have loaded before reaching for the 1st item in the list.
await page.getByRole( 'grid' ).waitFor();
await page.keyboard.press( 'Tab' );

// Use arrow up/down to move through the list.
await page.keyboard.press( 'ArrowDown' );
await expect( page.getByLabel( 'Sample Page' ) ).toBeFocused();

await page.keyboard.press( 'ArrowUp' );
await expect( page.getByLabel( 'Privacy Policy' ) ).toBeFocused();
} );

test( 'Actions are reachable via RIGHT/LEFT arrow keys', async ( {
page,
} ) => {
// Start the sequence on the search component.
await page.getByRole( 'searchbox', { name: 'Search' } ).click();

// Tab until reaching the items list.
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );

// Make sure the items have loaded before reaching for the 1st item in the list.
await page.getByRole( 'grid' ).waitFor();
await page.keyboard.press( 'Tab' );

// Use right/left arrow keys to move horizontally.
await page.keyboard.press( 'ArrowRight' );
await expect(
page
.getByRole( 'row', { name: 'Privacy Policy Edit Actions' } )
.getByLabel( 'Edit' )
).toBeFocused();

await page.keyboard.press( 'ArrowRight' );
await expect(
page
.getByRole( 'row', { name: 'Privacy Policy Edit Actions' } )
.getByLabel( 'Actions' )
).toBeFocused();

await page.keyboard.press( 'ArrowLeft' );
await expect(
page
.getByRole( 'row', { name: 'Privacy Policy Edit Actions' } )
.getByLabel( 'Edit' )
).toBeFocused();

await page.keyboard.press( 'ArrowLeft' );
await expect( page.getByLabel( 'Privacy Policy' ) ).toBeFocused();
} );

test( 'Navigates the list via UP/DOWN arrow keys from action buttons', async ( {
page,
} ) => {
// Start the sequence on the search component.
await page.getByRole( 'searchbox', { name: 'Search' } ).click();

// Tab until reaching the items list.
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );
await page.keyboard.press( 'Tab' );

// Make sure the items have loaded before reaching for the 1st item in the list.
await page.getByRole( 'grid' ).waitFor();
await page.keyboard.press( 'Tab' );

// Use arrow up/down to move through the list from the edit primary action button.
await page.keyboard.press( 'ArrowRight' );
await page.keyboard.press( 'ArrowDown' );
await expect(
page
.getByRole( 'row', { name: 'Sample Page Edit Actions' } )
.getByLabel( 'Edit' )
).toBeFocused();

await page.keyboard.press( 'ArrowUp' );
await expect(
page
.getByRole( 'row', { name: 'Privacy Policy Edit Actions' } )
.getByLabel( 'Edit' )
).toBeFocused();

// Use arrow up/down to move through the list from the all actions button.
await page.keyboard.press( 'ArrowRight' );
await page.keyboard.press( 'ArrowDown' );
await expect(
page
.getByRole( 'row', { name: 'Sample Page Edit Actions' } )
.getByLabel( 'Actions' )
).toBeFocused();

await page.keyboard.press( 'ArrowUp' );
await expect(
page
.getByRole( 'row', { name: 'Privacy Policy Edit Actions' } )
.getByLabel( 'Actions' )
).toBeFocused();
} );
} );
Loading