Migrate post-editor-template-mode to Playwright
#40175
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
kevin940726
added
the
[Type] Automated Testing
kevin940726
force-pushed
the
migrate/playwright-post-editor-template-mode
branch
from
910a059
to
39fb260
Compare
kevin940726
commented
Apr 8, 2022
There was a problem hiding this comment.
I noticed something weird in the original test file. @talldan Do you know why?
Comment on lines
-34
to
-36
| wp.data | ||
| .select( 'core/edit-post' ) | ||
| .isFeatureActive( 'welcomeGuideTemplate' ) |
There was a problem hiding this comment.
Here we're accessing the feature flag from the core/edit-post store (1/2)
Comment on lines
-235
to
-237
| !! wp.data | ||
| .select( 'core/preferences' ) | ||
| .get( 'core/edit-post', 'welcomeGuide' ) |
There was a problem hiding this comment.
But here we're accessing the feature flag from the core/preferences store. (2/2)
|
Size Change: +169 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
|
@kevin940726 Both do the same thing, the first one is an older API kept for backwards compatibility, but the implementation will now also select from the preferences store. The first API will probably be deprecated over the coming weeks. |
talldan
approved these changes
Apr 12, 2022
| } ); | ||
| } ); | ||
|
|
||
| class PostEditorTemplateMode { |
There was a problem hiding this comment.
The only thing I wonder about is whether a class is the right option here. This seems like a set of loosely defined utility functions while a class is usually strictly modelled around a particular object. Maybe this is idiomatic playwrighting. I don't know it well enough to say for sure 😄
It is just test code and it works well and is organised very nicely, so it's still a thumbs up 👍
There was a problem hiding this comment.
Yeah, it's debatable.
The idea is to follow the POM convention to create utility helpers for tests for a specific page. We could just create utility functions as we did in e2e-test-utils, but that would mean remembering to pass all those previously global variables like page and browser to each function. Using classes allows us to implicitly bind those variables to the helpers via the this keyword.
Another good idea will be to just define these functions as fixture functions. I just default to POM to try to make them consistent 😅 . There's still room for improvements, though, so ideas are welcome!
There was a problem hiding this comment.
I see, sounds like this is a recommended approach. I think it's ok as a convention, and for anyone contributing familiar with playwright, they'll probably find it helpful that we've followed this pattern.
It also seems a bit simpler than the fixture example.
Thanks for linking to the docs. 👍
adamziel
pushed a commit
that referenced
this pull request
Apr 13, 2022
* Migrate post-editor-template-mode to Playwright * Use core/preferences store
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?
Part of #38851. Migrate
post-editor-template-mode.test.jsto Playwright.Hopefully fixes #34805, fixes #39120, fixes #39121, fixes #38594, fixes #38595.
Why?
See this post for an overview of the migration.
How?
By following the migration guide.
Testing Instructions