-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
d4fd209
1d552ff
fe0e700
163c350
d7ed30b
ed815d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() | ||
); | ||
} |
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 ) { | ||
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: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a refactor from saving it in the 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 :)! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (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 |
||
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' ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logic is different here from the previous There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we use/need the returned data here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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' ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 }` | ||
); | ||
} |
This file was deleted.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.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.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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Yep, the
e2e-test-utils-playwright
package is not meant to be as a drop-in replacement fore2e-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.