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

Jump to bottom

Navigation: e2e tests: default to classic menu #47867

Merged
merged 15 commits into from
Feb 13, 2023
Merged

Navigation: e2e tests: default to classic menu #47867

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f26c7a0
added create and delete navigation functions
MaggieCabrera Feb 3, 2023
08fae6e
check markup of the block
MaggieCabrera Feb 3, 2023
36f04bb
checked the block in the canvas and frontend too
MaggieCabrera Feb 3, 2023
e46bbd5
removed pause, passed page as an argument to the test
MaggieCabrera Feb 6, 2023
dd0c1ca
Update test/e2e/specs/editor/blocks/navigation.spec.js
MaggieCabrera Feb 6, 2023
de0272b
commented on the theme selection
MaggieCabrera Feb 6, 2023
a129cc1
use toMatchObject to check the block instead of toEqual
MaggieCabrera Feb 7, 2023
b418078
change css selectors to use role instead
MaggieCabrera Feb 7, 2023
3e3d551
make string check case insensitive
MaggieCabrera Feb 8, 2023
6f78851
moved utils to utils package
MaggieCabrera Feb 8, 2023
574334d
undo regression from rebase
MaggieCabrera Feb 8, 2023
8acf0fd
create classic menu util
MaggieCabrera Feb 10, 2023
2a440cc
check that the block is visible
MaggieCabrera Feb 10, 2023
0dab307
reorder utils and finish the selector checks for the test
MaggieCabrera Feb 10, 2023
4d4d736
renaming variables and custom link label
MaggieCabrera Feb 13, 2023
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
8 changes: 8 additions & 0 deletions packages/e2e-test-utils-playwright/src/request-utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ import { activateTheme } from './themes';
import { deleteAllBlocks } from './blocks';
import { createComment, deleteAllComments } from './comments';
import { createPost, deleteAllPosts } from './posts';
import {
createClassicMenu,
createNavigationMenu,
deleteAllMenus,
} from './menus';
import { resetPreferences } from './preferences';
import { getSiteSettings, updateSiteSettings } from './site-settings';
import { deleteAllWidgets, addWidgetBlock } from './widgets';
Expand Down Expand Up @@ -125,6 +130,9 @@ class RequestUtils {
deleteAllBlocks = deleteAllBlocks;
createPost = createPost.bind( this );
deleteAllPosts = deleteAllPosts.bind( this );
createClassicMenu = createClassicMenu.bind( this );
createNavigationMenu = createNavigationMenu.bind( this );
deleteAllMenus = deleteAllMenus.bind( this );
createComment = createComment.bind( this );
deleteAllComments = deleteAllComments.bind( this );
deleteAllWidgets = deleteAllWidgets.bind( this );
Expand Down
108 changes: 108 additions & 0 deletions packages/e2e-test-utils-playwright/src/request-utils/menus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/**
* Internal dependencies
*/
import type { RequestUtils } from './index';

export interface MenuData {
title: string;
content: string;
}
export interface Menu {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the name Navigation?

Copy link
Contributor

Choose a reason for hiding this comment

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

navigationMenus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose menu because it is used both for classic and navigation menus, but I think we can go with NavigationMenu too.

id: number;
content: string;
status: 'publish' | 'future' | 'draft' | 'pending' | 'private';
}

/**
* Create a classic menu
*
* @param {string} name Menu name.
* @return {string} Menu content.
*/
export async function createClassicMenu( this: RequestUtils, name: string ) {
const menuItems = [
{
title: 'Home',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite a generic name. Is it possible that the test could pass but because it found a different navigation with a "Home" option? Maybe its safer to use a less generic name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. This function was way more complicated on the puppeteer tests, so I made it much simpler for this test, and maybe we can refactor it later if it's useful for other tests too.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I've found this to be a problem in the past. I agree with Ben it should use a page name that is unusual and quite long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to "Custom link", shall I make it longer?

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be good enough 👍

url: 'http://localhost:8889/',
type: 'custom',
menu_order: 1,
},
];

const menu = await this.rest< Menu >( {
method: 'POST',
path: `/wp/v2/menus/`,
data: {
name,
},
} );

if ( menuItems?.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed? Isn't menuItems statically defined above?

await this.batchRest(
menuItems.map( ( menuItem ) => ( {
method: 'POST',
path: `/wp/v2/menu-items`,
body: {
menus: menu.id,
object_id: undefined,
...menuItem,
parent: undefined,
},
} ) )
);
}

return menu;
}

/**
* Create a navigation menu
*
* @param {Object} menuData navigation menu post data.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Prefer TS types than JSDoc types. I think we can just omit the type here.

 * @param  menuData navigation menu post data.

* @return {string} Menu content.
*/
export async function createNavigationMenu(
this: RequestUtils,
menuData: MenuData
) {
return this.rest( {
method: 'POST',
path: `/wp/v2/navigation/`,
data: {
status: 'publish',
...menuData,
},
} );
}

/**
* Delete all navigation and classic menus
*
*/
export async function deleteAllMenus( this: RequestUtils ) {
const navMenus = await this.rest< Menu[] >( {
path: `/wp/v2/navigation/`,
} );

if ( navMenus?.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It's always going to return an array so let's remove the optional chaining here :)

await this.batchRest(
navMenus.map( ( menu ) => ( {
method: 'DELETE',
path: `/wp/v2/navigation/${ menu.id }?force=true`,
} ) )
);
}

const classicMenus = await this.rest< Menu[] >( {
path: `/wp/v2/menus/`,
} );

if ( classicMenus?.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

await this.batchRest(
classicMenus.map( ( menu ) => ( {
method: 'DELETE',
path: `/wp/v2/menus/${ menu.id }?force=true`,
} ) )
);
}
}
111 changes: 56 additions & 55 deletions test/e2e/specs/editor/blocks/navigation.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.use( {
navBlockUtils: async ( { page, requestUtils }, use ) => {
await use( new NavigationBlockUtils( { page, requestUtils } ) );
navBlockUtils: async ( { editor, page, requestUtils }, use ) => {
await use( new NavigationBlockUtils( { editor, page, requestUtils } ) );
},
} );

Expand All @@ -17,16 +17,13 @@ test.describe(
await requestUtils.activateTheme( 'twentytwentythree' );
} );

test.beforeEach( async ( { admin, navBlockUtils } ) => {
await Promise.all( [
navBlockUtils.deleteAllNavigationMenus(),
admin.createNewPost(),
] );
test.beforeEach( async ( { requestUtils } ) => {
await Promise.all( [ requestUtils.deleteAllMenus() ] );
} );

test.afterAll( async ( { requestUtils, navBlockUtils } ) => {
test.afterAll( async ( { requestUtils } ) => {
await Promise.all( [
navBlockUtils.deleteAllNavigationMenus(),
requestUtils.deleteAllMenus(),
requestUtils.activateTheme( 'twentytwentyone' ),
] );
} );
Expand All @@ -36,8 +33,10 @@ test.describe(
} );

test( 'default to a list of pages if there are no menus', async ( {
admin,
editor,
} ) => {
await admin.createNewPost();
await editor.insertBlock( { name: 'core/navigation' } );

const pageListBlock = editor.canvas.getByRole( 'document', {
Expand All @@ -63,24 +62,23 @@ test.describe(
} );

test( 'default to my only existing menu', async ( {
admin,
editor,
page,
requestUtils,
navBlockUtils,
} ) => {
const createdMenu = await navBlockUtils.createNavigationMenu( {
await admin.createNewPost();
const createdMenu = await requestUtils.createNavigationMenu( {
title: 'Test Menu 1',
content:
'<!-- wp:navigation-link {"label":"WordPress","type":"custom","url":"http://www.wordpress.org/","kind":"custom","isTopLevelLink":true} /-->',
} );

await editor.insertBlock( { name: 'core/navigation' } );

//check the block in the canvas.
await expect(
editor.canvas.locator(
'role=textbox[name="Navigation link text"i] >> text="WordPress"'
)
).toBeVisible();
// Check the block in the canvas.
await navBlockUtils.selectNavigationItemOnCanvas( 'WordPress' );

// Check the markup of the block is correct.
await editor.publishPost();
Expand All @@ -92,13 +90,34 @@ test.describe(
] );
await page.locator( 'role=button[name="Close panel"i]' ).click();

//check the block in the frontend.
await page.goto( '/' );
await expect(
page.locator(
'role=navigation >> role=link[name="WordPress"i]'
)
).toBeVisible();
// Check the block in the frontend.
await navBlockUtils.selectNavigationItemOnFrontend( 'WordPress' );
} );

test( 'default to the only existing classic menu if there are no block menus', async ( {
admin,
editor,
requestUtils,
navBlockUtils,
} ) => {
// Create a classic menu.
await requestUtils.createClassicMenu( 'Test Classic 1' );
await admin.createNewPost();

await editor.insertBlock( { name: 'core/navigation' } );
// We need to check the canvas after inserting the navigation block to be able to target the block.
const navigationBlock = editor.canvas.locator(
'role=document[name="Block: Navigation"i]'
);
await expect( navigationBlock ).toBeVisible();
MaggieCabrera marked this conversation as resolved.
Show resolved Hide resolved

// Check the block in the canvas.
await editor.page.pause();
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, again!

await navBlockUtils.selectNavigationItemOnCanvas( 'Home' );

// Check the block in the frontend.
await navBlockUtils.selectNavigationItemOnFrontend( 'Home' );
await editor.page.pause();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

} );
}
);
Expand All @@ -110,39 +129,21 @@ class NavigationBlockUtils {
this.requestUtils = requestUtils;
}

/**
* Create a navigation menu
*
* @param {Object} menuData navigation menu post data.
* @return {string} Menu content.
*/
async createNavigationMenu( menuData ) {
return this.requestUtils.rest( {
method: 'POST',
path: `/wp/v2/navigation/`,
data: {
status: 'publish',
...menuData,
},
} );
async selectNavigationItemOnCanvas( name ) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer inlining one-liner assertions like this, makes the test a bit easier to read. selectNavigationItemOnCanvas doesn't really select anything here anyway 😆 .

await expect(
this.editor.canvas.locator(
`role=textbox[name="Navigation link text"i] >> text="${ name }"`
)
).toBeVisible();
}

/**
* Delete all navigation menus
*
*/
async deleteAllNavigationMenus() {
const menus = await this.requestUtils.rest( {
path: `/wp/v2/navigation/`,
} );

if ( ! menus?.length ) return;

await this.requestUtils.batchRest(
menus.map( ( menu ) => ( {
method: 'DELETE',
path: `/wp/v2/navigation/${ menu.id }?force=true`,
} ) )
);
async selectNavigationItemOnFrontend( name ) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

await this.page.goto( '/' );
await this.editor.page.pause();
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

await expect(
this.page.locator(
`role=navigation >> role=link[name="${ name }"i]`
)
).toBeVisible();
}
}