Migrate cut-copy-paste-whole-blocks to Playwright #39807
Conversation
kevin940726
added
the
[Type] Automated Testing
|
Size Change: 0 B Total Size: 1.22 MB ℹ️ View Unchanged
|
kevin940726
force-pushed
the
add/migrate-cut-copy-paste-whole-blocks
branch
from
4aea8d7
to
8073fd0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
kevin940726
force-pushed
the
add/migrate-cut-copy-paste-whole-blocks
branch
from
8073fd0
to
ed815d7
Compare
| */ | ||
| import type { PageUtils } from './index'; | ||
|
|
||
| let clipboardDataHolder: { |
There was a problem hiding this comment.
Why do we need this object that is used and is updated in the helper functions?
There was a problem hiding this comment.
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 :)!
There was a problem hiding this comment.
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.)
| } ) | ||
| ); | ||
|
|
||
| return { |
There was a problem hiding this comment.
Do we use/need the returned data here?
There was a problem hiding this comment.
Yep, the returned data will be resolved to a Serializable and assigned to clipboardDataHolder.
| ( [ _type, _clipboardData ] ) => { | ||
| const clipboardDataTransfer = new DataTransfer(); | ||
|
|
||
| if ( _type === 'paste' ) { |
There was a problem hiding this comment.
Logic is different here from the previous emulateClipboard util. Was there a bug or something else?
| modifier: WPKeycodeModifier, | ||
| key: string | ||
| ) { | ||
| if ( modifier.toLowerCase() === 'primary' && key.toLowerCase() === 'c' ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * @this {import('./').PageUtils} | ||
| * @param {BlockRepresentation} blockRepresentation Inserted block representation. | ||
| */ | ||
| async function insertBlock( blockRepresentation ) { |
There was a problem hiding this comment.
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.
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.
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.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What?
Part of #38851. Migrate
cut-copy-paste-whole-blocks.test.jsto Playwright.Hopefully fixes #39075, fixes #35617.
Why?
See this post for an overview of the migration.
How?
By following the migration guide.
This PR also ports some utils to
e2e-test-utils-playwrightwith slight differences between their puppeteer counterparts.Testing Instructions