-
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
e2e-test-utils-playwright: Increase timeout of site-editor selector #66672
e2e-test-utils-playwright: Increase timeout of site-editor selector #66672
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alexflorisca! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
packages/e2e-test-utils-playwright/src/admin/visit-site-editor.ts
Outdated
Show resolved
Hide resolved
@@ -54,13 +54,13 @@ export async function visitSiteEditor( | |||
|
|||
// Wait for the canvas loader to appear first, so that the locator that | |||
// waits for the hidden state doesn't resolve prematurely. | |||
await canvasLoader.waitFor( { state: 'visible' } ); | |||
await canvasLoader.waitFor( { state: 'visible', timeout: 60000 } ); |
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'm not sure this change will have any impact to be honest, The spinner is supposed to render right away, while all the other asynchronous stuff are being loaded.
Did you actually notice improvements with this change or is it a guess.
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.
It's just a guess. But we have pretty consistent test failures due to elements on the page not being available after timeout period. So I do think it will help, and it can't hurt!
The spinner is supposed to render right away,
We need to check elements on the page, after page has loaded, so I do think this will help
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'm not convinced to be honest. For me, this change is really useless as we already have a timeout in the next line. Do you have any example of consistent test failure that we can debug further.
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 where the test fails, waiting for the canvasLoader to be visible. With cache dissabled and a throttled network connection of Fast 4g in chrome, it takes close to 9s have the spinner visible on my local machine. So that's cutting it pretty fine. I think this allows a bit more leeway for congested or slow gh action containers.
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 the flaky test that started this woocommerce/woocommerce#52404
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.
Ok that's convincing enough for me. I'm happy to ship. 9s to see the spinner is a problem in itself though (even in 4G), you might want to check why is that as well.
What?
This PR increases the timeout of a selector in the
e2e-test-utils-playwright
package. It also fixes a typo in another timeout valueWhy?
This will hopefully solve some test flakiness that we're experiencing. The site editor loads around 500 assets. It takes around 5-6 seconds on my local machine to load just the canvas for the site editor. 10s timeout here is pretty low, so I've increased it to 60s
How?
We're just increasing the timeout from 10s to 60s.
Testing Instructions
visitSiteEditor
function from thee2e-test-utils-package
in an e2e test on a slow network connection. 2. Verify that the test doesn't timeout after 10s.Testing Instructions for Keyboard
n/a>
Screenshots or screencast
n/a