Skip to content

[py] Fix remaining mypy errors and enable CI for type checking#16828

Closed
cgoldberg wants to merge 14 commits intoSeleniumHQ:trunkfrom
cgoldberg:py-fix-remaining-type-hints
Closed

[py] Fix remaining mypy errors and enable CI for type checking#16828
cgoldberg wants to merge 14 commits intoSeleniumHQ:trunkfrom
cgoldberg:py-fix-remaining-type-hints

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Jan 2, 2026

User description

🔗 Related Issues

Fixes #15697
Supersedes #16827

💥 What does this PR do?

This PR fixes the rest of the type annotation errors across the Python bindings. You can now run mypy (tox -e typecheck) and there are zero errors with the current rule set.

This PR also:

  • moves typing stubs for third party packages into actual package dependencies so they are available to users
  • updates the typing job in ci-py to fail if any type errors are encountered in PR's or in trunk

🔧 Implementation Notes

There are still some additional mypy rules we can enable in pyproject.toml to make type checking stricter and mandatory.

This PR also adds some instance variables on some Service classes to make them similar to the others, and cleans up docstrings and removes a deprecation messages that shouldn't be there.

🔄 Types of changes

  • Cleanup
  • Type Checking
  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Fix type annotation errors across Python bindings for mypy compliance

  • Add null checks for devtools module in CDP and BiDi implementations

  • Enforce type checking in CI pipeline with strict mypy configuration

  • Standardize parameter types and docstrings across webdriver classes


Diagram Walkthrough

flowchart LR
  A["Type Annotations<br/>Across Webdrivers"] -->|"Fix parameter types"| B["Remote Connection<br/>Classes"]
  A -->|"Store instance vars"| C["WebDriver Classes"]
  A -->|"Add null checks"| D["CDP/BiDi Modules"]
  B -->|"Remove Optional"| E["ignore_proxy param"]
  C -->|"Store service/options"| F["LocalWebDriver"]
  D -->|"Validate devtools"| G["Error Handling"]
  I["Mypy Config<br/>Stricter Rules"] -->|"Enable in CI"| J["Type Check Pipeline"]
  J -->|"Fail on errors"| K["CI Enforcement"]
Loading

File Walkthrough

Relevant files
Type annotation fix
14 files
remote_connection.py
Remove optional type from ignore_proxy parameter                 
+1/-1     
remote_connection.py
Remove optional type from ignore_proxy parameter                 
+1/-1     
webdriver.py
Fix type annotations and store options/service as instance variables
+9/-10   
remote_connection.py
Remove optional type from ignore_proxy parameter                 
+1/-1     
webdriver.py
Store options and service as instance variables                   
+7/-10   
remote_connection.py
Cast browser_name to str and fix ignore_proxy type             
+2/-2     
webdriver.py
Store options and service as instance variables                   
+9/-10   
webdriver.py
Store options and service as instance variables                   
+6/-7     
remote_connection.py
Fix type annotations and add validation for initialization
+19/-14 
remote_connection.py
Remove optional type from ignore_proxy parameter                 
+1/-1     
webdriver.py
Reorder parameters and store options as instance variable
+8/-10   
event_firing_webdriver.py
Add type ignore comment for dynamic method assignment       
+2/-1     
webdriver.py
Add type annotations and exception handling                           
+10/-7   
webdriver.py
Add type annotations and exception handling                           
+10/-7   
Documentation
1 files
webdriver.py
Update docstring and remove blank line                                     
+1/-2     
Error handling
2 files
cdp.py
Add null checks for devtools module initialization             
+6/-0     
webdriver.py
Add null checks for CDP and BiDi initialization                   
+12/-1   
Configuration changes
3 files
ci-python.yml
Enable type checking to fail on errors                                     
+1/-1     
pyproject.toml
Enforce stricter mypy configuration rules                               
+3/-6     
tox.ini
Specify selenium module for mypy type checking                     
+1/-1     

@cgoldberg cgoldberg requested a review from iampopovich January 2, 2026 02:07
@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Jan 2, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 2, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix potential resource leak by correctly handling service object
Suggestion Impact:The commit removed the positional-args scanning logic (and the Service import) that the suggestion called out as problematic, and adjusted shutdown to always call self.service.stop(). However, it did not implement the suggested fix of initializing self.service from kwargs via kwargs.pop("service", None); instead it appears to rely on subclasses/superclass to provide self.service.

code diff:

-from selenium.webdriver.common.service import Service
 from selenium.webdriver.remote.webdriver import WebDriver as RemoteWebDriver
 
 
@@ -24,10 +23,6 @@
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
-        self.service = None
-        for arg in args:
-            if isinstance(arg, Service):
-                self.service = arg
         self._is_remote = False
 
     def __new__(cls, *args, **kwargs):
@@ -43,8 +38,8 @@
             # We don't care about the message because something probably has gone wrong
             pass
         finally:
-            if self.service:
-                self.service.stop()
+            # this is calling the subclass method at runtime.
+            self.service.stop()  # type: ignore
 

Fix a resource leak in LocalWebDriver by correctly initializing self.service
from keyword arguments instead of positional arguments.

py/selenium/webdriver/common/webdriver.py [25-31]

 def __init__(self, *args, **kwargs):
+    self.service = kwargs.pop("service", None)
     super().__init__(*args, **kwargs)
-    self.service = None
-    for arg in args:
-        if isinstance(arg, Service):
-            self.service = arg
     self._is_remote = False

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug introduced in the PR where the service object is not correctly retrieved, which would lead to resource leaks because service.stop() would not be called.

High
Ensure proxy setting is correctly applied
Suggestion Impact:The commit adds `ignore_proxy=ignore_proxy` to the ClientConfig initialization, ensuring the ignore_proxy setting is propagated.

code diff:

@@ -319,6 +319,7 @@
             self._client_config = ClientConfig(
                 remote_server_addr=remote_server_addr,
                 keep_alive=keep_alive,
+                ignore_proxy=ignore_proxy,
                 ignore_certificates=ignore_certificates,
                 init_args_for_pool_manager=init_args_for_pool_manager,

Fix a bug in RemoteConnection.init by passing the ignore_proxy argument to
the ClientConfig constructor.

py/selenium/webdriver/remote/remote_connection.py [307-326]

 def __init__(
     self,
     remote_server_addr: str | None = None,
     keep_alive: bool = True,
     ignore_proxy: bool = False,
     ignore_certificates: bool | None = False,
     init_args_for_pool_manager: dict | None = None,
     client_config: ClientConfig | None = None,
 ):
     if client_config:
         self._client_config = client_config
     elif remote_server_addr:
         self._client_config = ClientConfig(
             remote_server_addr=remote_server_addr,
             keep_alive=keep_alive,
+            ignore_proxy=ignore_proxy,
             ignore_certificates=ignore_certificates,
             init_args_for_pool_manager=init_args_for_pool_manager,
         )
     else:
         raise WebDriverException("Must provide either 'remote_server_addr' or 'client_config'")

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug in the refactored __init__ method where the ignore_proxy parameter is not passed to the ClientConfig constructor, causing the setting to be ignored.

Medium
General
Validate command executor type earlier

In start_devtools, move the isinstance check for self.command_executor to the
beginning of the function to fail faster on invalid types.

py/selenium/webdriver/remote/webdriver.py [1033-1063]

 def start_devtools(self) -> tuple[Any, WebSocketConnection]:
     global cdp
     import_cdp()
+
+    if not isinstance(self.command_executor, RemoteConnection):
+        raise WebDriverException("command_executor must be a RemoteConnection instance for CDP support")
+
     if self.caps.get("se:cdp"):
         ws_url = self.caps.get("se:cdp")
         cdp_version = self.caps.get("se:cdpVersion")
         if cdp_version is None:
             raise WebDriverException("CDP version not found in capabilities")
         version = cdp_version.split(".")[0]
     else:
         version, ws_url = self._get_cdp_details()
 
     if not ws_url:
         raise WebDriverException("Unable to find url to connect to from capabilities")
 
     if cdp is None:
         raise WebDriverException("CDP module not loaded")
 
     self._devtools = cdp.import_devtools(version)
     if self._websocket_connection:
         return self._devtools, self._websocket_connection
     if self.caps["browserName"].lower() == "firefox":
         raise RuntimeError("CDP support for Firefox has been removed. Please switch to WebDriver BiDi.")
-    if not isinstance(self.command_executor, RemoteConnection):
-        raise WebDriverException("command_executor must be a RemoteConnection instance for CDP support")
     self._websocket_connection = WebSocketConnection(
         ws_url,
         self.command_executor.client_config.websocket_timeout,
         self.command_executor.client_config.websocket_interval,
     )
     return self._devtools, self._websocket_connection

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion proposes a "fail-fast" improvement by moving a type check earlier in the function, which enhances robustness and debuggability, but does not fix a bug.

Low
Learned
best practice
Avoid mutating caller-provided options

If options is provided by the caller, defensively copy it before mutating fields
like binary_location and browser_version to avoid surprising external side
effects.

py/selenium/webdriver/chromium/webdriver.py [26-52]

-class ChromiumDriver(LocalWebDriver):
-    """Control the WebDriver instance of ChromiumDriver and drive the browser."""
+import copy
+...
+self.service = service if service else ChromiumService()
+self.options = copy.deepcopy(options) if options is not None else ChromiumOptions()
 
-    def __init__(
-        self,
-        browser_name: str,
-        vendor_prefix: str,
-        options: ChromiumOptions | None = None,
-        service: ChromiumService | None = None,
-        keep_alive: bool = True,
-    ) -> None:
-        ...
-        self.service = service if service else ChromiumService()
-        self.options = options if options else ChromiumOptions()
+finder = DriverFinder(self.service, self.options)
+if finder.get_browser_path():
+    self.options.binary_location = finder.get_browser_path()
+    self.options.browser_version = None
 
-        finder = DriverFinder(self.service, self.options)
-        if finder.get_browser_path():
-            self.options.binary_location = finder.get_browser_path()
-            self.options.browser_version = None
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - When storing collection-like or mutable state provided by callers, avoid mutating caller-owned objects; store a defensive copy to prevent side effects.

Low
  • Update

@SeleniumHQ SeleniumHQ deleted a comment from selenium-ci Jan 2, 2026
@SeleniumHQ SeleniumHQ deleted a comment from qodo-code-review bot Jan 2, 2026
@cgoldberg
Copy link
Member Author

cgoldberg commented Jan 2, 2026

CI keeps failing on Windows runners with ERROR: error loading package '@@protobuf+//java/core': Label '@@protobuf+//upb/cmake:build_defs.bzl' is invalid because 'upb/cmake' is not a package ??

@iampopovich
Copy link
Contributor

Weird. The tests on Windows stubbornly refuse to build. In my CI, the tests build and pass on the current trunk branch. This is definitely not a problem with changes in Python code typing. I need someone to help me with the failing tests. I'm satisfied with the passing tests on Ubuntu and Mac runners.
@diemol @titusfortner any suggestions here?

@cgoldberg
Copy link
Member Author

I have no idea what's wrong with CI, so I squashed all these changes and am trying again from a different branch. I'm closing this is favor of #16837.

@cgoldberg cgoldberg closed this Jan 3, 2026
@cgoldberg cgoldberg deleted the py-fix-remaining-type-hints branch January 5, 2026 15:29
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 B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-py Python Bindings Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [py] Many type annotation errors need fixing

3 participants