-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Update Playwright to 1.44.1 from 1.41.1 #48291
Conversation
Hi @WunderBart, @gigitux, @woocommerce/vortex Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
Size Change: +10 B (0%) Total Size: 2.47 MB |
@WunderBart @gigitux -- I'd like to update Playwright to the latest version in the Monorepo (we're about 3 versions old currently). Looks like everything is okay, except there's one Blocks Playwright test that seems to have turned flaky with this PR. |
Releasing the `page.goto` before the `load` event (default) doesn't really speed up the tests because we still need to wait for the locator that we're going to interact with. Furthermore, it can destabilize the test because the said locator timer starts earlier than it normally would (after page "load"), so it might timeout in a situation when a page takes longer to load than usually.
8877a70
to
1d627f3
Compare
// The mini cart button scripts are loaded when the button is either | ||
// hovered or focused. The click event alone does not trigger neither of | ||
// those actions so we need to perform one explicitly. | ||
await miniCartButton.hover(); | ||
await miniCartButton.click(); |
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 should fix the failing mini cart tests. Those tests failed because the cart scripts aren't loaded until the cart button is explicitly either hovered or focused. This is an implementation detail and, ideally, it should not be necessary to account for it in the test. However, it is weird that the Playwright's physical click event does not seem to lead with hover
, as it's only possible to click with hovering first. I'll try to investigate whether it's a bug or an intentional change with the newer release.
cc: @Aljullu, since you last worked on mini cart frontend scripts.
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 for the explanation, @WunderBart! It makes sense to me. 👍
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've investigated it a bit further, and the issue seems to be that the mini cart button scripts (loaded lazily on button hover) aren't hydrating the button element — they remove it and replace it with a new one. This means there's no continuity in the click handler, which should be enough to load the scripts lazy and open the mini cart drawer. A workaround could be re-querying the button, for which I put together a PoC here: #48330.
In general, the current implementation should not be an issue with fast machines/connection, but it can cause further flakiness due to the disconnection between the button being ready to be clicked and the actual click happening. Let me know if it makes sense.
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've investigated it a bit further, and the issue seems to be that the mini cart button scripts (loaded lazily on button hover) aren't hydrating the button element — they remove it and replace it with a new one. This means there's no continuity in the click handler, which should be enough to load the scripts lazy and open the mini cart drawer.
Right, good catch! I didn't think that was the reason of the issue, but it makes total sense. 💯
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 know the issue is clear, but let me share these awesome diagrams I created by explaining the issue to GPT and asking for a mermaid
output. 😅 TIL that it works that well! 💪
Successful scenario
sequenceDiagram
participant User
participant CartButton
participant Scripts
User->>CartButton: Hover
CartButton->>Scripts: Trigger lazy load
Scripts-->>CartButton: Scripts loaded
User->>CartButton: Click
CartButton->>Scripts: Open cart drawer
Scripts-->>User: Cart drawer opened
Failure scenario
sequenceDiagram
participant User
participant CartButton
participant Scripts
User->>CartButton: Hover
CartButton->>Scripts: Trigger lazy load
User->>CartButton: Click
CartButton->>User: No action
Scripts-->>CartButton: Scripts loaded
Not sure why the JS Unit tests are having issues. Seems to be different ones, so maybe just flaky? |
Looks like pnpm 9.1.4 is breaking |
🤔 I wonder if the lint errors 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.
Looks good. Thank you @lanej0 for this much needed update.
Co-authored-by: Jon Lane <jon.lane@automattic.com> Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com> Co-authored-by: Adrian Moldovan <3854374+adimoldovan@users.noreply.github.com>
Submission Review Guidelines:
Changes proposed in this Pull Request:
We had fallen a bit behind in Playwright in the monorepo, mostly because Playwright introduced this breaking change a few versions back. This PR updates Playwright to the latest release and refactors some shopper/merchant tests in core so that they're compatible with the latest version.
Also updates the Blocks e2e tests to the latest version (ran tests, and no apparent issues with new version of Playwright)
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
pnpm env:restart && pnpm test:e2e-pw
(for core tests). Will probably be prompted to install updated browsers first.pnpm env:restart && pnpm test:e2e
(for blocks tests)Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment