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.

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

Migrate cut-copy-paste-whole-blocks to Playwright #39807

Merged
merged 6 commits into from
Apr 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Returns a promise which resolves with the edited post content (HTML string).
*
* @this {import('./').PageUtils}
* @return {Promise} Promise resolving with post content markup.
*/
export async function getEditedPostContent() {
return await this.page.evaluate( () =>
window.wp.data.select( 'core/editor' ).getEditedPostContent()
);
}
10 changes: 10 additions & 0 deletions packages/e2e-test-utils-playwright/src/page/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,14 @@ import type { Browser, Page, BrowserContext } from '@playwright/test';
*/
import { clickBlockToolbarButton } from './click-block-toolbar-button';
import { createNewPost } from './create-new-post';
import { getEditedPostContent } from './get-edited-post-content';
import { getPageError } from './get-page-error';
import { insertBlock } from './insert-block';
import { isCurrentURL } from './is-current-url';
import {
setClipboardData,
pressKeyWithModifier,
} from './press-key-with-modifier';
import { showBlockToolbar } from './show-block-toolbar';
import { visitAdminPage } from './visit-admin-page';

Expand All @@ -26,8 +32,12 @@ class PageUtils {

clickBlockToolbarButton = clickBlockToolbarButton;
createNewPost = createNewPost;
getEditedPostContent = getEditedPostContent;
getPageError = getPageError;
insertBlock = insertBlock;
isCurrentURL = isCurrentURL;
pressKeyWithModifier = pressKeyWithModifier;
setClipboardData = setClipboardData;
showBlockToolbar = showBlockToolbar;
visitAdminPage = visitAdminPage;
}
Expand Down
33 changes: 33 additions & 0 deletions packages/e2e-test-utils-playwright/src/page/insert-block.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* @typedef {Object} BlockRepresentation
* @property {string} name Block name.
* @property {?Object} attributes Block attributes.
* @property {?BlockRepresentation[]} innerBlocks Nested blocks.
*/

/**
* @this {import('./').PageUtils}
* @param {BlockRepresentation} blockRepresentation Inserted block representation.
*/
async function insertBlock( blockRepresentation ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be the playwright version of this one: https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-test-utils/src/inserter.js#L165?

If yes, we're bypassing all the insertion flow from the inserter and doesn't seem something e2e tests should be doing. The good thing about the original util is that it's using the normal user flow with the inserter and we can catch and regressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think it depends on the intention of the test. If the test is meant for testing the insertion flow then we should definitely follow how an end user would do it. However, in most cases, we're testing other things and just need to populate the editor with some content. In such cases, calling the API directly to speed up the test is recommended. It's the same reason why we use requestUtils rather than visiting and making changes manually.

I think similar things have been discussed before, but I forget where 😅. Kent explained it better though 😆. We already have some tests covering inserting blocks via the global inserter in inserting-blocks.test.js so I think it should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get what you mean, but still I don't think this is the same with using requestUtils to create/delete posts etc.. and the main example from Kent is logging a user. This flow though handles other things useful for many tests, like where a block should be inserted(rootClientId), focus, selection etc.. which they may be needed in many tests. An example would be to insert blocks -> navigate between and insert other blocks.

My personal opinion is that we could have a util like this with a different name and since we're doing a migration to playwright we should have one similar to the previous one(that follows the flow). This way a user could have the option to select what they want. Also if we care about speed and want to just have some content, we could add this content in the request when we create a post, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is the same with using requestUtils to create/delete posts etc.. and the main example from Kent is logging a user.

I don't think these are that different though. Neither of them is the main thing being tested in their test cases. Just that inserting blocks is faster than logging in a user.

This flow though handles other things useful for many tests, like where a block should be inserted(rootClientId), focus, selection etc.. which they may be needed in many tests

Do you have an example where it isn't covered in the dedicated inserting-blocks test suite? If this is something that depends on the inserted blocks then I think it's worth it to add it to the utils as you said. But then I think there might be a bigger issue in our architecture where the flow would change if inserting a different block.

we could add this content in the request when we create a post, no?

True! FWIW, there's a new util added recently called editor.setContent that can set and replace the full content in the editor. I think it should be used when we're just setting/resetting the editor, but not all of the tests have been migrated yet.

since we're doing a migration to playwright we should have one similar to the previous one(that follows the flow).

IMHO, not every util should have its Playwright version of the same thing. I'd prefer inlining utils or using POM to encapsulate them in their belonging test suite. This encourages best practices and discourages repeating ourselves by accidentally using the wrong utils. We're also trying to refactor the tests during the migration to hopefully leverage the full power of Playwright.

Full coverage isn't a priority of these e2e tests, and I'd favor explicit assertions over implicit flow hidden in e2e utils. If it's something special enough to be tested, then I think we should write a test case for it and do the normal flow explicitly.

This, however, is just my own preference. I don't know what everyone else thinks of this though. I sort of just took the liberty to write the best practices guide because there wasn't anyone else opposing it either. Happy to discuss this further if you feel strongly about this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an example where it isn't covered in the dedicated inserting-blocks test suite? If this is something that depends on the inserted blocks then I think it's worth it to add it to the utils as you said. But then I think there might be a bigger issue in our architecture where the flow would change if inserting a different block.

I don't have something specific in mind, but I'd guess there are some tests with my example: An example would be to insert blocks -> navigate between and insert other blocks. Maybe there are in the remaining tests and this will be visible when we migrate those..

I don't have a strong opinion right now. The consistency between the two is what I'd expected at first, but this isn't necessarily something we aim to, as they are different packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

An example would be to insert blocks -> navigate between and insert other blocks

Ahh, I think I know what you mean here. I'm not sure though, I don't think that's mostly relevant to the block we're testing. It would be nice to provide an option to call insertBlock at any given position though.

The consistency between the two is what I'd expected at first, but this isn't necessarily something we aim to, as they are different packages.

Yep, the e2e-test-utils-playwright package is not meant to be as a drop-in replacement for e2e-test-utils. It's a package to help us write e2e tests with Playwright. I understand that it's a common misconception though since the term "migration" implies a lot.

await this.page.evaluate( ( _blockRepresentation ) => {
function recursiveCreateBlock( {
name,
attributes = {},
innerBlocks = [],
} ) {
return window.wp.blocks.createBlock(
name,
attributes,
innerBlocks.map( ( innerBlock ) =>
recursiveCreateBlock( innerBlock )
)
);
}
const block = recursiveCreateBlock( _blockRepresentation );

window.wp.data.dispatch( 'core/block-editor' ).insertBlock( block );
}, blockRepresentation );
}

export { insertBlock };
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/**
* External dependencies
*/
import { capitalize } from 'lodash';
import type { Page } from '@playwright/test';

/**
* WordPress dependencies
*/
import { modifiers, SHIFT, ALT, CTRL } from '@wordpress/keycodes';
import type { WPKeycodeModifier } from '@wordpress/keycodes';

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

let clipboardDataHolder: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this object that is used and is updated in the helper functions?

Copy link
Member Author

@kevin940726 kevin940726 Apr 25, 2022

Choose a reason for hiding this comment

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

This is a refactor from saving it in the window._clipboardData (which pollutes the page's global scope) to saving it using a local variable in the module scope.

I tested it in the other tests migrated here and it seems to work fine. However, there might be edge cases I missed that break the new tests though. If you've spotted any inconsistency or error, please LMK :)!

Copy link
Member Author

Choose a reason for hiding this comment

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

This potentially (not tested 😅) also fixes a bug in the original util where copied data won't persist after page navigations (Because window._clipboardData will be destroyed).

(But now that I think about it, using local module scope might be problematic if we want to run tests in parallel someday. Maybe we should bind it under the pageUtils class scope 🤔. But that's for another day I guess.)

plainText: string;
html: string;
} = {
plainText: '',
html: '',
};

/**
* Sets the clipboard data that can be pasted with
* `pressKeyWithModifier( 'primary', 'v' )`.
*
* @param this
* @param clipboardData
* @param clipboardData.plainText
* @param clipboardData.html
*/
export function setClipboardData(
this: PageUtils,
{ plainText = '', html = '' }: typeof clipboardDataHolder
) {
clipboardDataHolder = {
plainText,
html,
};
}

async function emulateClipboard( page: Page, type: 'copy' | 'cut' | 'paste' ) {
clipboardDataHolder = await page.evaluate(
( [ _type, _clipboardData ] ) => {
const clipboardDataTransfer = new DataTransfer();

if ( _type === 'paste' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic is different here from the previous emulateClipboard util. Was there a bug or something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Commented here.

clipboardDataTransfer.setData(
'text/plain',
_clipboardData.plainText
);
clipboardDataTransfer.setData(
'text/html',
_clipboardData.html
);
} else {
const selection = window.getSelection()!;
const plainText = selection.toString();
let html = plainText;
if ( selection.rangeCount ) {
const range = selection.getRangeAt( 0 );
const fragment = range.cloneContents();
html = Array.from( fragment.childNodes )
.map( ( node ) =>
Object.prototype.hasOwnProperty.call(
node,
'outerHTML'
)
? ( node as Element ).outerHTML
: node.nodeValue
)
.join( '' );
}
clipboardDataTransfer.setData( 'text/plain', plainText );
clipboardDataTransfer.setData( 'text/html', html );
}

document.activeElement?.dispatchEvent(
new ClipboardEvent( _type, {
bubbles: true,
cancelable: true,
clipboardData: clipboardDataTransfer,
} )
);

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use/need the returned data 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.

Yep, the returned data will be resolved to a Serializable and assigned to clipboardDataHolder.

plainText: clipboardDataTransfer.getData( 'text/plain' ),
html: clipboardDataTransfer.getData( 'text/html' ),
};
},
[ type, clipboardDataHolder ] as const
);
}

/**
* Performs a key press with modifier (Shift, Control, Meta, Alt), where each modifier
* is normalized to platform-specific modifier.
*
* @param this
* @param modifier
* @param key
*/
export async function pressKeyWithModifier(
this: PageUtils,
modifier: WPKeycodeModifier,
key: string
) {
if ( modifier.toLowerCase() === 'primary' && key.toLowerCase() === 'c' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here about the logic being different. Also there are some missing handling like emulateSelectAll. I'd expect that if we migrate a util it should preserve the previous logic because right now these util is only used to the copy/paste tests. But when we migrate more tests that use pressKeyWithModifier with the missing handling cases, it will be really hard to debug.

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 utils are never meant to be one-to-one migration because Playwright and Puppeteer are still very different in some cases. I didn't include emulateSelectAll because Playwright seems to be handling it well implicitly. And we can always add it back if it turns out we still need it.

The current strategy is that if we migrate a test then we should make sure it still works as expected. Developers shouldn't rely on the old util to work seamlessly on Playwright. Instead, they should understand the problem the test is trying to solve, and follow the best practices when migrating/refactoring it. One of the benefits Playwright brings is that we don't need that many utils anymore, so we should think about minimizing them in the process. I'm not saying it's an easy job though, so yeah, it might be hard to debug for a migration. But that's expected, IMO, we're still writing e2e tests in the end, which is also not an easy job.

return await emulateClipboard( this.page, 'copy' );
}

if ( modifier.toLowerCase() === 'primary' && key.toLowerCase() === 'x' ) {
return await emulateClipboard( this.page, 'cut' );
}

if ( modifier.toLowerCase() === 'primary' && key.toLowerCase() === 'v' ) {
return await emulateClipboard( this.page, 'paste' );
}

const isAppleOS = () => process.platform === 'darwin';
const overWrittenModifiers = {
...modifiers,
shiftAlt: ( _isApple: () => boolean ) =>
_isApple() ? [ SHIFT, ALT ] : [ SHIFT, CTRL ],
};
const mappedModifiers = overWrittenModifiers[ modifier ](
isAppleOS
).map( ( keycode ) =>
keycode === CTRL ? 'Control' : capitalize( keycode )
);

await this.page.keyboard.press(
`${ mappedModifiers.join( '+' ) }+${ key }`
);
}
11 changes: 11 additions & 0 deletions packages/e2e-test-utils-playwright/src/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ function observeConsoleLogging( message: ConsoleMessage ) {
const test = base.extend<
{
pageUtils: PageUtils;
snapshotSuffix: void;
},
{
requestUtils: RequestUtils;
Expand Down Expand Up @@ -136,6 +137,16 @@ const test = base.extend<
},
{ scope: 'worker' },
],
// A work-around automatic fixture to remove the default snapshot suffix.
// See https://github.com/microsoft/playwright/issues/11134
snapshotSuffix: [
async ( {}, use, testInfo ) => {
testInfo.snapshotSuffix = '';

await use();
},
{ auto: true },
],
} );

export { test, expect };

This file was deleted.

Loading