fix(test): increase Selenium UI test timeouts#2743
Closed
anusshukla wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughTiming and retry parameters were increased across UI tests, a browser setup delay was added, and the Selenium service healthcheck now requires two endpoints to be healthy before proceeding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ui/test_01_dev_webviews.py (1)
27-27: Consider centralizing this retry budget instead of hard-coding20twice.These are valid CI-hardening changes, but duplicating retry counts makes later tuning harder (and worst-case misses now wait much longer). A shared constant (or timeout-based helper wrapper) would keep both tests consistent and easier to calibrate.
♻️ Suggested small refactor
+FORM_READY_RETRIES = 20 ... - find_element_across_iframes(driver, "//form[`@id`='devfile-form']", retries=20) + find_element_across_iframes(driver, "//form[`@id`='devfile-form']", retries=FORM_READY_RETRIES) ... - retries=20, + retries=FORM_READY_RETRIES,Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_01_dev_webviews.py` at line 27, The test hard-codes the retry count (20) when calling find_element_across_iframes in test_01_dev_webviews.py (lines showing calls to find_element_across_iframes at start and around line 73); centralize this retry budget by introducing a shared constant (e.g., DEFAULT_IFRAME_RETRIES or IFRAME_FIND_RETRIES) or a timeout-based helper used by both call sites, replace the literal 20 with that constant/helper in both calls, and update any related tests to import/use the new constant so tuning happens in one place.test/ui/test_00_commands.py (1)
46-46: Prefer condition-based waits over longer fixed sleeps here.These increases will help CI, but hard sleeps can still be flaky and slow. Consider waiting on a concrete UI condition (terminal prompt/output ready, or editor lines present) instead of unconditional sleeps.
Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_00_commands.py` at line 46, Replace the fixed time.sleep(5) (and the similar sleep at the later occurrence) with a condition-based wait: poll the actual UI/terminal readiness (e.g., loop until terminal prompt/output appears, editor buffer has expected lines, or a helper like terminal.read_until()/expect()/get_lines() returns the expected content) with a short sleep interval and a total timeout, then assert readiness; update calls to time.sleep(...) in test_00_commands.py (the explicit time.sleep lines) to use this loop/utility so CI waits on a concrete condition rather than an unconditional long sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ui/test_00_commands.py`:
- Line 46: Replace the fixed time.sleep(5) (and the similar sleep at the later
occurrence) with a condition-based wait: poll the actual UI/terminal readiness
(e.g., loop until terminal prompt/output appears, editor buffer has expected
lines, or a helper like terminal.read_until()/expect()/get_lines() returns the
expected content) with a short sleep interval and a total timeout, then assert
readiness; update calls to time.sleep(...) in test_00_commands.py (the explicit
time.sleep lines) to use this loop/utility so CI waits on a concrete condition
rather than an unconditional long sleep.
In `@test/ui/test_01_dev_webviews.py`:
- Line 27: The test hard-codes the retry count (20) when calling
find_element_across_iframes in test_01_dev_webviews.py (lines showing calls to
find_element_across_iframes at start and around line 73); centralize this retry
budget by introducing a shared constant (e.g., DEFAULT_IFRAME_RETRIES or
IFRAME_FIND_RETRIES) or a timeout-based helper used by both call sites, replace
the literal 20 with that constant/helper in both calls, and update any related
tests to import/use the new constant so tuning happens in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 508ceedc-539f-4a74-bb1e-1d510bbb9097
📒 Files selected for processing (3)
test/ui/test_00_commands.pytest/ui/test_01_dev_webviews.pytest/ui/utils/ui_utils.py
anusshukla
force-pushed
the
fix/ui-selenium-timeouts
branch
from
9f784d5 to
13039fa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
The container healthcheck only verified Selenium (port 4444) but not code-server (port 8080), so tests could start before VS Code was ready. Test-level timeouts were also too aggressive for CI. - Check both Selenium and code-server in healthcheck - Wait for extension activation after container becomes healthy - Retry page load in ensure_vscode_ready on timeout - Increase timeouts: command palette 2s->5s, tab wait 2s->10s, terminal output 10s->30s, webview form retries 10->20 related: #2741 Made-with: Cursor
anusshukla
force-pushed
the
fix/ui-selenium-timeouts
branch
from
13039fa to
6e90bd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ui/fixtures/ui_fixtures.py (1)
92-95: Make the post-healthcheck wait configurable instead of hard-coded.This unconditional 30s pause adds fixed latency to every session and may still be too short/long depending on runner speed. Prefer an env-configurable delay.
Proposed change
log.info( "Container healthy, waiting for code-server extension activation..." ) - time.sleep(30) + activation_wait_seconds = int( + os.environ.get("UI_EXTENSION_ACTIVATION_WAIT_SECONDS", "30") + ) + if activation_wait_seconds > 0: + time.sleep(activation_wait_seconds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/fixtures/ui_fixtures.py` around lines 92 - 95, Replace the hard-coded time.sleep(30) in test/ui/fixtures/ui_fixtures.py with a configurable delay sourced from an environment variable (e.g., UI_POST_HEALTHCHECK_DELAY) and a sensible default of 30s; parse and validate the value (int, non-negative) into a variable like post_healthcheck_delay and call time.sleep(post_healthcheck_delay), and update the log.info message (or add a new log.debug) to include the chosen delay so the behavior is explicit at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ui/utils/ui_utils.py`:
- Around line 81-90: The loop in ensure_vscode_ready uses fixed 90s waits and
fixed 10s sleeps, which can exceed the caller-supplied timeout; change it to
track elapsed time (e.g. via time.monotonic()) and compute a remaining_time
budget for each iteration when calling wait_displayed(driver,
"//a[`@aria-label`='Ansible']", timeout=remaining_time) and for any sleep (sleep
only min(10, remaining_time)), stop retrying and raise if remaining_time <= 0;
update handling around exceptions (TimeoutException, TimeOutError) and the
driver.refresh() call so they respect the remaining timeout budget.
---
Nitpick comments:
In `@test/ui/fixtures/ui_fixtures.py`:
- Around line 92-95: Replace the hard-coded time.sleep(30) in
test/ui/fixtures/ui_fixtures.py with a configurable delay sourced from an
environment variable (e.g., UI_POST_HEALTHCHECK_DELAY) and a sensible default of
30s; parse and validate the value (int, non-negative) into a variable like
post_healthcheck_delay and call time.sleep(post_healthcheck_delay), and update
the log.info message (or add a new log.debug) to include the chosen delay so the
behavior is explicit at runtime.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06fb6469-5d4d-4f8d-a255-5471c810b78a
📒 Files selected for processing (5)
docker-compose.ymltest/ui/fixtures/ui_fixtures.pytest/ui/test_00_commands.pytest/ui/test_01_dev_webviews.pytest/ui/utils/ui_utils.py
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ui/test_01_dev_webviews.py
- test/ui/test_00_commands.py
Comment on lines
+81
to
+90
| max_nav_attempts = 3 | ||
| for attempt in range(max_nav_attempts): | ||
| try: | ||
| wait_displayed(driver, "//a[@aria-label='Ansible']", timeout=90) | ||
| break | ||
| except (TimeoutException, TimeOutError): | ||
| if attempt == max_nav_attempts - 1: | ||
| raise | ||
| driver.refresh() | ||
| time.sleep(10) |
There was a problem hiding this comment.
ensure_vscode_ready currently bypasses its timeout budget.
The new loop uses fixed 90s attempts and 10s sleeps, so total wait can exceed the function’s timeout argument. Use a remaining-time budget tied to timeout.
Proposed change
- max_nav_attempts = 3
- for attempt in range(max_nav_attempts):
+ max_nav_attempts = 3
+ deadline = time.monotonic() + timeout
+ for attempt in range(max_nav_attempts):
+ remaining = int(deadline - time.monotonic())
+ if remaining <= 0:
+ raise TimeOutError("timeout waiting for VSCode Ansible icon")
try:
- wait_displayed(driver, "//a[`@aria-label`='Ansible']", timeout=90)
+ wait_displayed(
+ driver,
+ "//a[`@aria-label`='Ansible']",
+ timeout=min(remaining, 90),
+ )
break
except (TimeoutException, TimeOutError):
if attempt == max_nav_attempts - 1:
raise
driver.refresh()
- time.sleep(10)
+ time.sleep(min(10, max(0, deadline - time.monotonic())))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| max_nav_attempts = 3 | |
| for attempt in range(max_nav_attempts): | |
| try: | |
| wait_displayed(driver, "//a[@aria-label='Ansible']", timeout=90) | |
| break | |
| except (TimeoutException, TimeOutError): | |
| if attempt == max_nav_attempts - 1: | |
| raise | |
| driver.refresh() | |
| time.sleep(10) | |
| max_nav_attempts = 3 | |
| deadline = time.monotonic() + timeout | |
| for attempt in range(max_nav_attempts): | |
| remaining = int(deadline - time.monotonic()) | |
| if remaining <= 0: | |
| raise TimeOutError("timeout waiting for VSCode Ansible icon") | |
| try: | |
| wait_displayed( | |
| driver, | |
| "//a[`@aria-label`='Ansible']", | |
| timeout=min(remaining, 90), | |
| ) | |
| break | |
| except (TimeoutException, TimeOutError): | |
| if attempt == max_nav_attempts - 1: | |
| raise | |
| driver.refresh() | |
| time.sleep(min(10, max(0, deadline - time.monotonic()))) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui/utils/ui_utils.py` around lines 81 - 90, The loop in
ensure_vscode_ready uses fixed 90s waits and fixed 10s sleeps, which can exceed
the caller-supplied timeout; change it to track elapsed time (e.g. via
time.monotonic()) and compute a remaining_time budget for each iteration when
calling wait_displayed(driver, "//a[`@aria-label`='Ansible']",
timeout=remaining_time) and for any sleep (sleep only min(10, remaining_time)),
stop retrying and raise if remaining_time <= 0; update handling around
exceptions (TimeoutException, TimeOutError) and the driver.refresh() call so
they respect the remaining timeout budget.
5 tasks
github-project-automation
Bot
moved this from In Progress
to Done
in 🧰 devtools project board
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Timeouts calibrated for local dev were too aggressive for CI runners, causing cascading TimeoutExceptions across all three non-Lightspeed UI tests.
related: #2741
Increases Selenium UI test timeouts and retries to prevent CI flakiness caused by CI being slower than local runs; approach: raise waits, poll intervals, retry counts, and add post-healthcheck delay.
related: #2741