Playwright Utils: Fix the method of getting post ID in 'publishPost' #56421
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
Mamaduka
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
WunderBart
reviewed
Nov 22, 2023
| @@ -48,7 +48,7 @@ test.describe( 'Sidebar Permalink', () => { | |||
| await editor.canvas | |||
| .getByRole( 'textbox', { name: 'Add title' } ) | |||
| .fill( 'aaaaa' ); | |||
| await editor.saveDraft(); | |||
| await editor.publishPost(); | |||
There was a problem hiding this comment.
Mostly to showcase that it works. See #56253 (comment).
| .getByRole( 'button', { name: 'Dismiss this notice' } ) | ||
| .filter( { hasText: 'published' } ) | ||
| .waitFor(); | ||
| const postId = new URL( this.page.url() ).searchParams.get( 'post' ); | ||
|
|
||
| return typeof postId === 'string' ? parseInt( postId, 10 ) : null; |
There was a problem hiding this comment.
In which situations would the postId be unavailable?
There was a problem hiding this comment.
Not sure. The safeguard was here, so I just left it.
| .getByRole( 'button', { name: 'Dismiss this notice' } ) | ||
| .filter( { hasText: 'published' } ) | ||
| .waitFor(); | ||
| const postId = new URL( this.page.url() ).searchParams.get( 'post' ); |
There was a problem hiding this comment.
This looks OK, but if we expect the post param to be there, we might want to wrap it with waitForFunction or an expect.poll so we're not fetching it too early.
There was a problem hiding this comment.
I don't have strong opinions here, but if the query param isn't available, there's something wrong with the test, and it's better to fail.
We're already using a similar pattern without any additional checks. Here's an example from perf utils:
P.S. The post query arg doesn't change after a post is published or saved as a draft.
There was a problem hiding this comment.
(…) if the query param isn't available, there's something wrong with the test, and it's better to fail.
That's why I asked if there's a situation where the post id would not be available after saving/publishing. If not, we should probably throw from this util instead of returning null, IIUC.
The post query arg doesn't change after a post is published or saved as a draft.
Right, the subsequent saves would already have the post param. The only potential flakey would come from the first save, then. I guess this is fine, though it would not be the first time I see racing issues caused by assuming one element's state off of another's – in this case, URL off of the post-publish notice.
There was a problem hiding this comment.
I guess this is fine, though it would not be the first time I see racing issues caused by assuming one element's state off of another element's state
E2E tests have to make those assumptions, IMO.
If not, we should probably throw from this util instead of returning null, IIUC.
Somethings like this?
if ( postId !== 'string' ) {
throw new Error( 'Unable to retrieve post ID from URL.' );
}
return parseInt( postId, 10 );There was a problem hiding this comment.
E2E tests have to make those assumptions, IMO.
Not if we can wait for stuff! 😄 (post ID in this case)
Somethings like this?
I realized now that we could get the ID via the getCurrentPostId() which we don't need to wait for (id is set when the new-post.php is open)
const postId = await this.page.evaluate( () => {
return window.wp.data.select( 'core/editor' ).getCurrentPostId();
} );
return postId;There was a problem hiding this comment.
...but we can also wait for the post param, like this:
const postId = await this.page.waitForFunction( () => {
const url = new URL( window.location.href );
if ( url.searchParams.has( 'post' ) ) {
return url.searchParams.get( 'post' );
}
return false;
} );
return parseInt( postId, 10 );There was a problem hiding this comment.
Thank you, @WunderBart 🙇 Do you think we should update the code or keep it as it is for now?
There was a problem hiding this comment.
If it ain't broke, don't fix it, right? 😄 No pressure here - let's just keep that in mind in case it starts making trouble.
kevin940726
approved these changes
Nov 22, 2023
|
Thanks for the feedback! I will merge this and be happy to do a follow-up if/when there's a need for it. |
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.
What?
This is a follow-up to #56253 (comment).
PR fixes the method of getting post ID in
publishPostto avoid failures when the post publish panel isn't displayed.Why?
Displaying the post-publish panel isn't guaranteed. It can be hidden when a post-type isn't viewable or disabled using the "Include pre-publish checklist" preference.
How?
Testing Instructions
Playwright E2E tests are passing on CI.