Avatar: Add test to ensure that user image updates when user switched. #42657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hes when set to a new user.
|
Size Change: -19.8 kB (-2%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
talldan
reviewed
Aug 10, 2022
There was a problem hiding this comment.
Thanks for contributing tests! (sorry for the slow review)
I left a few comments, but there shouldn't be anything challenging. The main thing will be agreeing on the right way to implement the user utils, as there are a couple of PRs trying to do the same thing slightly differently.
| * @param this | ||
| * @param userId The ID of the user. | ||
| */ | ||
| async function deleteUser( this: RequestUtils, userId: number ) { |
There was a problem hiding this comment.
#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.
There was a problem hiding this comment.
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.
| */ | ||
| async function deleteUser( this: RequestUtils, userId: number ) { | ||
| // Do not delete main user account. | ||
| if ( userId === 1 ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Comment on lines
11
to
18
| export interface UserData { | ||
| username: string; | ||
| email: string; | ||
| firstName: string; | ||
| lastName: string; | ||
| password: string; | ||
| roles: string[]; | ||
| } |
There was a problem hiding this comment.
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;
Comment on lines
50
to
55
| username: user.username, | ||
| email: user.email, | ||
| first_name: user.firstName, | ||
| last_name: user.lastName, | ||
| password: user.password, | ||
| roles: user.roles, |
There was a problem hiding this comment.
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 responses; | ||
| } | ||
|
|
||
| export { listUsers, createUser, deleteAllUsers, deleteUser }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good, I can leave them as private members of the users.ts file.
| const username = 'Gravatar Gravatar'; | ||
|
|
||
| const avatarBlock = page.locator( | ||
| 'role=document[name="Block: Avatar"]' |
There was a problem hiding this comment.
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.
| 'role=document[name="Block: Avatar"]' | ||
| ); | ||
|
|
||
| await expect( avatarBlock ).toBeVisible(); |
There was a problem hiding this comment.
This line shouldn't be needed, as there's already await expect( avatarImage ).toBeVisible(); below, which will assert that the block is visible.
| 'role=option[name="' + username + '"]' | ||
| ); | ||
|
|
||
| await expect( newUser ).toBeVisible(); |
There was a problem hiding this comment.
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
|
|
||
| expect( newSrc ).not.toBe( originalSrc ); | ||
|
|
||
| await expect( userInput ).toBeVisible(); |
There was a problem hiding this comment.
I don't think this final assertion is needed.
Comment on lines
52
to
68
| 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(); |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Sounds good, thank you for the info.
…s that are clicked.
…e checks. Only use accessible selectors.
|
@talldan Thank you for the review and info! I believe I have addressed all of your points. |
talldan
approved these changes
Aug 25, 2022
talldan
added
the
[Type] Automated Testing
cbravobernal
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #39645.
What?
Add e2e tests in Playwright for the avatar block. When a user is switched check if the image src updates as well.
Why?
Adds e2e tests for the avatar block to ensure that we are always properly updating the image src.
How?
Adds user helper utilities to the
@wordpress/e2e-test-utils-playwrightfor adding users and cleaning up added users. This is used to add a sample user, Gravatar Gravatar, The new user is selected from the user combo box and the original source is checked against the new source after the user has been updated.Testing Instructions
npm inpm run buildnpm run test-e2e:playwright -- editor/blocks/avatar.spec.jsScreenshots or screencast
Not applicable.