-
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 switch-to-draft
to Playwright
#48120
Conversation
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
Flaky tests detected in 74718d9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4229592408
|
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.
LGTM! I think the nested loops made the code look cleaner 👍
[ 'schedule', 'publish' ].forEach( ( postStatus ) => { | ||
[ 'large', 'small' ].forEach( ( viewport ) => { | ||
[ 'post', 'page' ].forEach( ( postType ) => { |
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.
This is a wild use for test loops, and I'm here for it 😄
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.
FWIW, it's what Playwright recommended too 😆 .
content: `<!-- wp:paragraph --> | ||
<p>This will be a scheduled ${ postType } edited in a ${ viewport } viewport</p> | ||
<!-- /wp:paragraph -->`, |
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.
What do you think about making those more readable like this?
content: [
`<!-- wp:paragraph -->`,
`<p>This will be a scheduled ${ postType } edited in a ${ viewport } viewport</p>`,
`<!-- /wp:paragraph -->`,
].join( '\n' ),
Possibly also less chance that accidental leading whitespace will kick in.
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 actually think this is less readable 😅 . It's only three lines so I think it's fine. For longer content, I agree your solution works better. However, I think a better option might be to allow passing blocks
to the createPost
/createPage
util and do the serializing automatically there. WDYT?
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.
Yep, that would work even better! 😄
43833d4
to
df91f45
Compare
df91f45
to
74718d9
Compare
What?
Part of #38851. Migrate
switch-to-draft
to Playwright to address some of the flakiest tests. Hopefully closes #47499 and closes #47500.Why?
The
switch-to-draft
e2e tests have been pretty flaky recently. After debugging the artifacts, it seems like they failed in an unrelated part when selecting dates in the scheduling date picker.How?
We already have a dedicated e2e test for scheduling posts or pages in
scheduling.test.js
, hence we don't need to duplicate that logic here forswitch-to-draft
tests.Furthermore, the original tests create published/scheduled posts and pages by interacting with the UI, which can be very slow and unnecessary. The refactored tests use REST API to create those posts and pages to speed up the tests.
Testing Instructions
CI should pass.
Screenshots or screencast