[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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
This really less about this PR, and more about how this was originally set up: i.e. if we wait up to 15 seconds (WAIT_TIMEOUT) for the body tag to load, and then give it another hard 5 seconds (or 60 seconds), then the question is: why not just include that time in the original WebDriverWait.
We're already allowing a maximum of 15 + 60 seconds for the body to load, so why not just allow 60 seconds for the body to load directly in the WebDriverWait. That way the 60 second fallback can be broken out of rather than being hard.
Then we would have the overall PAGE_LOAD_WAIT happen outside of the try/except (since it is really unrelated to the body tag, and rather for giving time for the pages themselves to load).
It is not something we need to deal with here, but maybe we could look at a TODO to make this part more efficient.
| 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.
This really less about this PR, and more about how this was originally set up: i.e. if we wait up to 15 seconds (WAIT_TIMEOUT) for the body tag to load, and then give it another hard 5 seconds (or 60 seconds), then the question is: why not just include that time in the original WebDriverWait.
We're already allowing a maximum of 15 + 60 seconds for the body to load, so why not just allow 60 seconds for the body to load directly in the WebDriverWait. That way the 60 second fallback can be broken out of rather than being hard.
Then we would have the overall PAGE_LOAD_WAIT happen outside of the try/except (since it is really unrelated to the body tag, and rather for giving time for the pages themselves to load).
It is not something we need to deal with here, but maybe we could look at a TODO to make this part more efficient.
| from selenium.webdriver.support.ui import WebDriverWait | ||
|
|
||
| WAIT_TIMEOUT = 15 | ||
| PAGE_LOAD_SECONDS = 60 |
There was a problem hiding this comment.
This will include a constant 60 second per page wait on every page, including pages where we probably don't need it.
I'm thinking about pages like the home page, /version, /about, /faq.
We could add a flag into the urls.json to flag certain pages a not needing a long wait (because there is extensive CSR, we do still need a short wait, but that could be very short).
| from selenium.webdriver.support.ui import WebDriverWait | ||
|
|
||
| WAIT_TIMEOUT = 15 | ||
| PAGE_LOAD_SECONDS = 60 |
There was a problem hiding this comment.
This bumps the wait from 5 seconds (which we have seen in practice is too short) to 60 seconds. The only downside of 60 seconds is the longer wait time for the tests to run.
Should we try a wait between those two as a first go, to see if that resolves the partial page loads, before increasing it? Maybe even 10 or 15 seconds would be enough, and if not, we could bump it up.
There was a problem hiding this comment.
In the long turn, it would be nice to turn the hard waits into soft waits (i.e., for the appearance or disappearance of a particular element). However, that isn't trivial in our case, because the snapshot test is not page-specific (but the logic required would be). This is something we can revisit (and going forward with any new repos where we use Percy, we can consider this ahead of time. For now, I think we have to keep some form of hard wait.
2 participants
Increased screenshot wait times
Removed ranking feature flag