Use CDP for standalone render smoke - #402
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChromium sessions now enforce shared launch, connection, page-setup, and shutdown deadlines. Failed WebSocket, browser, target, and file operations clean up resources. The render smoke check now uses CDP through ChangesChromium timeout and cleanup flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant run_probe
participant ChromiumSession
participant PageSession
run_probe->>ChromiumSession: launch with shared deadline
run_probe->>PageSession: navigate and wait for load
PageSession-->>run_probe: return document.title
run_probe->>ChromiumSession: stop browser after result publication
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_render_smoke_nonumpy.py (2)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
exceptionDetailsbranch.The fake
_callalways returns a clean result, sorun_probenever reaches theRuntimeError(f"probe page exception: ...")branch inscripts/render_smoke_nonumpy.pyline 95. A third test that returns{"exceptionDetails": {...}}would pin that failure path, which is how a broken probe page surfaces to the operator.💚 Sketch of the added test
def test_run_probe_reports_a_page_exception( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: clock = _Clock() page_path = tmp_path / "probe.html" class ThrowingSession: def __init__(self, _executable: str, **_kwargs) -> None: pass def __enter__(self): return self def __exit__(self, _exc_type, _exc, _tb) -> None: # noqa: ANN001 pass def _page_session(self, _page: str, _timeout_s: float): return "target", "session", page_path def _call(self, _method: str, _params=None, **_kwargs): # noqa: ANN001 return {"exceptionDetails": {"text": "boom"}} def _wait_event(self, *_args, **_kwargs) -> None: # noqa: ANN002, ANN003 pass monkeypatch.setattr(render_smoke.time, "monotonic", clock.monotonic) monkeypatch.setattr(render_smoke.time, "sleep", clock.sleep) monkeypatch.setattr(render_smoke, "find_chromium", lambda: "/fake/chromium") monkeypatch.setattr(render_smoke, "ChromiumSession", ThrowingSession) with pytest.raises(RuntimeError, match="probe page exception"): render_smoke.run_probe("<title>pending</title>", timeout_s=300.0)🤖 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 `@tests/test_render_smoke_nonumpy.py` at line 84, Extend the run_probe test coverage with a third test using a fake ChromiumSession whose _call method returns an exceptionDetails response. Keep the existing session, timing, and page setup pattern, then assert that run_probe raises RuntimeError matching “probe page exception” so the page-failure branch is exercised.
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the session exits on the timeout path.
The stated goal of this PR is that a real hang leaves no Chrome process behind.
StallingSession.__exit__is a no-op and no assertion records that it ran. Record the call and assert it, so a future refactor that movesrun_probeoff the context manager fails this test.♻️ Proposed change
clock = _Clock() calls: list[tuple[str, float]] = [] page_path = tmp_path / "probe.html" + exits: list[bool] = [] class StallingSession: def __init__(self, _executable: str, *, launch_timeout_s: float, **_kwargs) -> None: calls.append(("launch", launch_timeout_s)) clock.advance(100.0) def __enter__(self): return self def __exit__(self, _exc_type, _exc, _tb) -> None: # noqa: ANN001 - pass + exits.append(True)assert calls == [ ("launch", pytest.approx(300.0)), ("page_session", pytest.approx(200.0)), ("Page.navigate", pytest.approx(100.0)), ] + assert exits == [True]Also applies to: 150-157
🤖 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 `@tests/test_render_smoke_nonumpy.py` around lines 118 - 122, Update StallingSession.__exit__ to record that the context manager exited, then assert this recorded state in the timeout-path test after run_probe completes. Ensure the assertion covers both referenced timeout test locations and would fail if run_probe no longer uses the session context manager.
🤖 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 `@scripts/render_smoke_nonumpy.py`:
- Around line 998-1001: Update the re-bin note near the deterministic probe to
state only the probes that explicitly set `_sampleRebinDisabled`, or apply that
flag consistently to every density-bearing probe, including dv1, dv2, v4, and
v5. Ensure the statement remains valid for v4’s zoomed rendering and does not
claim a broader invariant than the implementation guarantees.
In `@tests/test_render_smoke_nonumpy.py`:
- Around line 10-23: Update _load_render_smoke() to snapshot the relevant
sys.path and sys.modules entries before loading the module, then restore the
original state in a finally block around spec.loader.exec_module(). Remove any
added scripts/python paths and restore or delete render_smoke_nonumpy and
_protocol entries so test imports cannot leak across cases.
---
Nitpick comments:
In `@tests/test_render_smoke_nonumpy.py`:
- Line 84: Extend the run_probe test coverage with a third test using a fake
ChromiumSession whose _call method returns an exceptionDetails response. Keep
the existing session, timing, and page setup pattern, then assert that run_probe
raises RuntimeError matching “probe page exception” so the page-failure branch
is exercised.
- Around line 118-122: Update StallingSession.__exit__ to record that the
context manager exited, then assert this recorded state in the timeout-path test
after run_probe completes. Ensure the assertion covers both referenced timeout
test locations and would fail if run_probe no longer uses the session context
manager.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da3bb2fa-e5b2-41d9-a0e2-745e397cd98d
📒 Files selected for processing (5)
python/xy/_chromium.pyscripts/render_smoke_nonumpy.pyspec/process/contributing.mdtests/test_chromium_session.pytests/test_render_smoke_nonumpy.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/xy/_chromium.py (1)
254-265: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two identical handlers in
_stop_process.
subprocess.TimeoutExpiredis a subclass ofException. Theexcept subprocess.TimeoutExpiredblock and theexcept Exceptionblock run the same two statements. One handler expresses the same behavior.♻️ Proposed simplification
try: proc.wait(timeout=wait_timeout()) - except subprocess.TimeoutExpired: - with contextlib.suppress(Exception): - proc.kill() - with contextlib.suppress(Exception): - proc.wait(timeout=reap_timeout()) except Exception: with contextlib.suppress(Exception): proc.kill() with contextlib.suppress(Exception): proc.wait(timeout=reap_timeout())🤖 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 `@python/xy/_chromium.py` around lines 254 - 265, In _stop_process, collapse the subprocess.TimeoutExpired and generic Exception handlers into a single exception handler that preserves the existing proc.kill() suppression and reap wait behavior.tests/test_chromium_session.py (1)
258-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared helper for raw
ChromiumSessionconstruction.Six tests build a bare
ChromiumSessionwithobject.__new__and then set_ws,_next_id,_events, and sometimes_procor_tmpby hand. Extract a small factory, for example_bare_session(**attrs), to remove this repetition and keep the private-attribute list in one place.♻️ Example helper
def _bare_session(**attrs) -> _chromium.ChromiumSession: session = object.__new__(_chromium.ChromiumSession) session._next_id = 0 session._events = {} for name, value in attrs.items(): setattr(session, name, value) return sessionAlso applies to: 280-281, 337-340, 365-368, 394-398, 438-439
🤖 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 `@tests/test_chromium_session.py` around lines 258 - 261, Introduce a shared _bare_session helper in tests/test_chromium_session.py that constructs a raw ChromiumSession and initializes the common _next_id and _events state, while accepting optional attributes such as _ws, _proc, and _tmp. Replace the six duplicated object.__new__ construction blocks with this helper, preserving each test’s specific attributes and behavior.
🤖 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 `@python/xy/_chromium.py`:
- Around line 318-325: Both receive paths must fail closed for every exception:
widen the handler in _call at python/xy/_chromium.py lines 318-325 from OSError
to BaseException while retaining _invalidate_websocket() and re-raising; apply
the same change in _wait_event at python/xy/_chromium.py lines 350-354.
---
Nitpick comments:
In `@python/xy/_chromium.py`:
- Around line 254-265: In _stop_process, collapse the subprocess.TimeoutExpired
and generic Exception handlers into a single exception handler that preserves
the existing proc.kill() suppression and reap wait behavior.
In `@tests/test_chromium_session.py`:
- Around line 258-261: Introduce a shared _bare_session helper in
tests/test_chromium_session.py that constructs a raw ChromiumSession and
initializes the common _next_id and _events state, while accepting optional
attributes such as _ws, _proc, and _tmp. Replace the six duplicated
object.__new__ construction blocks with this helper, preserving each test’s
specific attributes and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9057d674-d893-44f2-a42a-7e243933c2a6
📒 Files selected for processing (5)
python/xy/_chromium.pyscripts/render_smoke_nonumpy.pyspec/process/contributing.mdtests/test_chromium_session.pytests/test_render_smoke_nonumpy.py
🚧 Files skipped from review as they are similar to previous changes (3)
- spec/process/contributing.md
- tests/test_render_smoke_nonumpy.py
- scripts/render_smoke_nonumpy.py
1 participant
What changed
--dump-dom --virtual-time-budgetsubprocess path with the repository's CDP-basedChromiumSessionharness.Why
On macOS arm64, the raw headless Chrome process could stay alive until the five-minute timeout even after the page had completed. The CDP harness already provides bounded lifecycle control elsewhere in the repository.
Impact
The no-NumPy standalone smoke completes reliably while preserving rendering, interaction, context-loss, DPR, and cleanup coverage.
Validation
.venv/bin/python scripts/render_smoke_nonumpy.py '/Applications/Google Chrome.app/Contents/MacOS/Google Chrome'— passed all assertions, including 3 context-loss cycles.venv/bin/python scripts/check_python_floor.py.venv/bin/ruff check scripts/render_smoke_nonumpy.pyFixes #394
Summary by CodeRabbit
Bug Fixes
Documentation
Tests