Skip to content
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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 10, 2024
+1 −1
Merged

e2e-test-utils-playwright: Increase timeout of site-editor selector #66672

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38068e4
Increase timeout of site-editor selector
alexflorisca Nov 1, 2024
a5e79cf
Add the separator back in
alexflorisca Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/e2e-test-utils-playwright/src/admin/visit-site-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 } );
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

await canvasLoader.waitFor( {
state: 'hidden',
// Bigger timeout is needed for larger entities, like the Large Post
// HTML fixture that we load for performance tests, which often
// doesn't make it under the default timeout value.
timeout: 60_000,
timeout: 60000,
alexflorisca marked this conversation as resolved.
Show resolved Hide resolved
} );
}

Expand Down
Loading