Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
E2E Tests: Add test for avatar block to ensure that user avatar switc…
…hes when set to a new user.
  • Loading branch information
BE-Webdesign committed Jul 23, 2022
commit f1f4c782a281e97e79f4fcee7e1b4538a3cf17d8
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { WP_ADMIN_USER, WP_BASE_URL } from '../config';
import type { User } from './login';
import { login } from './login';
import { listMedia, uploadMedia, deleteMedia, deleteAllMedia } from './media';
import { listUsers, deleteUser, createUser, deleteAllUsers } from './users';
import { setupRest, rest, getMaxBatchSize, batchRest } from './rest';
import { getPluginsMap, activatePlugin, deactivatePlugin } from './plugins';
import { deleteAllTemplates } from './templates';
Expand Down Expand Up @@ -133,6 +134,10 @@ class RequestUtils {
uploadMedia = uploadMedia.bind( this );
deleteMedia = deleteMedia.bind( this );
deleteAllMedia = deleteAllMedia.bind( this );
listUsers = listUsers.bind( this );
createUser = createUser.bind( this );
deleteUser = deleteUser.bind( this );
deleteAllUsers = deleteAllUsers.bind( this );
}

export type { StorageState };
Expand Down
102 changes: 102 additions & 0 deletions packages/e2e-test-utils-playwright/src/request-utils/users.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/**
* Internal dependencies
*/
import type { RequestUtils } from './index';

export interface User {
id: number;
email: string;
}

export interface UserData {
username: string;
email: string;
firstName: string;
lastName: string;
password: string;
roles: string[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not every property is required by the REST API. I think email is the only one. A question mark can be used to mark something as optional:

lastName?: string;


/**
* List all users.
*
* @see https://developer.wordpress.org/rest-api/reference/users/#list-users
* @param this
*/
async function listUsers( this: RequestUtils ) {
const response = await this.rest< User[] >( {
method: 'GET',
path: '/wp/v2/users',
params: {
per_page: 100,
},
} );

return response;
}

/**
* Add a test user.
*
* @see https://developer.wordpress.org/rest-api/reference/users/#create-a-user
* @param this
* @param user User data to create.
*/
async function createUser( this: RequestUtils, user: UserData ) {
const response = await this.rest< User >( {
method: 'POST',
path: '/wp/v2/users',
data: {
username: user.username,
email: user.email,
first_name: user.firstName,
last_name: user.lastName,
password: user.password,
roles: user.roles,
Copy link
Contributor

Choose a reason for hiding this comment

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

As only some properties are required, I wonder whether it's ok to pass all properties like this even if some are undefined, or whether any undefined properties needs to be omitted first.

Note that there's also other implementations of this in open PRs #43064 and #42695, so we'll need some agreement on the right way to do this before merging.

},
} );

return response;
}

/**
* Delete a user.
*
* @see https://developer.wordpress.org/rest-api/reference/users/#delete-a-user
* @param this
* @param userId The ID of the user.
*/
async function deleteUser( this: RequestUtils, userId: number ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

#43064 is also implementing this function, and generally I prefer the way that version allows deleting a single user by passing a username.

It's a difficult line to tread, whether the function is an exact facsimile of the REST API, or whether there are some convenience features built-in to make writing tests easier.

Though perhaps in both cases deleteAllUsers is the only function needed.

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 think only exposing deleteAllUsers is a good approach. If somewhere down the road an individual deleteUser needs to be surfaced for testing, that can happen then.

// Do not delete main user account.
if ( userId === 1 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is more useful in deleteAllUsers than in deleteUser. Maybe filter users before the map call.

If any test does try deleting the main user directly it'd be a big case of the test doing it wrong, and I imagine it'd be quite noticeable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can move the check into the deleteAllUsers. Now that deleteUser is not exported, it will not be accessible where someone could accidentally delete the main user.

return new Promise( ( resolve ) => {
resolve( true );
} );
}

const response = await this.rest( {
method: 'DELETE',
path: `/wp/v2/users/${ userId }`,
params: { force: true, reassign: 1 },
} );

return response;
}

/**
* Delete all users except main root user.
*
* @param this
*/
async function deleteAllUsers( this: RequestUtils ) {
const users = await this.listUsers();

// The users endpoint doesn't support batch request yet.
const responses = await Promise.all(
users.map( ( user ) => this.deleteUser( user.id ) )
);

return responses;
}

export { listUsers, createUser, deleteAllUsers, deleteUser };
Copy link
Contributor

@talldan talldan Aug 10, 2022

Choose a reason for hiding this comment

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

It'd be good to only export the functions that are used in tests. I'm not sure listUsers will be needed, and possibly deleteUser might not be needed either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I can leave them as private members of the users.ts file.

87 changes: 87 additions & 0 deletions test/e2e/specs/editor/blocks/avatar.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* WordPress dependencies
*/

const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Avatar', () => {
test.beforeAll( async ( { requestUtils } ) => {
await requestUtils.deleteAllUsers();
} );

test.beforeEach( async ( { admin, requestUtils } ) => {
await requestUtils.createUser( {
username: 'user',
email: 'gravatartest@gmail.com',
firstName: 'Gravatar',
lastName: 'Gravatar',
roles: [ 'author' ],
password: 'gravatargravatar123magic',
} );

await admin.createNewPost();
} );

test.afterEach( async ( { requestUtils } ) => {
await requestUtils.deleteAllUsers();
} );
Comment on lines +8 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to structure this as:

  • beforeAll - createUser
  • afterAll - deleteAllUsers


test( 'should change image when user is changed', async ( {
editor,
page,
} ) => {
// Inserting a quote block
await editor.insertBlock( {
name: 'core/avatar',
} );

const username = 'Gravatar Gravatar';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the variable could be called userFullname to be more accurate.


const avatarBlock = page.locator(
'role=document[name="Block: Avatar"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'role=document[name="Block: Avatar"]'
'role=document[name="Block: Avatar"i]'

The i flag is good, it makes it case insensitive. That's useful as if the casing is changed in the code the test will still pass.

);

await expect( avatarBlock ).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

This line shouldn't be needed, as there's already await expect( avatarImage ).toBeVisible(); below, which will assert that the block is visible.


const avatarImage = avatarBlock.locator( 'img' );

await expect( avatarImage ).toBeVisible();

const originalSrc = await avatarImage.getAttribute( 'src' );

const blockInspectorControls = page.locator(
'.block-editor-block-inspector'
);

await expect( blockInspectorControls ).toBeVisible();

const userComboBox = blockInspectorControls.locator(
'.components-combobox-control'
);

await expect( userComboBox ).toBeVisible();

const userInput = userComboBox.locator(
'.components-combobox-control__input'
);

await expect( userInput ).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, there's no need to worry about all the intermediate elements being visible. An action like click will implicitly check that an element is visible.

Another thing is that it's good to prefer role selectors over css selectors. This practice will go some way to helping make sure Gutenberg is using accessible HTML.

You should be able to achieve the same sort of thing by doing:

const userInput = page.locator( 'role=region[name="Editor settings"i] >> role=combobox[name="User"i]' );
await userInput.click()

The accessibility inspector in the browser dev tools is pretty good for figuring this stuff out:
Screen Shot 2022-08-10 at 2 19 00 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thank you for the info.


// Set new user.
await userInput.click();

const newUser = userComboBox.locator(
'role=option[name="' + username + '"]'
);

await expect( newUser ).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

Actions like click will always implicitly wait for an element to be visible in Playwright, so there's no need to make the assertion here.

There's a handy table showing how that works here - https://playwright.dev/docs/actionability


await newUser.click();

const newSrc = await avatarImage.getAttribute( 'src' );

expect( newSrc ).not.toBe( originalSrc );

await expect( userInput ).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this final assertion is needed.

} );
} );