Skip to content

Add is_cdp flag and CDP robustness improvements#97

Open
wwakabobik wants to merge 8 commits into
CustomEnv:masterfrom
wwakabobik:feature/connect-cdp
Open

Add is_cdp flag and CDP robustness improvements#97
wwakabobik wants to merge 8 commits into
CustomEnv:masterfrom
wwakabobik:feature/connect-cdp

Conversation

@wwakabobik

@wwakabobik wwakabobik commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Problem

When users connect to a remote browser via CDP (e.g., Electron app with --remote-debugging-port=9222) and wrap it in a DriverWrapper, several runtime issues occur:

  • PlayDriver.quit() calls tracing.stop() which fails because tracing was never started on pre-existing CDP contexts
  • PlayDriver.quit() calls page.close() / context.close() which can throw PlaywrightError for externally-managed browsers
  • CoreDriver.quit() calls driver.quit() which can throw WebDriverException for externally-managed browsers
  • PlayDriver.get_inner_window_size() crashes when viewport_size is None (default for CDP pages without explicit viewport)

These are runtime bugs that every project connecting via CDP would hit.

Solution

A new is_cdp boolean flag on DriverWrapper (default False) that users set after creating their CDP connection:

from playwright.sync_api import sync_playwright
from mops.base.driver_wrapper import DriverWrapper
from mops.mixins.objects.driver import Driver

pw = sync_playwright().start()
browser = pw.chromium.connect_over_cdp("http://localhost:9222")
context = browser.contexts[0]
page = context.pages[0]

wrapper = DriverWrapper(Driver(driver=page, context=context, instance=browser))
wrapper.is_cdp = True  # enables CDP robustness

# ... use wrapper normally ...
wrapper.quit()
pw.stop()

Driver creation stays on the user's project side (per wrapper-pattern convention). MOPS only provides the robustness layer via is_cdp.

Changes

Driver Change
PlayDriver.quit() tracing.stop() skipped when is_cdp=True; page.close() and context.close() wrapped in contextlib.suppress(PlaywrightError) for CDP only
CoreDriver.quit() driver.quit() wrapped in try/except SeleniumWebDriverException for CDP only
PlayDriver.get_inner_window_size() Returns Size(0, 0) when viewport_size is None

Non-CDP behavior is completely unchanged — error suppression only applies when is_cdp=True.

Files Changed

File Change
mops/base/driver_wrapper.py is_cdp: bool = False flag
mops/abstraction/driver_wrapper_abc.py is_cdp attribute in ABC
mops/playwright/play_driver.py quit(): tracing skip + suppress errors for CDP; get_inner_window_size() null-safe
mops/selenium/core/core_driver.py quit(): try/except guarded by is_cdp
CHANGELOG.md v3.5.1 entry
tests/static_tests/unit/test_cdp_robustness.py 11 unit tests

Backward Compatibility

Purely additive — no existing attributes, methods, or signatures change. is_cdp defaults to False. Changes in PlayDriver.quit() and CoreDriver.quit() only activate when is_cdp=True.

Test Plan

  • 4 is_cdp flag tests: default False (Playwright/Selenium), set True (Playwright/Selenium)
  • 3 PlayDriver CDP quit tests: suppress page close error, suppress context close error, skip tracing
  • 1 PlayDriver non-CDP quit test: tracing still called
  • 1 PlayDriver non-CDP quit test: close error propagates (not suppressed)
  • 1 CoreDriver CDP quit test: suppress WebDriverException
  • 1 CoreDriver non-CDP quit test: WebDriverException propagates
  • 2 viewport null-safe tests: NoneSize(0, 0), set → correct values
  • All 380 static tests pass

@wwakabobik wwakabobik force-pushed the feature/connect-cdp branch 4 times, most recently from d57fc68 to 24661a6 Compare March 23, 2026 18:14
Problem:
MOPS requires users to manually create driver objects when connecting to
remote browsers via Chrome DevTools Protocol (CDP). This involves
boilerplate code for lifecycle management and cleanup.

Use cases:
- Testing Electron apps on remote machines via CDP + SSH tunnel
- Connecting to cloud browser services (BrowserStack, Sauce Labs)
- Attaching to an already-running browser instance for debugging
- Testing CEF-based or WebView2 applications

Solution:
Add a factory classmethod DriverWrapper.connect_cdp(endpoint_url) that
handles the full lifecycle. Supports both Playwright and Selenium engines
via the engine parameter (default: "playwright").

Playwright engine:
  Starts Playwright, connects via chromium.connect_over_cdp, wraps the
  resulting page. Playwright instance is stopped on quit().

Selenium engine:
  Creates Chrome WebDriver with debugger_address option pointing to the
  CDP endpoint. URL is parsed via urllib.parse for robustness.

Architectural changes:
- DriverWrapper.is_cdp flag: identifies CDP-connected wrappers
- PlayDriver.quit(): tracing.stop() skipped for CDP contexts
- CoreDriver.quit(): try/except guarded by is_cdp only (non-CDP
  Selenium quit behavior preserved)
- PlayDriver.get_inner_window_size(): null-safe for CDP default viewport

Version bump: 3.3.1 -> 3.4.0

Documentation:
- Getting Started: CDP section with both engine examples + limitations
- Index: CDP listed in key features
- DriverWrapper overview: CDP mentioned in description and status attrs
- CHANGELOG: v3.4.0 entry

Tests: 13 unit tests covering both engines, is_cdp flag, URL parsing,
parameters, cleanup, sessions, and compatibility

Made-with: Cursor
@wwakabobik wwakabobik force-pushed the feature/connect-cdp branch from 24661a6 to 54c88fd Compare March 23, 2026 18:16
Resolve conflicts in mops/__init__.py and CHANGELOG.md:
- Version set to 3.4.3 (CDP feature on top of upstream 3.4.2)
- CHANGELOG: upstream 3.4.0-3.4.2 entries preserved, CDP entry renumbered to 3.4.3

Made-with: Cursor
@VladimirPodolian

Copy link
Copy Markdown
Member

@wwakabobik thanks for the contribution, but this PR cannot be accepted in its current state as it violates the wrapper pattern rules. DriverWrapper should not be responsible for creating new driver connections. The driver setup should be handled on the user's project side instead, since there can be a wide variety of settings and capabilities involved — custom browser options, capability configs, environment-specific flags, and so on. The wrapper's sole responsibility is to wrap an already-initialized driver, not to manage its creation or lifecycle bootstrapping even with cdp connection. That said, the is_cdp flag and the related robustness changes in quit() and get_inner_window_size() look genuinely useful and are worth keeping

Maintainer feedback: DriverWrapper should not create driver connections,
only wrap already-initialized drivers. The connect_cdp factory violated
the wrapper pattern — driver setup with custom options/capabilities
belongs on the user's project side.

Removed:
- connect_cdp(), _connect_cdp_playwright(), _connect_cdp_selenium()
- sync_playwright import and _playwright_instance cleanup
- CDP connection docs from getting_started, index, driver_wrapper overview

Kept (accepted by maintainer):
- is_cdp flag on DriverWrapper and ABC
- PlayDriver.quit() tracing skip + error guards for CDP
- CoreDriver.quit() error guard for CDP
- PlayDriver.get_inner_window_size() null-safe viewport

Tests: replaced test_connect_cdp.py (14 factory tests) with
test_cdp_robustness.py (11 tests covering is_cdp, quit behavior,
viewport null-safety). All 390 static + 15 ABC tests pass.

Made-with: Cursor
@wwakabobik wwakabobik changed the title Add DriverWrapper.connect_cdp for CDP browser connections Add is_cdp flag and CDP robustness improvements Apr 3, 2026
@wwakabobik

Copy link
Copy Markdown
Contributor Author

Updated ro conform changes proposed/approved by maintainer.

wwakabobik and others added 2 commits June 8, 2026 14:34
Resolved conflicts in CHANGELOG.md, mops/__init__.py,
mops/abstraction/driver_wrapper_abc.py, mops/base/driver_wrapper.py,
and mops/playwright/play_driver.py.

- Preserved is_cdp flag and CDP robustness changes from this PR
- Adopted upstream v3.5.0 changes (storage API, ruff linter, Python 3.10+)
- Combined is_cdp tracing skip with upstream's contextlib.suppress pattern

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants