-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Cover Block tests to Playwright #45784
Migrate Cover Block tests to Playwright #45784
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @worldomonation! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Hi folks - this is my first PR in the Gutenberg repo. As I understand it, I must wait for the tests to be triggered manually. |
Looks like my run at https://github.com/WordPress/gutenberg/actions/runs/3474015855 failed. I will need to investigate why it is failing on CI and not locally. |
Hi! I only just saw this now 😞 . Sorry for the late reply! And thank you for your contribution! |
@kevin940726 I have a quick question regarding the When I call the aforementioned method in one of the specs I am migrating, the following error is thrown at the end:
I'm do not understand why the |
- update spec with accessibility-based selectors where possible.
@worldomonation Could you share a branch so that I can test locally? |
Sure, my work is in the forked repo |
@worldomonation I just checked out your code and the test passed locally for me. Could you try again? |
Thanks for trying. I had marked the step with the issue, |
@worldomonation Yep, I unskipped it. |
I think I figured it out. It now passes for me locally too. Thanks for helping me work through the issues. I hope to have this ready for review soon. |
- rebase, rebuild and update the selectors
- remove Puppeteer file.
- clarify use of `span` element in the first spec.
…-box__handle-bottom'. - move image upload and filename methods previously handled by `getTestImage` into a POM-like function at bottom of file.
…at locator as starting point for all other interactions. - use `getComputedStyle` instad of `node.style.<attribute>`. - replace implementation detail check on image upload with a `expect.toPass` check. - remove steps when resizing box. - replace implementation detail check for coverbox height with truthy check.
@kevin940726 I left two questions - 1 and 2 that I needed further clarification on how to proceed. I'm more than happy for more rounds of code reviews should you deem it necessary, so please feel free to take apart the revised spec. Thanks! |
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.
Thanks again for the updates! I left some suggestions and feedback below. Overall this is looking very good though! 💯
- add comment explaining the use of `span[aria-hidden="true"]`.
…esizing the block.
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.
Looking great! Just a small suggestion and I think we're good to go! Thank you so much for the hard work! 🙇
} ); | ||
} ); | ||
|
||
class ImageBlockUtils { |
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.
Maybe we should name it CoverBlockUtils
? 😆
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.
Makes sense. I thought maybe ImageBlockUtils
was always used as the name for the POM class, for some reason.
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.
Name changed to CoverBlockUtils
in 405c709.
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.
Thanks again! 💯
@worldomonation This is really well done! I couldn't have done it better myself! Also congrats on your first merged PR!
Congratulations on your first merged pull request, @worldomonation! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Thanks @kevin940726 for the many rounds of review. I really enjoyed working on this and would love to pick up a few more in the coming weeks! Thank you again. |
What?
This PR is part of the migration effort tracked at #38851.
Why?
This PR proposes a migration of the
cover.test.js
to the new Playwright-based framework.How?
Testing Instructions
npm run test:e2e:playwright -- test/e2e/specs/editor/blocks/cover.spec.js
In the current form (pending outcome of this question, it should produce the following outcome:
Screenshots or screencast