E2E Tests: Migrate Comments block test to Playwright #39826
Conversation
| @@ -0,0 +1,32 @@ | |||
| /** | |||
There was a problem hiding this comment.
| params: { | ||
| per_page: 100, | ||
| // All possible statuses. | ||
| status: 'unapproved,approved,spam,trash', |
There was a problem hiding this comment.
|
Size Change: -215 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
migrate/comments-query-loop-e2e-to-playwright
branch
from
20bb3a2 to
e532479
Compare
|
Rebased (to include #39818, which has now been merged). |
ockham
force-pushed
the
migrate/comments-query-loop-e2e-to-playwright
branch
3 times, most recently
from
408ccf1 to
884a2f3
Compare
ockham
force-pushed
the
migrate/comments-query-loop-e2e-to-playwright
branch
from
884a2f3 to
51ec368
Compare
ockham
changed the title
|
This PR is currently blocked because of the following problem: In The problem is however that this kind of navigation isn't allowed in (I'm not sure if Some potential solutions and/or workarounds I can think of:
Maybe our Playwright experts in residence can advise here 😅 cc/ @kevin940726 @noisysocks |
|
I had the same question before, but the Playwright team convinced me ;)
Going to each individual page to set those options is already a performance bottleneck 😅. Doing this per test case is not that bad as a temporary solution IMO.
Believe it or not, we don't have a REST API for logging in a user either 😆. We used to do something similar in Puppeteer to create a plugin to help log the test user in. We don't have REST APIs for activating or deactivating themes either. What we do is open the network panel in the devtool to find the undocumented requests. I think we can try similar things here for setting the settings. If that doesn't work, I think it might not be a bad idea to start creating an official API for that? For a temporary solution before that is merged, we can always change |
|
Thanks for your reply @kevin940726! It's quite helpful to get a better grasp of how things are done with Playwright (and how we can work around the shortcomings of WP's REST API).
I'm afraid there really might not be an endpoint for the settings; the wp-admin screen just
Yeah, I think it'd be a good idea to expose these things through the REST API 👍
Ah, that's great, I didn't think of that! I gave it a try locally, seems to work fine! Thanks a lot! 🙌 |
ockham
force-pushed
the
migrate/comments-query-loop-e2e-to-playwright
branch
from
48331d7 to
088c372
Compare
This reverts commit 088c372.
|
Okay, I’m setting this to „Ready for review“. I really like how Playwright allowed me to get rid of a bunch of boilerplate and overhead; it makes e2e tests a lot easier on the eye! |
|
Thanks @c4rl0sbr4v0! I'm going to merge this, hoping that this will fix the intermittent errors we were seeing with the Puppeteer-based tests (see e.g. #42164), and to prevent this from getting stale in case we want to add or modify e2e tests for the Comments block with some other PR(s). I'd be grateful if @kevin940726 could maybe have a look and leave some notes if there's anything that isn't following best practices; I'd be happy to address in a follow-up 😊 |
| */ | ||
| import type { Admin } from './'; | ||
|
|
||
| export async function setOption( this: Admin, setting: string, value: string ) { |
There was a problem hiding this comment.
Personally, I wouldn't put this inside e2e-test-utils-playwright since it's just a temporary solution for one test suite. Breaking changes will be made once the REST options API is implemented.
Instead, I would put this inside the comments test file to make it clear that it's an internal API.
| @@ -29,4 +29,9 @@ export async function publishPost( this: Editor ) { | |||
| await this.page.click( | |||
| 'role=region[name="Editor publish"i] >> role=button[name="Publish"i]' | |||
| ); | |||
|
|
|||
| const urlString = await this.page.inputValue( 'text="Post address"' ); | |||
There was a problem hiding this comment.
Normally, we would prefer role selectors over text selectors if they're available.
| this.rest( { | ||
| method: 'POST', | ||
| path: '/wp/v2/comments', | ||
| data: comment, |
There was a problem hiding this comment.
I don't think we have the id field of the comment data yet before creating it?
There was a problem hiding this comment.
Also, why couldn't we call /wp/v2/users/me here to get the current user id to pass it into the data? So that we don't have to create another util getCurrentUser for this.
| * @param {} comment Comment. | ||
| */ | ||
| export async function createComment( this: RequestUtils, comment: Comment ) { | ||
| this.rest( { |
There was a problem hiding this comment.
Missing await here?
We should probably return the response of the API too to capture the created comment id.
| await admin.createNewPost(); | ||
| await editor.insertBlock( { name: 'core/comments' } ); | ||
| await expect( | ||
| await page.locator( 'text="No results found."' ) |
There was a problem hiding this comment.
locator doesn't return a promise.
|
|
||
| // We check that there is a previous comments page link. | ||
| await expect( | ||
| await page.locator( 'text="Older Comments"' ) |
There was a problem hiding this comment.
locator doesn't return a promise.
| // We check that there is a previous comments page link. | ||
| await expect( | ||
| await page.locator( 'text="Older Comments"' ) | ||
| ).toHaveCount( 1 ); |
There was a problem hiding this comment.
Let's use toBeVisible() rather than arbitrary toHaveCount( 1 ).
| ).toHaveCount( 1 ); | ||
| await expect( | ||
| await page.locator( 'text="Newer Comments"' ) | ||
| ).toHaveCount( 0 ); |
There was a problem hiding this comment.
Same here, let's use toBeHidden().
| await page.click( 'text="Older Comments"' ); | ||
|
|
||
| // We check that there are a previous and a next link. | ||
| await expect( | ||
| await page.locator( 'text="Older Comments"' ) | ||
| ).toHaveCount( 1 ); | ||
| await expect( | ||
| await page.locator( 'text="Newer Comments"' ) | ||
| ).toHaveCount( 1 ); | ||
|
|
||
| await page.click( 'text="Older Comments"' ); | ||
|
|
||
| // We check that there is only have a next link | ||
| await expect( | ||
| await page.locator( 'text="Older Comments"' ) | ||
| ).toHaveCount( 0 ); | ||
| await expect( | ||
| await page.locator( 'text="Newer Comments"' ) | ||
| ).toHaveCount( 1 ); |
| } ); | ||
|
|
||
| test.afterAll( async ( { requestUtils } ) => { | ||
| await requestUtils.deleteAllComments(); |
There was a problem hiding this comment.
Why aren't we deleting comments after each test instead?
Thanks a lot for your guidance @kevin940726! I'll be AFK for most of this week. I had a thought, maybe someone else from @WordPress/frontend-dx could file a PR to address your feedback in order to familiarize themselves with Playwright? I think it should be doable with a half-day. The PR desc has links to documentation and contains information how to run Playwright-based e2e tests locally. Otherwise, I'll work on this next week 😄 |
|
Hey, @ockham, thanks for the ping! I don't mind opening a new PR to apply @kevin940726's suggestions and learn more about Playwright. I can take care of that. 😊 |
What?
Migrate Comments block e2e tests to Playwright.
Why?
Playwright infrastructure was recently added to Gutenberg (as covered on make.wp.org/core); here's a list of all e2e tests that need migrating.
The Comments block e2e test was only added recently (#39502), so our memory is still fresh, making it a good candidate for migration 🙂
How?
Following migration instructions found here.
Specifically, we're introducing new utilities to
deleteAllComments(via REST API) and tosetOption(viawp-admin).Testing Instructions
Verify that the new e2e test passes (check CI). To test locally:
To run in interactive mode, append
--headed.TODO
Add Changelog entries to(Still unreleased, so not required)packages/e2e-test-utils-playwright.