Skip to content

[py] drive WebKitGTK/WPE drivers from conftest and deprecate the shipped modules#17649

Draft
titusfortner wants to merge 3 commits into
trunkfrom
webkit
Draft

[py] drive WebKitGTK/WPE drivers from conftest and deprecate the shipped modules#17649
titusfortner wants to merge 3 commits into
trunkfrom
webkit

Conversation

@titusfortner

Copy link
Copy Markdown
Member

While working with Python bazel test targets I saw we are generating a bunch of targets that aren't being run, so I investigated the usage of these classes.

The real consumer of WebKitGTK / WPE WebKit driver classes is the WebKit project, except they don't use it as part of a library.

WebKit imports the Selenium repo at a specific commit to run Selenium's Python tests referencing the driver(s) in their project.

We don't need to maintain public classes in our library for this purpose, we just need to provide the right test harness to them.

@carlosgcampos & @lauromoura can you verify that this will work for your needs, or if I am missing a use case you require?

Context on how these classes are used:

  • They are only used/needed in Python binding
  • They do not get tested with Bazel
  • They do not get tested on GitHub (Selenium Manager does not support MiniBrowser or the webkit drivers).

💥 What does this PR do?

  • Deprecates all public facing WebKit classes
  • Moves all WebKit references out of primary harness and put it all in one place.
  • Removes all webkit Bazel test targets and unit tests

🔧 Implementation Notes

  • Any changes WebKit needs can be made in one place at the bottom of conftest.py file

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: the conftest harness refactor and the deprecation warnings
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • Non-breaking for WebKit; it will just work on their next pin bump (they won't even see the deprecation notices)

When the deprecated classes are removed, delete:

  • py/selenium/webdriver/webkitgtk/ and py/selenium/webdriver/wpewebkit/
  • :webkitgtk / :wpewebkit py_library targets in py/BUILD.bazel (and their entries in the :selenium deps)
  • WebKit/WPE entries in selenium/webdriver/__init__.py (_LAZY_IMPORTS, _LAZY_SUBMODULES)
  • WebKit/WPE entries in selenium/webdriver/__init__.pyi (imports, __all__, submodules)
  • WEBKITGTK / WPEWEBKIT in selenium/webdriver/common/desired_capabilities.py
  • WebKit/WPE sections in docs/source/api.rst and docs/source/index.rst

Do not remove:

  • The conftest.py WebKit harness and the drivers tuple (WebKitGTK/WPE stay runnable drivers)
  • xfail_webkitgtk / xfail_wpewebkit markers in pyproject.toml and their test usages

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Jun 6, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Deprecate WebKitGTK/WPE driver modules and move logic to conftest

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Deprecate WebKitGTK/WPE public driver classes with warnings
• Move WebKit driver logic from deprecated modules to conftest.py
• Remove all WebKit Bazel test targets and unit tests
• Create isolated WebKit driver implementation in test harness
Diagram
flowchart LR
  A["WebKitGTK/WPE<br/>Public Modules"] -->|deprecated| B["Deprecation<br/>Warnings"]
  C["conftest.py"] -->|new| D["WebKitDriver<br/>Implementation"]
  D -->|uses| E["_WebKitService<br/>_WebKitLocalDriver"]
  F["Bazel Tests<br/>WebKit Targets"] -->|removed| G["Test Cleanup"]
  H["Unit Tests<br/>Options/Service"] -->|removed| I["Test Cleanup"]

Loading

Grey Divider

File Changes

1. py/conftest.py ✨ Enhancement +95/-12

Add WebKit driver implementation to test harness

py/conftest.py


2. py/selenium/webdriver/webkitgtk/options.py Deprecation +7/-0

Add deprecation warning to WebKitGTKOptions class

py/selenium/webdriver/webkitgtk/options.py


3. py/selenium/webdriver/webkitgtk/service.py Deprecation +7/-0

Add deprecation warning to WebKitGTKService class

py/selenium/webdriver/webkitgtk/service.py


View more (7)
4. py/selenium/webdriver/webkitgtk/webdriver.py Deprecation +8/-0

Add deprecation warning to WebKitGTK driver class

py/selenium/webdriver/webkitgtk/webdriver.py


5. py/selenium/webdriver/wpewebkit/options.py Deprecation +8/-0

Add deprecation warning to WPEWebKitOptions class

py/selenium/webdriver/wpewebkit/options.py


6. py/selenium/webdriver/wpewebkit/service.py Deprecation +7/-0

Add deprecation warning to WPEWebKitService class

py/selenium/webdriver/wpewebkit/service.py


7. py/selenium/webdriver/wpewebkit/webdriver.py Deprecation +8/-0

Add deprecation warning to WPEWebKit driver class

py/selenium/webdriver/wpewebkit/webdriver.py


8. py/test/unit/selenium/webdriver/webkitgtk/webkitgtk_options_tests.py 🧪 Tests +0/-72

Remove WebKitGTK options unit tests

py/test/unit/selenium/webdriver/webkitgtk/webkitgtk_options_tests.py


9. py/test/unit/selenium/webdriver/wpewebkit/wpewebkit_options_tests.py 🧪 Tests +0/-60

Remove WPEWebKit options unit tests

py/test/unit/selenium/webdriver/wpewebkit/wpewebkit_options_tests.py


10. py/BUILD.bazel ⚙️ Configuration changes +0/-184

Remove all WebKit Bazel test targets

py/BUILD.bazel


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

Grey Divider


Action required

1. Quit masks start failure ✓ Resolved 🐞 Bug ☼ Reliability
Description
_WebKitLocalDriver.__init__() calls self.quit() on any startup exception, but if
Service.start() fails before service.process exists, LocalWebDriver.quit() can raise
AttributeError from Service.stop(), masking the original failure. That AttributeError is not
retried by Driver._start_local_driver() (it only catches
WebDriverException/HTTPError/OSError), turning a recoverable start failure into a hard,
misleading error.
Code

py/conftest.py[R802-809]

+    def __init__(self, options=None, service=None):
+        self.service = service
+        try:
+            self.service.start()
+            super().__init__(command_executor=self.service.service_url, options=options)
+        except Exception:
+            self.quit()
+            raise
Evidence
The new _WebKitLocalDriver cleanup calls quit() on any exception, and quit() unconditionally
calls service.stop() when self.service is set. Service.start() can raise before a process
attribute is ever assigned, while Service.stop() reads self.process without guarding for
attribute existence, so cleanup can throw AttributeError and obscure the original startup
exception; additionally, the driver-start retry loop does not catch AttributeError.

py/conftest.py[801-809]
py/selenium/webdriver/common/webdriver.py[33-43]
py/selenium/webdriver/common/service.py[99-109]
py/selenium/webdriver/common/service.py[149-164]
py/conftest.py[463-471]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_WebKitLocalDriver.__init__()` calls `self.quit()` inside a broad `except Exception` block. If `service.start()` fails before `service.process` is created, `LocalWebDriver.quit()` will call `service.stop()`, which can raise `AttributeError` due to missing `process`, masking the real root-cause exception and potentially breaking the retry behavior higher up.

## Issue Context
This affects the new WebKit harness in `py/conftest.py` and is most visible when the driver process fails to spawn/connect (bad path, permissions, early failure inside `Service.start()`).

## Fix Focus Areas
- py/conftest.py[796-810]

## Suggested fix approach
1. Ensure the WebKit `Service` instance always has a `process` attribute initialized (e.g., implement `_WebKitService.__init__` that calls `super().__init__` then sets `self.process = None`).
2. Make the exception handler in `_WebKitLocalDriver.__init__` robust to cleanup failures so the *original* exception is re-raised (e.g., wrap `self.quit()` in `try/except` and suppress cleanup exceptions).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. WebKit service None crash 🐞 Bug ≡ Correctness
Description
_WebKitLocalDriver unconditionally calls service.start(), but Driver._start_local_driver only
provides a service when --driver-binary is set, so running with --driver=webkitgtk/wpewebkit and no
--driver-binary will crash with AttributeError. This regresses prior behavior where
selenium.webdriver.webkitgtk/wpewebkit created a default Service() (PATH-based) and used
DriverFinder to resolve the driver path.
Code

py/conftest.py[R801-809]

+class _WebKitLocalDriver(LocalWebDriver):
+    def __init__(self, options=None, service=None):
+        self.service = service
+        self.service.start()
+        try:
+            super().__init__(command_executor=self.service.service_url, options=options)
+        except Exception:
+            self.quit()
+            raise
Evidence
The new conftest implementation can call _WebKitLocalDriver(service=None), because the base retry
loop only injects service when driver_path is set; _WebKitLocalDriver then dereferences
None.start(). The pre-existing shipped WebKit drivers show the intended behavior: create a default
Service() when none is passed and resolve a driver path from PATH via DEFAULT_EXECUTABLE_PATH +
DriverFinder.

py/conftest.py[463-469]
py/conftest.py[849-859]
py/conftest.py[801-809]
py/selenium/webdriver/webkitgtk/webdriver.py[29-55]
py/selenium/webdriver/webkitgtk/service.py[25-67]
py/selenium/webdriver/wpewebkit/service.py[25-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_WebKitLocalDriver.__init__` calls `self.service.start()` even when `service` is `None`. In the current flow, `service` is only passed when `--driver-binary` is provided, so local WebKit runs without that flag crash.

### Issue Context
Historically, the shipped WebKit drivers (`selenium.webdriver.webkitgtk.WebDriver` / `wpewebkit.WebDriver`) created a default `Service()` (with an executable path resolved via `shutil.which(...)`) and used `DriverFinder(...).get_driver_path()` to set `service.path`, so they could run when the driver executable was on `PATH`.

### Fix Focus Areas
- py/conftest.py[796-859]

### What to change
- Ensure the WebKit code path always passes a non-None `service` into `_WebKitLocalDriver` for local runs.
 - Option A (recommended): override `WebKitDriver._start_local_driver` (or `_initialize_driver`) to always set `kwargs["service"] = self.service`.
- Update `WebKitDriver.service` to provide a default executable when `--driver-binary` is not set (e.g., `shutil.which("WebKitWebDriver")` / `shutil.which("WPEWebDriver")`) and raise a clear `WebDriverException` if it cannot be found.
- (Optional but closer to previous behavior) Use `DriverFinder(service, options).get_driver_path()` to populate `service.path` when a default service is created, matching the prior `webkitgtk/wpewebkit` driver initialization flow.
- Add a defensive check in `_WebKitLocalDriver.__init__` (fail fast with a clear exception) if `service` is still `None`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. _WebKitLocalDriver leaks service ✓ Resolved 📘 Rule violation ☼ Reliability
Description
_WebKitLocalDriver.__init__() starts the driver process via service.start() outside of an
exception-safe cleanup block, so a failed start can leave a stray driver process running. This can
cause resource leaks and flakiness in local/automation runs.
Code

py/conftest.py[R801-809]

+class _WebKitLocalDriver(LocalWebDriver):
+    def __init__(self, options=None, service=None):
+        self.service = service
+        self.service.start()
+        try:
+            super().__init__(command_executor=self.service.service_url, options=options)
+        except Exception:
+            self.quit()
+            raise
Evidence
The new _WebKitLocalDriver starts the service process before its exception handler, so failures in
service.start() bypass cleanup in this constructor. Service.start() can raise after starting a
process when it cannot connect, which makes the missing cleanup path a leak risk.

py/conftest.py[801-809]
py/selenium/webdriver/common/service.py[99-120]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_WebKitLocalDriver.__init__()` calls `self.service.start()` before entering the `try/except` that calls `quit()`. If `Service.start()` raises after spawning the process (e.g., cannot connect within its retry loop), the service process may be left running.

## Issue Context
`Service.start()` can raise `WebDriverException` after launching the subprocess and does not perform automatic teardown on failure. The WebKit harness should guarantee bounded cleanup on all exception paths.

## Fix Focus Areas
- py/conftest.py[801-810]
- py/selenium/webdriver/common/service.py[99-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. webkitgtk/wpewebkit tests removed 📘 Rule violation ☼ Reliability
Description
This PR removes the Bazel py_test_suite coverage for webkitgtk/wpewebkit and adds new WebKit
driver harness logic without replacement test coverage. This increases regression risk for the new
WebKit-specific options/service behavior.
Code

py/BUILD.bazel[L1222-1251]

-py_test_suite(
-    name = "test-webkitgtk-common",
-    size = "large",
-    srcs = glob(
-        [
-            "test/selenium/webdriver/common/**/*.py",
-            "test/selenium/webdriver/support/**/*.py",
-        ],
-        exclude = BIDI_TESTS + ACTIONS_TESTS + FEATURE_TESTS,
-    ),
-    args = [
-        "--instafail",
-        "--driver=webkitgtk",
-        "--browser-binary=MiniBrowser",
-        "--browser-args=--automation",
-    ],
-    tags = [
-        "exclusive-if-local",
-        "no-sandbox",
-        "skip-rbe",
-    ],
-    test_suffix = "webkitgtk",
-    deps = [
-        ":common",
-        ":init-tree",
-        ":support",
-        ":webkitgtk",
-        ":webserver",
-    ] + TEST_DEPS,
-)
Evidence
py/BUILD.bazel no longer defines any WebKit test suites (it transitions directly from the remote
test suite section to unrelated binaries), while py/conftest.py adds substantial new WebKit driver
harness code and explicitly notes these drivers are not run in CI, reducing practical coverage for
the change.

AGENTS.md: Prefer Tests First; Favor Small Unit Tests Over Browser Tests; Avoid Mocks
py/BUILD.bazel[1213-1227]
py/conftest.py[781-863]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR removes WebKit Bazel test suites and adds new WebKit harness logic in `py/conftest.py` without adding small/unit tests to cover the new behavior.

## Issue Context
Even if the full browser suites are not run in CI, the new logic (capability building, service wiring, local driver startup) can be covered with lightweight unit tests to prevent regressions.

## Fix Focus Areas
- py/conftest.py[781-863]
- py/BUILD.bazel[1213-1227]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread py/conftest.py
Comment thread py/conftest.py
@cgoldberg

Copy link
Copy Markdown
Member

The real consumer of WebKitGTK / WPE WebKit driver classes is the WebKit project

What about regular users that want to drive WebKitGTK or WPEWebKit? We won't support that anymore?

@titusfortner

Copy link
Copy Markdown
Member Author

What about regular users that want to drive WebKitGTK or WPEWebKit? We won't support that anymore?

If these users exist we should add support to Selenium Manager and classes in the other bindings. I believe the only reason we have this is for WebKit team. Agents didn't see any real-world usage of these classes in the wild.

@qodo-code-review

qodo-code-review Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit c94e7aa

Comment thread py/conftest.py
@titusfortner titusfortner marked this pull request as draft June 6, 2026 15:28
@titusfortner

Copy link
Copy Markdown
Member Author

I'm moving this to draft until we hear back from Igalia team

@carlosgcampos

Copy link
Copy Markdown
Contributor

The original idea was to add support for using selenium with any WebKitGTK and WPEWebKit based browsers, not just for WebKit tests, but for any user. We started with the python API because it's the most commonly used one in linux, with the idea of adding support to other APIs eventually, but it never happened. I understand the situation is not ideal, it's not tested so it can break easily and we don't even know if there are actual users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants