-
Notifications
You must be signed in to change notification settings - Fork 36
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
fix graybox-cc screenshot tests #466
Conversation
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.
Approved, but I did leave a couple comments/questions to consider that may or may not be applicable. Just something to consider.
stableURL, | ||
async () => { | ||
await page.waitForLoadState('networkidle'); | ||
await page.waitForTimeout(8000); |
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.
how arbitrary are these timeouts? Is there a better way for waiting on the page to fully load, like waiting for a specific element in the last section/block to load completely. Just wondering since there may be a potential for these timeouts to fluctuate.
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.
because we test different pages in CC, maybe we can find one, but when I test DC, I am not able to find one in all pages
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.
If it works, just a thought, you could make a parameter that allows you to send in an element to look for or by default if nothing is sent in use the arbitrary timeouts. Not a requirement, but could potentially help if the tests become flaky due to the timeouts.
betaURL, | ||
async () => { | ||
await page.waitForLoadState('networkidle'); | ||
await page.waitForTimeout(10000); |
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.
Same as above comment.
fix graybox-cc screenshot tests