E2E Utils: add setPreferences and editPost utils #55099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +8.37 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
@kevin940726 @Mamaduka, I'm looking for early feedback on the new utils: |
Mamaduka
reviewed
Oct 9, 2023
There was a problem hiding this comment.
Thank you, @WunderBart! I think these utils make sense; technically, Widgets are using the block editor.
It's necessary for the post to open in the editor and not redirect to the edit.php page
WunderBart
added
[Type] Automated Testing
|
@Mamaduka @kevin940726, it's ready for review! 🙌 |
Mamaduka
reviewed
Oct 12, 2023
There was a problem hiding this comment.
@WunderBart, I restarted the performance tests again, but failure might be related to the changes here.
Mamaduka
reviewed
Oct 12, 2023
Comment on lines
-58
to
-67
| // 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(); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Restored in 9559d8e See comment below
There was a problem hiding this comment.
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.
kevin940726
reviewed
Oct 12, 2023
packages/e2e-test-utils-playwright/src/admin/create-new-post.ts
Outdated
Show resolved
Hide resolved
|
|
||
| await this.visitAdminPage( 'post-new.php', query ); | ||
|
|
||
| await this.editor.setPreferences( 'core/edit-post', { |
There was a problem hiding this comment.
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 😅,
There was a problem hiding this comment.
I like the idea. The only time the welcome guide needs to be enabled is when we're actually testing it.
There was a problem hiding this comment.
Is there an existing util for setting prefs via a network request? 🤔
| this: Admin, | ||
| options: PostOptions = {} | ||
| ) { | ||
| if ( typeof options.postId === 'undefined' ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
A few notes on this:
- The 59720fe is a breaking change. We shouldn't ship that as
@wordpresspackages try to follow the same rules as WP regarding backward compatibility. - Personally, I prefer a more explicit
createNewPostaction.
@WunderBart, I guess we can make createNewPost a wrapper around visitPostEditor. We just need to make sure that the argument signature is the same.
There was a problem hiding this comment.
The wrapper idea makes sense to me. cc @kevin940726 if he has any objections here!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
keep the
createNewPostand implementeditPostinstead ofvisitPostEditor.
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.
There was a problem hiding this comment.
I appreciate the discussions and I agree with the final solution we settled on too! Thank you all 🧡
Combine the two utils leaving only the latter. Update tests accordingly.
Also, a little bit of a follow-up cleanup related to this change in #55441 |
WunderBart
force-pushed
the
add/e2e-utils-editor-preferences
branch
from
729beab
to
7658a0a
Compare
WunderBart
force-pushed
the
add/e2e-utils-editor-preferences
branch
from
131d582
to
7658a0a
Compare
10 seconds doesn't cut it for the perf tests as we're often testing against a large content there.
|
@Mamaduka @kevin940726 it's ready for another go! 🙏 |
|
Sorry, I was AFK last week and missed the notification. I am adding this to my to-do list and will review it later today. |
|
Sorry I'm also AFK until later next week 😅. Feel free to merge without me though! 🙏 |
Mamaduka
approved these changes
Nov 6, 2023
There was a problem hiding this comment.
Thanks for the follow-ups, @WunderBart. The changes here look good to me 🚢
WunderBart
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add
editor.setPreferences()utilityBeforeAfterAdd
admin.editPost()utilityBeforeAfterMigrate
admin.createNewPost()to TSTesting Instructions
Tests should pass.