cua-driver: add launch_app visible-window test, remove stale integration tests - #1438
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR restructures integration tests for the CUA driver on macOS. Three existing test modules that validated focus preservation during background operations (Blender interaction, double-click delivery, pixel-click delivery) are removed, and a new test module validating ChangesIntegration Test Restructuring
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/cua-driver/Tests/integration/test_launch_app_visible.py (1)
217-222: ⚡ Quick winAssertion message f-strings eagerly evaluate
call_tool— extra IPC call on every passing testPython evaluates all arguments to
assertGreater(including the message) before the assertion logic runs. Theself.client.call_tool('list_windows', ...)inside the f-string is therefore invoked on every test execution, not only on failure. The same pattern repeats at lines 272–277 and 320–325.A simple fix is to capture
all_windowsfrom the samelist_windowsresult already fetched inside_on_screen_windows, or by refactoring_on_screen_windowsto also return the unfiltered list:♻️ Example refactor for `test_01` (apply the same pattern to `test_02`/`test_03`)
- on_screen = _on_screen_windows(self.client, pid) - losses = _read_focus_losses() - losses_before - - self.assertGreater( - len(on_screen), 0, - f"launch_app(TextEdit) must produce at least one on-screen window. " - f"pid={pid}. All windows: " - f"{self.client.call_tool('list_windows', {'pid': pid}).get('structuredContent', {}).get('windows', [])}" - ) + raw = self.client.call_tool("list_windows", {"pid": pid}) + all_windows = raw.get("structuredContent", {}).get("windows", []) + on_screen = [ + w for w in all_windows + if w.get("is_on_screen") + and (w.get("bounds", {}).get("width", 0) or 0) > 50 + and (w.get("bounds", {}).get("height", 0) or 0) > 50 + ] + losses = _read_focus_losses() - losses_before + + self.assertGreater( + len(on_screen), 0, + f"launch_app(TextEdit) must produce at least one on-screen window. " + f"pid={pid}. All windows: {all_windows}" + )If the inline filter duplication is a concern, extend
_on_screen_windowsto return a(on_screen, all_windows)tuple instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/cua-driver/Tests/integration/test_launch_app_visible.py` around lines 217 - 222, The assertion message eagerly calls self.client.call_tool('list_windows', ...) inside the f-string causing extra IPC on every passing test; modify the test to avoid side-effectful message construction by having _on_screen_windows return both (on_screen, all_windows) or by capturing all_windows from the same list_windows result before the assert, then call self.assertGreater(len(on_screen), 0) and only build the detailed failure message (including all_windows) when the assertion fails (e.g., inside an if not on_screen: raise AssertionError(...)); update references in test_launch_app_visible.py (tests test_01/test_02/test_03 and the helper _on_screen_windows) accordingly so the assert message no longer invokes call_tool unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/Tests/integration/test_launch_app_visible.py`:
- Around line 163-170: After calling DriverClient(cls.binary).__enter__() in
setUpClass, immediately register a class cleanup that will call
cls.client.__exit__ so the client is always closed even if later setup (e.g.,
_build_focus_app or _launch_focus_app) raises; specifically, after the __enter__
call invoke cls.addClassCleanup(cls.client.__exit__, None, None, None) (or
equivalent) so DriverClient cleanup is guaranteed, and remove or adjust any
duplicate explicit __exit__ calls in tearDownClass to avoid double-cleanup.
- Around line 84-91: The test currently uses blocking proc.stdout.readline() in
a loop which can hang indefinitely; change the wait to a deadline-based
non-blocking read using select on proc.stdout.fileno() (or equivalent) to
enforce the timeout: repeatedly use select.select([proc.stdout], [], [],
remaining_timeout) and only call proc.stdout.readline() when select indicates
data is ready, then parse for the "FOCUS_PID=" line (the code that extracts pid
with line.split("=", 1)[1]); if the deadline elapses, terminate proc and raise
the same RuntimeError about FocusMonitorApp not printing FOCUS_PID in time.
Ensure you update the function that returns (proc, pid) to use this select-based
waiting instead of the blocking readline loop.
---
Nitpick comments:
In `@libs/cua-driver/Tests/integration/test_launch_app_visible.py`:
- Around line 217-222: The assertion message eagerly calls
self.client.call_tool('list_windows', ...) inside the f-string causing extra IPC
on every passing test; modify the test to avoid side-effectful message
construction by having _on_screen_windows return both (on_screen, all_windows)
or by capturing all_windows from the same list_windows result before the assert,
then call self.assertGreater(len(on_screen), 0) and only build the detailed
failure message (including all_windows) when the assertion fails (e.g., inside
an if not on_screen: raise AssertionError(...)); update references in
test_launch_app_visible.py (tests test_01/test_02/test_03 and the helper
_on_screen_windows) accordingly so the assert message no longer invokes
call_tool unconditionally.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6618c776-16f8-43a0-aec6-3e340214607b
📒 Files selected for processing (4)
libs/cua-driver/Tests/integration/test_blender_background.pylibs/cua-driver/Tests/integration/test_double_click_delivery.pylibs/cua-driver/Tests/integration/test_launch_app_visible.pylibs/cua-driver/Tests/integration/test_pixel_click_delivery.py
💤 Files with no reviewable changes (3)
- libs/cua-driver/Tests/integration/test_pixel_click_delivery.py
- libs/cua-driver/Tests/integration/test_double_click_delivery.py
- libs/cua-driver/Tests/integration/test_blender_background.py
Comment on lines
+84
to
+91
| for _ in range(40): | ||
| line = proc.stdout.readline().strip() | ||
| if line.startswith("FOCUS_PID="): | ||
| pid = int(line.split("=", 1)[1]) | ||
| return proc, pid | ||
| time.sleep(0.1) | ||
| proc.terminate() | ||
| raise RuntimeError("FocusMonitorApp did not print FOCUS_PID in time") |
Contributor
There was a problem hiding this comment.
readline() is a blocking call — the 4-second "timeout" is illusory and the test runner can hang indefinitely
time.sleep(0.1) is only reached after readline() returns. If FocusMonitorApp starts but hangs before printing FOCUS_PID= (e.g., stalls on Accessibility permission dialogs, resource contention, etc.), readline() blocks forever and the test runner is stuck permanently. The 40-iteration loop provides no actual deadline.
🔒 Proposed fix: deadline-based loop with `select`
+import select as _select
+
def _launch_focus_app() -> tuple[subprocess.Popen, int]:
proc = subprocess.Popen(
[_FOCUS_APP_EXE],
stdout=subprocess.PIPE,
stderr=subprocess.DEVNULL,
text=True,
)
- for _ in range(40):
- line = proc.stdout.readline().strip()
- if line.startswith("FOCUS_PID="):
- pid = int(line.split("=", 1)[1])
- return proc, pid
- time.sleep(0.1)
- proc.terminate()
+ deadline = time.monotonic() + 4.0
+ while time.monotonic() < deadline:
+ remaining = deadline - time.monotonic()
+ ready, _, _ = _select.select([proc.stdout], [], [], max(0.0, remaining))
+ if not ready:
+ break
+ line = proc.stdout.readline().strip()
+ if line.startswith("FOCUS_PID="):
+ pid = int(line.split("=", 1)[1])
+ return proc, pid
+ proc.terminate()
raise RuntimeError("FocusMonitorApp did not print FOCUS_PID in time")📝 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
| for _ in range(40): | |
| line = proc.stdout.readline().strip() | |
| if line.startswith("FOCUS_PID="): | |
| pid = int(line.split("=", 1)[1]) | |
| return proc, pid | |
| time.sleep(0.1) | |
| proc.terminate() | |
| raise RuntimeError("FocusMonitorApp did not print FOCUS_PID in time") | |
| deadline = time.monotonic() + 4.0 | |
| while time.monotonic() < deadline: | |
| remaining = deadline - time.monotonic() | |
| ready, _, _ = _select.select([proc.stdout], [], [], max(0.0, remaining)) | |
| if not ready: | |
| break | |
| line = proc.stdout.readline().strip() | |
| if line.startswith("FOCUS_PID="): | |
| pid = int(line.split("=", 1)[1]) | |
| return proc, pid | |
| proc.terminate() | |
| raise RuntimeError("FocusMonitorApp did not print FOCUS_PID in time") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/Tests/integration/test_launch_app_visible.py` around lines 84
- 91, The test currently uses blocking proc.stdout.readline() in a loop which
can hang indefinitely; change the wait to a deadline-based non-blocking read
using select on proc.stdout.fileno() (or equivalent) to enforce the timeout:
repeatedly use select.select([proc.stdout], [], [], remaining_timeout) and only
call proc.stdout.readline() when select indicates data is ready, then parse for
the "FOCUS_PID=" line (the code that extracts pid with line.split("=", 1)[1]);
if the deadline elapses, terminate proc and raise the same RuntimeError about
FocusMonitorApp not printing FOCUS_PID in time. Ensure you update the function
that returns (proc, pid) to use this select-based waiting instead of the
blocking readline loop.
Comment on lines
+163
to
+170
| cls.binary = default_binary_path() | ||
| cls.client = DriverClient(cls.binary).__enter__() | ||
|
|
||
| # Build + launch FocusMonitorApp BEFORE launch_app so it owns the | ||
| # foreground while we measure focus losses. | ||
| _build_focus_app() | ||
| cls.fm_proc, cls.fm_pid = _launch_focus_app() | ||
| time.sleep(0.8) |
Contributor
There was a problem hiding this comment.
DriverClient leaks if setUpClass raises after __enter__
Python's unittest skips tearDownClass when setUpClass raises. If _build_focus_app() or _launch_focus_app() throws after line 164, cls.client.__exit__ is never called, leaking whatever resources the DriverClient holds.
🛡️ Proposed fix: guard `DriverClient` cleanup on setup failure
cls.binary = default_binary_path()
cls.client = DriverClient(cls.binary).__enter__()
- # Build + launch FocusMonitorApp BEFORE launch_app so it owns the
- # foreground while we measure focus losses.
- _build_focus_app()
- cls.fm_proc, cls.fm_pid = _launch_focus_app()
- time.sleep(0.8)
+ try:
+ # Build + launch FocusMonitorApp BEFORE launch_app so it owns the
+ # foreground while we measure focus losses.
+ _build_focus_app()
+ cls.fm_proc, cls.fm_pid = _launch_focus_app()
+ time.sleep(0.8)
+ except Exception:
+ cls.client.__exit__(None, None, None)
+ raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/Tests/integration/test_launch_app_visible.py` around lines
163 - 170, After calling DriverClient(cls.binary).__enter__() in setUpClass,
immediately register a class cleanup that will call cls.client.__exit__ so the
client is always closed even if later setup (e.g., _build_focus_app or
_launch_focus_app) raises; specifically, after the __enter__ call invoke
cls.addClassCleanup(cls.client.__exit__, None, None, None) (or equivalent) so
DriverClient cleanup is guaranteed, and remove or adjust any duplicate explicit
__exit__ calls in tearDownClass to avoid double-cleanup.
…ion tests Add test_launch_app_visible.py covering: - TextEdit cold launch → on-screen window, 0 focus losses - Calculator cold launch → on-screen window, focus-steal suppressor restores prior foreground - Finder no-URL launch → on-screen window via home-directory fallback, 0 focus losses Remove test_blender_background.py, test_double_click_delivery.py, test_pixel_click_delivery.py — no longer maintained. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Swift changes: - FocusWithoutRaise: add withMenuShortcutActivation() — saves/restores frontmost PSN around key delivery so NSMenu shortcuts fire on backgrounded apps without any observable focus change (< 1ms, invisible to UXMonitor) - SkyLightEventPost: add setFrontProcessNoWindows, getProcessPSN SPIs - KeyboardInput: add attachAuthMessage param to route via IOHIDPostEvent - HotkeyTool: use withMenuShortcutActivation for window_id path - PressKeyTool: same NSMenu-safe path - ListWindowsTool / ListAppsTool: add structuredContent output - TypeTextTool: CGEvent fallback for browser web inputs - SetValueTool: AXPopUpButton child-pick + JS injection for Safari selects Test harness: - New v2 harness (harness/driver.py, monitor.py, tree.py, cv.py) - conftest.py: focus_monitor, ux_guard, html_server, tauri/electron fixtures - test_safari.py: 10 tests, all UX-guarded - test_chrome.py: 6 tests; probe_ax fallback + URL-bar skip in find_text_field - test_launch_app_visible.py: 3 tests - test_background_menu_shortcut.py: 2 tests, both ux_guard-clean Bug fixes: - driver.py: fix tree extraction regex (was r'- \[' missing indented lines); now r'\n\n(- .+)' captures full tree block from text content - find_text_field: skip Chrome "Address and search bar" alongside Safari's "smart search field" - find_window: add probe_ax option to fall back to window with AX content Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Files kept on disk (untracked); will land in a separate PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ddupont808
force-pushed
the
cua-driver/test-cleanup
branch
from
May 4, 2026 21:44
b25a73b to
de2feaf
Compare
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.
Summary
test_launch_app_visible.pycovering thelaunch_appbackground-visible-window contract:activates=falseholds)test_blender_background.py,test_double_click_delivery.py,test_pixel_click_delivery.py— no longer maintainedTest plan
scripts/test.sh test_launch_app_visible→ 3 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
Tests