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

Playwright Utils: Fix the method of getting post ID in 'publishPost' #56421

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Playwright Utils: Fix the method of getting post ID in 'publishPost' #56421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c86ac6
Playwright Utils: Fix the method of getting post ID in 'publishPost'
Mamaduka Nov 22, 2023
fb3c29c
Update sidebar permalink e2e test
Mamaduka Nov 22, 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
11 changes: 5 additions & 6 deletions packages/e2e-test-utils-playwright/src/editor/publish-post.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,11 @@ export async function publishPost( this: Editor ) {
'role=region[name="Editor publish"i] >> role=button[name="Publish"i]'
);

const urlString = await this.page
.getByRole( 'region', { name: 'Editor publish' } )
.getByRole( 'textbox', { name: 'address' } )
.inputValue();
const url = new URL( urlString );
const postId = url.searchParams.get( 'p' );
await this.page
.getByRole( 'button', { name: 'Dismiss this notice' } )
.filter( { hasText: 'published' } )
.waitFor();
const postId = new URL( this.page.url() ).searchParams.get( 'post' );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Mamaduka Mamaduka Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

gutenberg/test/performance/fixtures/perf-utils.ts

Line 62 in a791158

const postId = new URL( this.page.url() ).searchParams.get( 'post' );

P.S. The post query arg doesn't change after a post is published or saved as a draft.

Copy link
Member

@WunderBart WunderBart Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(…) 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.

Copy link
Member Author

@Mamaduka Mamaduka Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 );

Copy link
Member

@WunderBart WunderBart Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Member

Choose a reason for hiding this comment

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

...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 );

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @WunderBart 🙇 Do you think we should update the code or keep it as it is for now?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good!


return typeof postId === 'string' ? parseInt( postId, 10 ) : null;
Copy link
Member

Choose a reason for hiding this comment

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

In which situations would the postId be unavailable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. The safeguard was here, so I just left it.

}
2 changes: 1 addition & 1 deletion test/e2e/specs/editor/various/sidebar-permalink.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ test.describe( 'Sidebar Permalink', () => {
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
.fill( 'aaaaa' );
await editor.saveDraft();
await editor.publishPost();
Copy link
Member

Choose a reason for hiding this comment

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

Why this change here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly to showcase that it works. See #56253 (comment).

// Start editing again.
await editor.canvas
.getByRole( 'textbox', { name: 'Add title' } )
Expand Down
Loading