Reorganize playwright utils and make canvas utils usable in the site editor #40815
Conversation
talldan
added
the
[Type] Automated Testing
|
Size Change: +627 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
| test.use( { | ||
| editor: async ( { page }, use ) => { | ||
| await use( new Editor( { page } ) ); | ||
| }, | ||
| } ); |
There was a problem hiding this comment.
I don't think we actually need to do this in every test. Site editor tests can probably do something like:
editor.useIframeCanvas() in a beforeAll. 🤔
There was a problem hiding this comment.
Actually, this doesn't seem possible, there's an error if I try this - Error: "context" and "page" fixtures are not supported in beforeAll. Use browser.newContext() instead.
There was a problem hiding this comment.
Yeah we can't do any of that stuff in beforeAll or afterAll, that's one of the reasons we need requestUtils.
talldan
force-pushed
the
try/reorganize-playwright-utils
branch
from
f6d2ef2 to
ff79eb5
Compare
talldan
changed the title
| this.#hasIframe = hasIframe; | ||
| } | ||
|
|
||
| get canvas(): Frame | Page { |
There was a problem hiding this comment.
I wonder if we could also return PageUtils or Editor here. So that in tests we could do something like:
const { page /* or frame */, pageUtils, editor, ...etc } = editor.canvas;We can then chain all the existing utils with an explicit API. I haven't looked into it enough to know if it's possible though 😅.
There was a problem hiding this comment.
Would you be able to add some more detail on that? Would the goal be to use pageUtils within the editor canvas?
There was a problem hiding this comment.
Yep, exactly. I think it's still a problem, right? Though it's not really related to this PR though and we can discuss it in another issue :).
There was a problem hiding this comment.
It's tricky because we'd have to pass the appropriate frame or page into pageUtils, possibly instantiating it on the fly within the canvas getter, and each util would have to be written in a way that takes that into account.
Looking at the existing puppeteer utils, there aren't many functions that would benefit from this. Possibly only enableObserveFocusLoss and disableObserveFocusLoss because I think they'd have to listen inside the iframe.
It may be easier to do something like pageUtils.enableObserveFocusLoss( editor.canvas ).
I quite like the shorter names, it feels nice and concise when writing tests, which are often quite verbose. The thing that I noticed is that I wonder whether that will be confusing to newcomers. |
Ahh, I see! Putting it this way makes more sense, but I'm not sure if it'd be confusing either. I guess it'd still be okay as long as we document it though. Personally, I'm fine with it 👍. |
talldan
force-pushed
the
try/reorganize-playwright-utils
branch
from
16cd96f to
11beb76
Compare
|
Ok, I've updated the README file to add some details about the changes. I haven't updated the CHANGELOG, as it looks like the package is still private. All tests passing, so should be good for further feedback or merge. |
| hasIframe?: boolean; | ||
| }; | ||
|
|
||
| export class Editor { |
There was a problem hiding this comment.
Asking each test to manually include Editor and initiate it in test.use might not be ergonomic for some. I wonder if we could just auto-initiate them in test.ts like we do for pageUtils and admin? The problem is with the hasIframe option though.
Apologies if I missed something here, but could we remove the hasIframe boolean, and just use the canvas getter for such cases? So that we can initiate the editor fixture globally and also allow accessing from the iframe canvas in each test.
There was a problem hiding this comment.
Asking each test to manually include Editor and initiate it in test.use might not be ergonomic for some
Yeah, that is the downside.
Apologies if I missed something here, but could we remove the hasIframe boolean, and just use the canvas getter for such cases?
The idea is that we can write editor utils that interact with the canvas and internally they can use this.canvas. The test configuration determines whether the iframe or the page is used. Removing hasIframe makes that difficult to do.
Potentially we could have some separate configuration that determines whether an iframe is used in a test util. For example, another class that just stores configuration? If it didn't rely on Page then it could be instantiated in test.ts and hasIframe could be set in a beforeAll only for site editor tests.
There was a problem hiding this comment.
I see. 🤔
Technically, I think we can still useCanvas in beforeEach though? Would that make things easier?
There was a problem hiding this comment.
I think I managed to get it working without needing Editor to be setup in every test, the setup is now only needed in site editor tests. If this doesn't work we can try the beforeEach
There was a problem hiding this comment.
Cool! Amazingly simple solution 😆.
Where does that live? The overview issue seems to link to a blog post, and it didn't have any outdated info. |
|
|
Not sure how I missed that. I've updated those docs. |
…lasses Add new editor canvas utility Update tests to use EditorCanvas util WIP Reorganize e2e utils Fix a few things Fix test issues Rename folders for pageUtils and requestUtils and move isCurrentUrl back to pageUtils Fix remaining test issues Update docs Make constructors consistent Update new image block test to use reorganized utils Update migration guide Add step for configuring editor utils Avoid configuring Editor in every test
talldan
force-pushed
the
try/reorganize-playwright-utils
branch
from
f5f3108 to
e2e9383
Compare
talldan
changed the title
|
@talldan Since now that Post Editor also uses iframe, would it make sense to default the Also, curious to ask, could a simple check in the get canvas(): Frame | Page {
let frame = this.page.frame( 'editor-canvas' ) || this.page;
if ( ! frame ) {
throw new Error(
'EditorUtils: unable to find editor canvas iframe or page'
);
}
return frame;
} |
|
@kevin940726 Sounds like a good idea.
If it works, that'd be great. I think the 'editor-canvas' class still existed as a |
What?
related - #38851 (comment).
Reorganizes the playwright e2e test utils.
Also converts a lot more of them to TypeScript.
Why?
When migrating some site editor tests to playwright, I realised that it wasn't straightforward. A lot of existing e2e utils couldn't be used because they're unable to select from the site-editor's iframed editor canvas. Selecting from the iframe canvas requires using
frameorframeLocatorinstead ofpage.Puppeteer tests seem to have the same issue, and it resulted in a lot of utils being duplicated for the site editor, often these functions are just declared in the test file.
In this PR i'm trying to get ahead of the issue for the Playwright utils by organizing them in a way that works from the start.
How?
I've divided up the utils into more granular parts:
Admin- Any utils related to WP Admin (e.g.visitAdminPage)Editor- Any utils related to the block editor (e.g.insertBlock). Exposes acanvasproperty that can be used to select elements within the site editor's iframe. This has to be initialised within each test, and we pass ahasIframeprop when instantiating for the site editor.PageUtils- This was previously where all the utils (apart from RequestUtils) were defined, but now this is a place for any generic browser interactions (e.g.pressKeyWithModifier).