You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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"]
✅ Fix potential resource leak by correctly handling service objectSuggestion 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.
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 appliedSuggestion Impact:The commit adds `ignore_proxy=ignore_proxy` to the ClientConfig initialization, ensuring the ignore_proxy setting is propagated.
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.
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.
-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.
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 ??
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?
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
B-buildIncludes scripting, bazel and CI integrationsB-devtoolsIncludes everything BiDi or Chrome DevTools relatedB-supportIssue or PR related to support classesC-pyPython BindingsReview effort 3/5
3 participants
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.
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:
typingjob inci-pyto fail if any type errors are encountered in PR's or in trunk🔧 Implementation Notes
There are still some additional
mypyrules we can enable inpyproject.tomlto 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
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
File Walkthrough
14 files
Remove optional type from ignore_proxy parameterRemove optional type from ignore_proxy parameterFix type annotations and store options/service as instance variablesRemove optional type from ignore_proxy parameterStore options and service as instance variablesCast browser_name to str and fix ignore_proxy typeStore options and service as instance variablesStore options and service as instance variablesFix type annotations and add validation for initializationRemove optional type from ignore_proxy parameterReorder parameters and store options as instance variableAdd type ignore comment for dynamic method assignmentAdd type annotations and exception handlingAdd type annotations and exception handling1 files
Update docstring and remove blank line2 files
Add null checks for devtools module initializationAdd null checks for CDP and BiDi initialization3 files
Enable type checking to fail on errorsEnforce stricter mypy configuration rulesSpecify selenium module for mypy type checking