[py] Add new ruff lint rules, fix violations and type annotations by cgoldberg · Pull Request #16789 · SeleniumHQ/selenium

cgoldberg · 2025-12-24T00:40:01Z

User description

💥 What does this PR do?

This is a general cleanup for the Python bindings:

Adds new rules and expands existing rules for the Python linter (ruff) and fixes all violations:
- pydocstyle error (E): https://docs.astral.sh/ruff/rules/#error-e
- flake8-pytest-style (PT): https://docs.astral.sh/ruff/rules/#flake8-pytest-style-pt
- pyupgrade (UP): https://docs.astral.sh/ruff/rules/#pyupgrade-up
- ruff-specific (RUF): https://docs.astral.sh/ruff/rules/#ruff-specific-rules-ruf
A few specific rules are ignored in pyproject.toml because they seemed overly pedantic.
Adds some missing type annotations
Updates several type annotations to use newer 3.10+ syntax instead of Unions
Removes the ./selenium/types.py type definition file and moves explicit type annotations into their respective modules. This file only contained a few types that weren't shared across many files and it's not really necessary to maintain a central file for this.
Makes test assertions more clear
Converts old string formatting to f-strings

🔄 Types of changes

Cleanup (formatting, renaming)
Bugfixes

PR Type

Enhancement, Tests

Description

Remove centralized selenium/types.py and inline type definitions
Update Python 3.10+ syntax: replace Union with | operator
Add ruff lint rules (pydocstyle, pytest-style, pyupgrade, ruff-specific)
Fix pytest fixture scopes and improve test assertions
Modernize string formatting and simplify conditional logic

Diagram Walkthrough

flowchart LR
  A["selenium/types.py<br/>Centralized types"] -->|Remove| B["Inline type definitions<br/>in respective modules"]
  C["Union syntax<br/>Old style"] -->|Modernize| D["Pipe operator syntax<br/>Python 3.10+"]
  E["Ruff lint rules<br/>D, E, F, I, PT, UP, RUF"] -->|Fix violations| F["Compliant codebase"]
  G["Pytest fixtures<br/>with scope"] -->|Simplify| H["Fixtures without<br/>implied scope"]
  I["Old string formatting<br/>% and .format()"] -->|Update| J["f-string formatting"]

File Walkthrough

Relevant files

Enhancement

17 files

types.py `Remove centralized type definitions file`	+0/-27
service.py `Inline SubprocessStdAlias type definition`	+2/-3
service.py `Inline SubprocessStdAlias type definition`	+2/-2
action_chains.py `Remove Union import, inline AnyDevice type`	+7/-4
webextension.py `Update string formatting to f-string`	+1/-1
proxy.py `Simplify dictionary checks with .get() method`	+10/-10
service.py `Inline SubprocessStdAlias, modernize Union syntax`	+5/-6
utils.py `Inline AnyKey type definition`	+1/-2
service.py `Inline SubprocessStdAlias type definition`	+2/-2
service.py `Inline SubprocessStdAlias type definition`	+2/-2
service.py `Inline SubprocessStdAlias type definition`	+2/-2
file_detector.py `Inline AnyKey type definition`	+3/-4
color.py `Modernize Union syntax and string formatting`	+6/-7
expected_conditions.py `Modernize Union syntax to pipe operator`	+2/-2
wait.py `Inline WaitExcTypes, modernize Union syntax`	+4/-5
generate.py `Remove unused Union import, modernize f-strings`	+6/-7
webserver.py `Remove Python 2 compatibility imports, modernize error handling`	+10/-20

Cleanup

1 files

__init__.py `Remove noqa comments from imports`	+27/-27

Tests

20 files

conftest.py `Remove fixture scope, fix pytest assertions`	+9/-9
bidi_emulation_tests.py `Fix pytest parametrize tuple syntax`	+2/-2
click_scrolling_tests.py `Update string formatting to f-string`	+1/-1
cookie_tests.py `Split compound assertions into separate statements`	+4/-2
executing_javascript_tests.py `Update string formatting to f-string`	+1/-1
opacity_tests.py `Modernize string formatting`	+2/-3
position_and_size_tests.py `Convert tuple to list in parametrize`	+6/-7
takes_screenshots_tests.py `Split compound assertions into separate statements`	+6/-3
typing_tests.py `Add noqa comment for pyupgrade rule`	+2/-1
virtual_authenticator_tests.py `Fix variable naming and simplify assertions`	+3/-3
ff_takes_full_page_screenshots_tests.py `Split compound assertions into separate statements`	+4/-2
remote_connection_tests.py `Split compound assertions into separate statements`	+2/-1
remote_custom_element_tests.py `Remove empty parentheses from fixture decorator`	+1/-1
remote_custom_locator_tests.py `Remove empty parentheses from fixture decorator`	+1/-1
event_firing_webdriver_tests.py `Update string formatting to f-string`	+2/-2
firefox_options_tests.py `Split compound assertion into separate statements`	+1/-1
test_ie_options.py `Change fixture from yield to return`	+1/-1
error_handler_tests.py `Change fixture from yield to return`	+1/-1
remote_connection_tests.py `Remove fixture scope, change yield to return`	+5/-5
credentials_tests.py `Fix variable naming and simplify assertions`	+10/-23

Configuration changes

1 files

pyproject.toml `Expand ruff lint rules and add ignore exceptions`	+6/-1

qodo-code-review · 2025-12-28T18:58:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Assertion not raised: The exception handler constructs an `AssertionError` but does not raise it, allowing a failing click to incorrectly pass the test and silently hide the error. Referred Code try: link.click() except MoveTargetOutOfBoundsException as e: AssertionError(f"Should not be out of bounds: {e.msg}") Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Path disclosure error: The raised exception includes local filesystem paths and environment-derived values (`WEBDRIVER`, `HTML_ROOT`), which may disclose internal details depending on where/ how this test utility is executed and surfaced. Referred Code HTML_ROOT = os.path.join(WEBDRIVER, "../../../../common/src/web") if not os.path.isdir(HTML_ROOT): raise Exception( "Can't find 'common_web' directory, try setting WEBDRIVER environment variable.\n" f"WEBDRIVER: {WEBDRIVER}\nHTML_ROOT: {HTML_ROOT}" ) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-28T19:00:12Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Raise the AssertionError~~ Suggestion Impact: The commit fixed the same underlying issue (the test silently passing) by replacing the non-raised AssertionError expression with an explicit failure via pytest.fail(...), ensuring the test fails when the exception is caught. code diff: except MoveTargetOutOfBoundsException as e: - AssertionError(f"Should not be out of bounds: {e.msg}") + pytest.fail(f"Should not be out of bounds: {e.msg}") Add a `raise` statement before `AssertionError` to ensure the test fails correctly when an exception is caught. py/test/selenium/webdriver/common/click_scrolling_tests.py [52-53] except MoveTargetOutOfBoundsException as e: - AssertionError(f"Should not be out of bounds: {e.msg}") + raise AssertionError(f"Should not be out of bounds: {e.msg}") `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where a test would silently pass instead of failing because the `AssertionError` was not raised.	High
High-level	Re-evaluate removing the centralized types file The removal of `selenium/types.py` has caused duplication of complex type definitions. Consider restoring a centralized types file to improve maintainability. Examples: py/selenium/webdriver/chrome/service.py [40-48] def __init__( self, executable_path: str \| None = None, port: int = 0, service_args: Sequence[str] \| None = None, log_output: int \| str \| IO[Any] \| None = None, env: Mapping[str, str] \| None = None, kwargs, ) -> None: py/selenium/webdriver/edge/service.py [36-45] def __init__( self, executable_path: str \| None = None, port: int = 0, log_output: int \| str \| IO[Any] \| None = None, service_args: Sequence[str] \| None = None, env: Mapping[str, str] \| None = None, driver_path_env_key: str \| None = None, kwargs, ) -> None: Solution Walkthrough: Before: # In py/selenium/webdriver/chrome/service.py class Service(service.ChromiumService): def __init__( self, ..., log_output: int \| str \| IO[Any] \| None = None, ... ) -> None: ... # In py/selenium/webdriver/edge/service.py class Service(service.ChromiumService): def __init__( self, ..., log_output: int \| str \| IO[Any] \| None = None, ... ) -> None: ... # ... and 4 other service files with the same duplicated type After: # In a restored py/selenium/types.py from typing import IO, Any SubprocessStdAlias = int \| str \| IO[Any] # In py/selenium/webdriver/chrome/service.py from selenium.types import SubprocessStdAlias class Service(service.ChromiumService): def __init__( self, ..., log_output: SubprocessStdAlias \| None = None, ... ) -> None: ... # In py/selenium/webdriver/edge/service.py from selenium.types import SubprocessStdAlias class Service(service.ChromiumService): def __init__( self, ..., log_output: SubprocessStdAlias \| None = None, ... ) -> None: ... # ... and other service files use the alias Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that removing `selenium/types.py` introduced duplicated complex type hints across multiple files, which impacts maintainability. This is a valid design-level concern.	Low
Learned best practice	✅ ~~Validate caller-provided input shape~~ Suggestion Impact: The constructor was updated to return early when `raw` is None and to raise a TypeError if `raw` is not a dict, preventing attribute errors from calling `.get` on non-mapping inputs. A type annotation `raw: dict \| None` was also added. code diff: - def __init__(self, raw=None): + def __init__(self, raw: dict \| None = None): """Creates a new Proxy. Args: raw: Raw proxy data. If None, default class values are used. """ - if raw: - if raw.get("proxyType"): - self.proxy_type = ProxyType.load(raw["proxyType"]) - if raw.get("httpProxy"): - self.http_proxy = raw["httpProxy"] - if raw.get("noProxy"): - self.no_proxy = raw["noProxy"] - if raw.get("proxyAutoconfigUrl"): - self.proxy_autoconfig_url = raw["proxyAutoconfigUrl"] - if raw.get("sslProxy"): - self.sslProxy = raw["sslProxy"] - if raw.get("autodetect"): - self.auto_detect = raw["autodetect"] - if raw.get("socksProxy"): - self.socks_proxy = raw["socksProxy"] - if raw.get("socksUsername"): - self.socks_username = raw["socksUsername"] - if raw.get("socksPassword"): - self.socks_password = raw["socksPassword"] - if raw.get("socksVersion"): - self.socks_version = raw["socksVersion"] + if raw is None: + return + if not isinstance(raw, dict): + raise TypeError(f"`raw` must be a dict, got {type(raw)}") + if raw.get("proxyType"): + self.proxy_type = ProxyType.load(raw["proxyType"]) + if raw.get("httpProxy"): + self.http_proxy = raw["httpProxy"] + if raw.get("noProxy"): + self.no_proxy = raw["noProxy"] + if raw.get("proxyAutoconfigUrl"): + self.proxy_autoconfig_url = raw["proxyAutoconfigUrl"] + if raw.get("sslProxy"): + self.sslProxy = raw["sslProxy"] + if raw.get("autodetect"): + self.auto_detect = raw["autodetect"] + if raw.get("socksProxy"): + self.socks_proxy = raw["socksProxy"] + if raw.get("socksUsername"): + self.socks_username = raw["socksUsername"] + if raw.get("socksPassword"): + self.socks_password = raw["socksPassword"] + if raw.get("socksVersion"): + self.socks_version = raw["socksVersion"] `raw.get(...)` assumes `raw` is a mapping; add a type/structure guard (or normalize input) and raise a clear error for invalid types to avoid runtime attribute errors. py/selenium/webdriver/common/proxy.py [114-140] def __init__(self, raw=None): """Creates a new Proxy. Args: raw: Raw proxy data. If None, default class values are used. """ - if raw: - if raw.get("proxyType"): - self.proxy_type = ProxyType.load(raw["proxyType"]) - if raw.get("httpProxy"): - self.http_proxy = raw["httpProxy"] - if raw.get("noProxy"): - self.no_proxy = raw["noProxy"] - if raw.get("proxyAutoconfigUrl"): - self.proxy_autoconfig_url = raw["proxyAutoconfigUrl"] - if raw.get("sslProxy"): - self.sslProxy = raw["sslProxy"] - if raw.get("autodetect"): - self.auto_detect = raw["autodetect"] - if raw.get("socksProxy"): - self.socks_proxy = raw["socksProxy"] - if raw.get("socksUsername"): - self.socks_username = raw["socksUsername"] - if raw.get("socksPassword"): - self.socks_password = raw["socksPassword"] - if raw.get("socksVersion"): - self.socks_version = raw["socksVersion"] + if raw is None: + return + if not isinstance(raw, dict): + raise TypeError(f"`raw` must be a dict, got {type(raw)!r}") + if raw.get("proxyType"): + self.proxy_type = ProxyType.load(raw["proxyType"]) + if raw.get("httpProxy"): + self.http_proxy = raw["httpProxy"] + if raw.get("noProxy"): + self.no_proxy = raw["noProxy"] + if raw.get("proxyAutoconfigUrl"): + self.proxy_autoconfig_url = raw["proxyAutoconfigUrl"] + if raw.get("sslProxy"): + self.sslProxy = raw["sslProxy"] + if raw.get("autodetect"): + self.auto_detect = raw["autodetect"] + if raw.get("socksProxy"): + self.socks_proxy = raw["socksProxy"] + if raw.get("socksUsername"): + self.socks_username = raw["socksUsername"] + if raw.get("socksPassword"): + self.socks_password = raw["socksPassword"] + if raw.get("socksVersion"): + self.socks_version = raw["socksVersion"] + `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add explicit validation/guards at integration boundaries before using caller-provided inputs.	Low
General	✅ ~~Refactor string formatting for better readability~~ Suggestion Impact: The commit replaced the multiline `"abcd{}...".format(...) # noqa: UP032` construction with a `keys_to_send` list and `element.send_keys("".join(keys_to_send))`, matching the suggested readability refactor and removing the noqa. code diff: - element.send_keys( - "abcd{}{}{}{}{}{}{}{}{}{}{}{}abcd".format( # noqa: UP032 - Keys.MULTIPLY, - Keys.SUBTRACT, - Keys.ADD, - Keys.DECIMAL, - Keys.SEPARATOR, - Keys.NUMPAD0, - Keys.NUMPAD9, - Keys.ADD, - Keys.SEMICOLON, - Keys.EQUALS, - Keys.DIVIDE, - Keys.NUMPAD3, - ) - ) + keys_to_send = [ + "abcd", + Keys.MULTIPLY, + Keys.SUBTRACT, + Keys.ADD, + Keys.DECIMAL, + Keys.SEPARATOR, + Keys.NUMPAD0, + Keys.NUMPAD1, + Keys.NUMPAD2, + Keys.F1, + Keys.F2, + Keys.F3, + Keys.F4, + "abcd", + ] + element.send_keys("".join(keys_to_send)) Refactor the `str.format()` call into a `"".join()` with a list of keys to improve readability and remove the `noqa: UP032` comment. py/test/selenium/webdriver/common/typing_tests.py [301-316] -element.send_keys( - "abcd{}{}{}{}{}{}{}{}{}{}{}{}abcd".format( # noqa: UP032 - Keys.MULTIPLY, - Keys.SUBTRACT, - Keys.ADD, - Keys.DECIMAL, - Keys.SEPARATOR, - Keys.NUMPAD0, - Keys.NUMPAD1, - Keys.NUMPAD2, - Keys.F1, - Keys.F2, - Keys.F3, - Keys.F4, - ) -) +keys_to_send = [ + "abcd", + Keys.MULTIPLY, + Keys.SUBTRACT, + Keys.ADD, + Keys.DECIMAL, + Keys.SEPARATOR, + Keys.NUMPAD0, + Keys.NUMPAD1, + Keys.NUMPAD2, + Keys.F1, + Keys.F2, + Keys.F3, + Keys.F4, + "abcd", +] +element.send_keys("".join(keys_to_send)) `[Suggestion processed]` Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies an opportunity to refactor a complex `str.format()` call, improving readability and eliminating the need for a `noqa` comment.	Low
Update

navin772

LGTM!

[py] Add new ruff lint rules and fix violations

dff0c88

selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Dec 24, 2025

cgoldberg added 7 commits December 26, 2025 09:00

Merge branch 'trunk' into py-add-lint-rules

30f720e

Merge branch 'trunk' into py-add-lint-rules

1044ef4

[py] Improve test assertions

4fc5588

[py] Remove implied fixture scope

e53e241

[py] Fix paramaterize fixtures

b7513b1

[py] Fix more types and linting

dadb38f

Merge branch 'trunk' into py-add-lint-rules

461fb6c

cgoldberg changed the title ~~WIP - [py] Add new ruff lint rules and fix violations~~ [py] Add new ruff lint rules, fix violations and type annotations Dec 28, 2025

SeleniumHQ deleted a comment from selenium-ci Dec 28, 2025

[py] Fix line continuation

3f8da78

cgoldberg marked this pull request as ready for review December 28, 2025 18:57

qodo-code-review Bot added the Review effort 3/5 label Dec 28, 2025

cgoldberg added 3 commits December 28, 2025 17:11

[py] fail properly on test exception

f329277

[py] Add type hint

0ec6ba9

[py] Refactor f-string

0f8b376

cgoldberg requested review from Delta456 and navin772 and removed request for Delta456 December 28, 2025 22:33

Merge branch 'trunk' into py-add-lint-rules

17f6293

navin772 requested changes Dec 29, 2025

View reviewed changes

Comment thread py/test/selenium/webdriver/common/typing_tests.py Outdated

[py] Revert f-string change

9f47914

navin772 approved these changes Dec 29, 2025

View reviewed changes

cgoldberg merged commit 88273fd into SeleniumHQ:trunk Dec 29, 2025
23 checks passed

cgoldberg deleted the py-add-lint-rules branch December 29, 2025 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[py] Add new ruff lint rules, fix violations and type annotations#16789

[py] Add new ruff lint rules, fix violations and type annotations#16789
cgoldberg merged 14 commits into
SeleniumHQ:trunkfrom
cgoldberg:py-add-lint-rules

cgoldberg commented Dec 24, 2025 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Dec 28, 2025 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Dec 28, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

navin772 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

cgoldberg commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

💥 What does this PR do?

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review Bot commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review Bot commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

navin772 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cgoldberg commented Dec 24, 2025 •

edited

Loading

qodo-code-review Bot commented Dec 28, 2025 •

edited

Loading

qodo-code-review Bot commented Dec 28, 2025 •

edited

Loading