-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[code-infra] Tentative fix for Argos flaky screenshot tests #15374
[code-infra] Tentative fix for Argos flaky screenshot tests #15374
Conversation
@@ -62,7 +62,7 @@ async function main() { | |||
|
|||
// Wait for all requests to finish. | |||
// This should load shared resources such as fonts. | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle0' }); | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle' }); |
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 seemed a bug
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.
Are you sure? 🤔
https://www.webshare.io/academy-article/puppeteer-networkidle0-vs-networkidle2
it seems like it is correct. 🤔
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.
We are using playwright, the types flag that as a type error
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, nice. 👍
It's a difference between Playwright and Puppeteer. 🙈
https://playwright.dev/docs/api/class-page#page-goto-option-wait-until
Deploy preview: https://deploy-preview-15374--material-ui-x.netlify.app/ |
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.
Nice improvement, thanks for looking into this! 🙏 👍
@@ -62,7 +62,7 @@ async function main() { | |||
|
|||
// Wait for all requests to finish. | |||
// This should load shared resources such as fonts. | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle0' }); | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle' }); |
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.
Are you sure? 🤔
https://www.webshare.io/academy-article/puppeteer-networkidle0-vs-networkidle2
it seems like it is correct. 🤔
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.
Let's test this. 🚀 🤞
@@ -62,7 +62,7 @@ async function main() { | |||
|
|||
// Wait for all requests to finish. | |||
// This should load shared resources such as fonts. | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle0' }); | |||
await page.goto(`${baseUrl}#no-dev`, { waitUntil: 'networkidle' }); |
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, nice. 👍
It's a difference between Playwright and Puppeteer. 🙈
https://playwright.dev/docs/api/class-page#page-goto-option-wait-until
WDYT, @JCQuintas, should we cherry-pick this to v7.x as we'll be doing a lot of PRs for that branch? 🤔 |
probably, yeah |
(cherry picked from commit 60d0913)
page.waitForLoadState()
before screenshot