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

Jump to bottom

E2E Utils: add setPreferences and editPost utils #55099

Merged
merged 20 commits into from
Nov 6, 2023
Merged

E2E Utils: add setPreferences and editPost utils #55099

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fe70678
Create editor.setPreferences util
WunderBart Oct 5, 2023
aecd765
Migrate createNewPost to TS; use editor.setPreferences
WunderBart Oct 5, 2023
ecef484
Create visitPostEditor util
WunderBart Oct 5, 2023
86c8f79
Allow for setting preferences context
WunderBart Oct 5, 2023
a3c356e
Pass context correctly
WunderBart Oct 5, 2023
8ac5f11
Make visitSiteEditor consistent with other utils
WunderBart Oct 5, 2023
148c15b
Update tests to use editor.setPreferences()
WunderBart Oct 5, 2023
c3f20f5
Merge branch 'trunk' into add/e2e-utils-editor-preferences
WunderBart Oct 6, 2023
ddfd869
Make action param fixed
WunderBart Oct 11, 2023
256f5b9
Update specs
WunderBart Oct 11, 2023
34f93b3
Pass props as object to evaluate call
WunderBart Oct 11, 2023
6e5feb7
Merge remote-tracking branch 'origin' into add/e2e-utils-editor-prefe…
WunderBart Oct 16, 2023
59720fe
Merge createNewPost into visitPostEditor
WunderBart Oct 17, 2023
d2aba51
Update visitSiteEditor to use URLSearchParams
WunderBart Oct 17, 2023
9559d8e
Restore canvas check to avoid unrelated changes
WunderBart Oct 17, 2023
7658a0a
Fix accidental page->editor rename
WunderBart Oct 18, 2023
36b26c2
Increase perf tests action timeout to 2 minutes.
WunderBart Oct 18, 2023
99c2b2d
Remove canvas waiter from visitSiteEditor as it's actually breaking s…
WunderBart Oct 19, 2023
8b7ffd2
Merge remote-tracking branch 'origin' into add/e2e-utils-editor-prefe…
WunderBart Oct 23, 2023
fa96ad2
Drop visitPostEditor in favor of createNewPost + editPost
WunderBart Nov 3, 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
47 changes: 0 additions & 47 deletions packages/e2e-test-utils-playwright/src/admin/create-new-post.js
View file
Open in desktop

This file was deleted.

42 changes: 42 additions & 0 deletions packages/e2e-test-utils-playwright/src/admin/create-new-post.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* WordPress dependencies
*/
import { addQueryArgs } from '@wordpress/url';

/**
* Internal dependencies
*/
import type { Admin } from './';

interface NewPostOptions {
postType?: string;
title?: string;
content?: string;
excerpt?: string;
showWelcomeGuide?: boolean;
}

/**
* Creates new post.
*
* @param this
* @param options Options to create new post.
*/
export async function createNewPost(
this: Admin,
options: NewPostOptions = {}
) {
const query = addQueryArgs( '', {
WunderBart marked this conversation as resolved.
Show resolved Hide resolved
post_type: options.postType,
post_title: options.title,
content: options.content,
excerpt: options.excerpt,
} ).slice( 1 );

await this.visitAdminPage( 'post-new.php', query );

await this.editor.setPreferences( 'core/edit-post', {
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious to know if we need to do this every time, seems wasteful to me. Could we just do this in global-setup with a network request? I know that the preference API doesn't seem stable in e2e tests though 😅,

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea. The only time the welcome guide needs to be enabled is when we're actually testing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an existing util for setting prefs via a network request? 🤔

welcomeGuide: options.showWelcomeGuide ?? false,
fullscreenMode: false,
} );
}
13 changes: 10 additions & 3 deletions packages/e2e-test-utils-playwright/src/admin/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,30 @@ import type { Browser, Page, BrowserContext } from '@playwright/test';
import { createNewPost } from './create-new-post';
import { getPageError } from './get-page-error';
import { visitAdminPage } from './visit-admin-page';
import { visitPostEditor } from './visit-post-editor';
import { visitSiteEditor } from './visit-site-editor';
import type { PageUtils } from '../page-utils';
import type { Editor } from '../editor';

type AdminConstructorProps = {
page: Page;
pageUtils: PageUtils;
editor: Editor;
};

export class Admin {
browser: Browser;
page: Page;
pageUtils: PageUtils;
context: BrowserContext;
browser: Browser;
pageUtils: PageUtils;
editor: Editor;

constructor( { page, pageUtils }: AdminConstructorProps ) {
constructor( { page, pageUtils, editor }: AdminConstructorProps ) {
this.page = page;
this.context = page.context();
this.browser = this.context.browser()!;
this.pageUtils = pageUtils;
this.editor = editor;
}

/** @borrows createNewPost as this.createNewPost */
Expand All @@ -36,6 +41,8 @@ export class Admin {
getPageError: typeof getPageError = getPageError.bind( this );
/** @borrows visitAdminPage as this.visitAdminPage */
visitAdminPage: typeof visitAdminPage = visitAdminPage.bind( this );
/** @borrows visitPostEditor as this.visitPostEditor */
visitPostEditor: typeof visitPostEditor = visitPostEditor.bind( this );
/** @borrows visitSiteEditor as this.visitSiteEditor */
visitSiteEditor: typeof visitSiteEditor = visitSiteEditor.bind( this );
}
42 changes: 42 additions & 0 deletions packages/e2e-test-utils-playwright/src/admin/visit-post-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* WordPress dependencies
*/
import { addQueryArgs } from '@wordpress/url';

/**
* Internal dependencies
*/
import type { Admin } from './';

interface PostOptions {
postId?: string | number;
action?: string;
showWelcomeGuide?: boolean;
}

/**
* Creates new post.
*
* @param this
* @param options Options to create new post.
*/
export async function visitPostEditor(
this: Admin,
options: PostOptions = {}
) {
if ( typeof options.postId === 'undefined' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have the opportunity to refactor this, I wonder if we should just combine createNewPost and visitPostEditor?

// Call with `postId`: visit the existing post.
admin.visitPostEditor( postId: number ): Promise<void>;

// No `postId`: visit `post-new.php` instead.
admin.visitPostEditor(): Promise<void>;

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Combined in 59720fe.

Copy link
Member

Choose a reason for hiding this comment

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

A few notes on this:

  1. The 59720fe is a breaking change. We shouldn't ship that as @wordpress packages try to follow the same rules as WP regarding backward compatibility.
  2. Personally, I prefer a more explicit createNewPost action.

@WunderBart, I guess we can make createNewPost a wrapper around visitPostEditor. We just need to make sure that the argument signature is the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

The wrapper idea makes sense to me. cc @kevin940726 if he has any objections here!

Copy link
Member Author

@WunderBart WunderBart Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I prefer a more explicit createNewPost action.

I'm actually starting to lean in this direction as well. The main issue with merging createNewPost and visitPostEditor are the props (and their typing) that can be confusing (e.g. if postId is passed, all the props for creating a new post are ignored). Personally, at this point I also feel like a more explicit approach would be better - keep the createNewPost and implement editPost instead of visitPostEditor. For the Site Editor, it makes sense to be reachable by visitSiteEditor because there's only one URL we can visit it by: /site-editor.php.

Sorry for going back and forth with it, but I just want to avoid overcomplicating stuff (which I might already be doing 😅). Please let me know if the above would make sense to you!

Copy link
Member

Choose a reason for hiding this comment

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

keep the createNewPost and implement editPost instead of visitPostEditor.

100% - I think this fits the WP glossary around posts much better.

Sorry for going back and forth with it, but I just want to avoid overcomplicating stuff

No worries. It's easier to anticipate what would work best when we try. Thanks for taking the time and experimenting with different approaches.

Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the discussions and I agree with the final solution we settled on too! Thank you all 🧡

throw new Error( 'postId is required.' );
}

const query = addQueryArgs( '', {
post: options.postId,
action: options.action,
} ).slice( 1 );

await this.visitAdminPage( 'post.php', query );

await this.editor.setPreferences( 'core/edit-post', {
welcomeGuide: options.showWelcomeGuide ?? false,
fullscreenMode: false,
} );
}
64 changes: 21 additions & 43 deletions packages/e2e-test-utils-playwright/src/admin/visit-site-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,64 +8,42 @@ import { addQueryArgs } from '@wordpress/url';
*/
import type { Admin } from './';

export interface SiteEditorQueryParams {
postId: string | number;
postType: string;
interface SiteEditorOptions {
postId?: string | number;
postType?: string;
path?: string;
canvas?: string;
showWelcomeGuide?: boolean;
}

const CANVAS_SELECTOR = 'iframe[title="Editor canvas"i] >> visible=true';

/**
* Visits the Site Editor main page
*
* By default, it also skips the welcome guide. The option can be disabled if need be.
* Visits the Site Editor main page.
*
* @param this
* @param query Query params to be serialized as query portion of URL.
* @param skipWelcomeGuide Whether to skip the welcome guide as part of the navigation.
* @param options Options to visit the site editor.
*/
export async function visitSiteEditor(
this: Admin,
query: SiteEditorQueryParams,
skipWelcomeGuide = true
options: SiteEditorOptions = {}
) {
const path = addQueryArgs( '', {
...query,
const query = addQueryArgs( '', {
postId: options.postId,
postType: options.postType,
path: options.path,
canvas: options.canvas,
} ).slice( 1 );

await this.visitAdminPage( 'site-editor.php', path );

if ( skipWelcomeGuide ) {
await this.page.evaluate( () => {
window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-site', 'welcomeGuide', false );

window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-site', 'welcomeGuideStyles', false );
await this.visitAdminPage( 'site-editor.php', query );

window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-site', 'welcomeGuidePage', false );

window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-site', 'welcomeGuideTemplate', false );
if ( ! options.showWelcomeGuide ) {
await this.editor.setPreferences( 'core/edit-site', {
welcomeGuide: false,
welcomeGuideStyles: false,
welcomeGuidePage: false,
welcomeGuideTemplate: false,
} );
}

// Check if the current page has an editor canvas first.
if ( ( await this.page.locator( CANVAS_SELECTOR ).count() ) > 0 ) {
// The site editor initially loads with an empty body,
// we need to wait for the editor canvas to be rendered.
await this.page
.frameLocator( CANVAS_SELECTOR )
.locator( 'body > *' )
.first()
.waitFor();
}
Comment on lines -58 to -67
Copy link
Member

Choose a reason for hiding this comment

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

This code removal doesn't look related to the changes proposed in this PR. As discussed in #54911, there were usually good reasons for introducing similar guardrails.

Maybe we should split this into a separate PR. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, nice catch! I actually forgot to restore this part. I wasn't planning on removing it in this PR, only running a single round without it to see how it goes. Restoring it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's good, though, that now we know only perf tests fail without it - these tests require specific handling, so it might be good to move this safeguard there if it's not necessary here for the regular E2Es. Anyway, it's just a note for myself, I guess - let's leave that for another PR.

Copy link
Member Author

@WunderBart WunderBart Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Restored in 9559d8e See comment below

Copy link
Member Author

@WunderBart WunderBart Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I had to remove it as the slight timing change caused by using setPreferences in visitSiteEditor revealed that the part above is actually faulty. Before, it checked the canvas iframe(s) before they were attached, so that check has always been skipped and moved instantly to wait until the loading spinner is hidden. Now, because setPreferences adds a little delay by waiting until the window.wp.data is available, all the editor iframes get attached in the meantime, which ends up violating the locator strict mode:

at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/site-editor/patterns.spec.js:165:3
Error:   1) [chromium] › site-editor/patterns.spec.js:137:2 › Patterns › search and filter patterns ───────

    Retry #1 ───────────────────────────────────────────────────────────────────────────────────────
    Error: locator.waitFor: Error: strict mode violation: locator('iframe[title="Editor canvas"i]').locator('visible=true') resolved to 3 elements:
        1) <iframe tabindex="-1" aria-hidden="true" title="Editor c…></iframe> aka locator('#edit-site-patterns-synced-footer')
        2) <iframe tabindex="-1" aria-hidden="true" title="Editor c…></iframe> aka locator('#edit-site-patterns-unsynced-header')
        3) <iframe tabindex="-1" aria-hidden="true" title="Editor c…></iframe> aka locator('#edit-site-patterns-unsynced-footer')

ℹ️ The other iframes are previews of the patterns created in that test.


// TODO: Ideally the content underneath the canvas loader should be marked inert until it's ready.
await this.page
.locator( '.edit-site-canvas-loader' )
Expand Down
3 changes: 3 additions & 0 deletions packages/e2e-test-utils-playwright/src/editor/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { openPreviewPage } from './preview';
import { publishPost } from './publish-post';
import { selectBlocks } from './select-blocks';
import { setContent } from './set-content';
import { setPreferences } from './set-preferences';
import { showBlockToolbar } from './show-block-toolbar';
import { saveSiteEditorEntities } from './site-editor';
import { setIsFixedToolbar } from './set-is-fixed-toolbar';
Expand Down Expand Up @@ -73,6 +74,8 @@ export class Editor {
selectBlocks: typeof selectBlocks = selectBlocks.bind( this );
/** @borrows setContent as this.setContent */
setContent: typeof setContent = setContent.bind( this );
/** @borrows setPreferences as this.setPreferences */
setPreferences: typeof setPreferences = setPreferences.bind( this );
/** @borrows showBlockToolbar as this.showBlockToolbar */
showBlockToolbar: typeof showBlockToolbar = showBlockToolbar.bind( this );
/** @borrows setIsFixedToolbar as this.setIsFixedToolbar */
Expand Down
35 changes: 35 additions & 0 deletions packages/e2e-test-utils-playwright/src/editor/set-preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Internal dependencies
*/
import type { Editor } from './index';

type PreferencesContext =
| 'core/edit-post'
| 'core/edit-site'
| 'core/customize-widgets';

/**
* Set the preferences of the editor.
*
* @param this
* @param context Context to set preferences for.
* @param preferences Preferences to set.
*/
export async function setPreferences(
this: Editor,
context: PreferencesContext,
preferences: Record< string, any >
) {
await this.page.waitForFunction( () => window?.wp?.data );

await this.page.evaluate(
async ( [ _context, _preferences ] ) => {
for ( const [ key, value ] of Object.entries( _preferences ) ) {
await window.wp.data
.dispatch( 'core/preferences' )
.set( _context, key, value );
}
},
[ context, preferences ]
);
}
4 changes: 2 additions & 2 deletions packages/e2e-test-utils-playwright/src/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ const test = base.extend<
lighthousePort: number;
}
>( {
admin: async ( { page, pageUtils }, use ) => {
await use( new Admin( { page, pageUtils } ) );
admin: async ( { page, pageUtils, editor }, use ) => {
await use( new Admin( { page, pageUtils, editor } ) );
},
editor: async ( { page }, use ) => {
await use( new Editor( { page } ) );
Expand Down
17 changes: 4 additions & 13 deletions test/e2e/specs/editor/local/demo.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,12 @@ test.describe( 'New editor state', () => {
test( 'content should load, making the post dirty', async ( {
page,
admin,
editor,
} ) => {
await admin.visitAdminPage( 'post-new.php', 'gutenberg-demo' );
await page.waitForFunction( () => {
if ( ! window?.wp?.data?.dispatch ) {
return false;
}
window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-post', 'welcomeGuide', false );

window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-post', 'fullscreenMode', false );

return true;
await editor.setPreferences( 'core/edit-site', {
welcomeGuide: false,
fullscreenMode: false,
} );

const isDirty = await page.evaluate( () => {
Expand Down
6 changes: 2 additions & 4 deletions test/e2e/specs/editor/various/block-renaming.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@ test.describe( 'Block Renaming', () => {
pageUtils,
} ) => {
// Turn on block list view by default.
await page.evaluate( () => {
window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-site', 'showListViewByDefault', true );
await editor.setPreferences( 'core/edit-site', {
showListViewByDefault: true,
} );

const listView = page.getByRole( 'treegrid', {
Expand Down
6 changes: 2 additions & 4 deletions test/e2e/specs/editor/various/post-editor-template-mode.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,8 @@ class PostEditorTemplateMode {

async disableTemplateWelcomeGuide() {
// Turn off the welcome guide.
await this.page.evaluate( () => {
window.wp.data
.dispatch( 'core/preferences' )
.set( 'core/edit-post', 'welcomeGuideTemplate', false );
await this.editor.setPreferences( 'core/edit-post', {
welcomeGuideTemplate: false,
} );
}

Expand Down
Loading
Loading