[screenshot] update wait times#6293
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the automated visual regression testing process by extending the page load timeout. It also simplifies the configuration for ranking page snapshots by removing an obsolete feature flag, ensuring the snapshots reflect the current production state. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request increases the page load sleep time from 5 to 60 seconds in the Percy snapshot script and removes the disable_feature flag from ranking page URLs. Feedback suggests that the hard-coded 60-second sleep will significantly slow down the CI pipeline and recommends using dynamic waits or increasing the WAIT_TIMEOUT instead of a fixed sleep to improve efficiency.
| from selenium.webdriver.support.ui import WebDriverWait | ||
|
|
||
| WAIT_TIMEOUT = 15 | ||
| PAGE_LOAD_SECONDS = 60 |
There was a problem hiding this comment.
Increasing the hard sleep time to 60 seconds per page will significantly slow down the CI pipeline. Since shards process URLs sequentially (due to max_workers=1), this adds a minimum of 1 minute per URL. Consider using a more dynamic wait (e.g., WebDriverWait for a specific element that indicates the page is fully rendered) to improve efficiency.
| f"[PID {os.getpid()}] Warning: Timed out waiting for primary element (body) on '{name}'. Proceeding with fallback sleep. ⏳" | ||
| ) | ||
| time.sleep(5) | ||
| time.sleep(PAGE_LOAD_SECONDS) |
There was a problem hiding this comment.
If the initial wait for the body element times out, it is unlikely that a 60-second sleep will result in a successful snapshot. If some pages are known to be slow, it would be more efficient to increase the WAIT_TIMEOUT so the script can proceed as soon as the element appears, rather than always waiting the full 60 seconds in the error path.
Increased screenshot wait times
Removed ranking feature flag