Skip to content

[py] adopt the worthwhile ruff 0.16 rule families (tranches 1 and 2) - #17992

Open
AutomatedTester wants to merge 6 commits into
trunkfrom
py-ruff-b017-narrow
Open

[py] adopt the worthwhile ruff 0.16 rule families (tranches 1 and 2)#17992
AutomatedTester wants to merge 6 commits into
trunkfrom
py-ruff-b017-narrow

Conversation

@AutomatedTester

@AutomatedTester AutomatedTester commented Sep 7, 2026

Copy link
Copy Markdown
Member

Part of #17989.

Implements tranche 1 ("adopt now") and tranche 2 ("adopt with scoping") from #17989. Six commits, one per decision, each leaving //py:ruff-check and //py:ruff-format --check clean and //py:unit at 32/32, so any of them can be dropped independently.

commit adds to select findings
[py] enforce the mechanical ruff rule families from ruff 0.16 C4 FLY W + FURB122/167/188/192 PERF102 20 fixed
[py] settle shebang/exec-bit consistency EXE 6
[py] replace the assert 1 == 0 idiom with pytest.raises PLR0133 4
[py] enforce ruff PIE and PLE PIE PLE 5 fixed, 4 suppressed
[py] enforce ruff bugbear (B) with carve-outs B + 4 extend-ignore 7 fixed, 18 carved out
[py] narrow pytest.raises(Exception) and enforce B017 (drops the B017 ignore) 7 narrowed

Three things that differ from the issue

1. select takes whole families, not ruff's default subset. This is the important one. The issue's "adopting a family means adding it to select" would enforce far more than the numbers in its table:

family issue / default subset whole family
PLR 4 332 (251 are PLR2004)
B 33 59
FURB 10 29
PERF 2 19
PYI 9 17

Tranches 1 + 2 as bare family names is 479 findings, not ~81. So PLR, PERF and FURB go in as explicit rule codes here, and B goes in as a family plus a matching extend-ignore block — family form on purpose, so new bugbear rules in a future ruff release surface as one review decision instead of silently.

2. Neither "genuine bug" cited for tranche 1 is a bug. PLE0605 is __all__ = sorted(_LAZY_IMPORTS.keys()) — sorted() returns a list, and __init__.pyi already re-exports every name explicitly, so nothing static reads that line. PIE796's duplicate enum values in generate_bidi.py are deliberate aliases, and the bug they used to cause was fixed in 9d0ae9e68f. Both are now noqa + a reason rather than "fixed".

3. The B018 carve-out needs one more entry. "py/test/**" = ["B018"] takes 19 findings to 1; the survivor is the driver-liveness probe in py/conftest.py, which is not under py/test/**. Both globs are also resolved against the working directory rather than pyproject.toml, so they only work because py/private/ruff.py chdirs to the workspace root — there is a comment saying so, since they fail silently if ruff is run from inside py/.

Also worth knowing: //py:ruff-check lints py scripts common dotnet java javascript rb, so this touches scripts/ and javascript/private/gen_file.py too. No runtime cross-binding impact.

Verification

  • //py:unit 32/32 on every commit.
  • visibility_tests-chrome 15/15, with all four converted tests confirmed as run rather than skipped.
  • The PIE/PLE commit touches the BiDi generator, so the BiDi sources were regenerated and diffed against trunk: byte-identical across all 22 files, apart from the one _script_handlers.py line changed deliberately in the first commit.
  • B017 exception types were each confirmed by running the Bazel target with a deliberately wrong assertion and reading the real type back, not guessed: ValueError (proxy), TypeError (script argument rejected client-side by json.dumps), NoSuchFrameException (closed context), NoSuchHandleException (disowned handle). quit_tests needed a tuple — a local driver gives MaxRetryError because the process is gone, a Grid gives InvalidSessionIdException because the session is — verified against both quit_tests-chrome and quit_tests-chrome-remote.
  • The comprehension rewrites in webdriver.py (_wrap_value/_unwrap_value) and the dict.fromkeys change feeding a BiDi preload script were checked for equivalence against the pre-change code over a generated input corpus before being applied.

Deliberately not included

  • B904 (17, mostly py/selenium) — raise ... from err inside except. For a library this is arguably the highest-value rule in the whole 184-finding set, and the issue buries it inside the B row. In extend-ignore here; wants its own PR.
  • B028 (4), B007 (3), B024 (2).
  • PYI021 (8) should join the issue's decline list: every hit is the "This stub file is necessary for type checkers…" docstring in the __init__.pyi files, which the rule wants deleted.
  • Tranche 3 (SIM / TRY / G) untouched. Of note, SIM115 — the one the issue calls substantive — is mostly a false positive: service.py:63 and gen_file.py hold intentionally process-lifetime handles, and service.py has an _owns_log_output flag that exists precisely because it cannot be a with block. G201 (5) looks worth adopting on its own.

Follow-ups found along the way

  • Nothing asserts that __init__.pyi's re-export list and _LAZY_IMPORTS agree.
  • urllib3.exceptions.MaxRetryError escapes the public API after driver.quit(); arguably should be wrapped in a WebDriverException.
  • _bidi/browsing_context_tests.py::test_activate_browsing_context is flaky — fails roughly one run in two on trunk as well as on this branch (asynchronous focus handover). Unrelated to this PR; happy to file it separately.

Note on measurement

The issue's table says 192 findings; the tree is at 184 now (W 1→0, B 35→33, SIM 28→26, PLW 16→13), so the numbers above were all re-measured against 8cbf4bfa2d with the pinned ruff 0.16.5 rather than taken from the issue.

Disclosure per CONTRIBUTING.md: substantial parts of this PR were written with AI assistance (Claude Code). All of it has been reviewed, and every claim above was verified by running the Bazel targets named.

Co-Authored-By: Copse noreply@copse.dev
Copse-Models: acp:claude-agent-acp#opus[1m]

@AutomatedTester
[py] enforce the mechanical ruff rule families from ruff 0.16 (#17989)
965d770 
Tranche 1 of #17989. Adds the rule families whose findings are pure local
rewrites, and fixes the 20 findings so //py:ruff-check stays green.

Adopted as whole families (verified clean at full-family scope): C4, FLY, W.
Adopted as individual rules, because the rest of their family is far noisier
than the subset ruff 0.16 enables by default: FURB122, FURB167, FURB188,
FURB192, PERF102. For scale, whole-family PLR is 332 findings (251 of them
PLR2004) against 4 in the default set, and whole-family FURB/PERF are 29/19
against 10/2 -- so select entries have to be rule codes there, not family
names.

Fixes:
- FURB188 x4 generate_bidi.py: removeprefix/removesuffix
- FURB122 x2 generate.py, firefox_profile.py: writelines
- FURB167 x2 update_cddl.py: re.S -> re.DOTALL
- FURB192 x2 pinned_browsers.py, update_cdp.py: sorted(...)[-1] -> max(...)
- C400 x2, C408 x2, C417, C419, C420: comprehension rewrites
- PERF102 x2 cdp.py: .items() -> .values()
- FLY002 remote_server_tests.py: static join -> literal

Note on FURB192: sorted(...)[-1] and max(...) disagree when two entries tie on
the sort key -- a stable sort returns the last tied element, max the first.
Both call sites pick the newest chrome-for-testing build within a milestone,
where version strings are unique, so this is inert in practice. Calling it out
because it is the only behavioural change in the commit.

The comprehension rewrites in webdriver.py touch _wrap_value/_unwrap_value and
the one in _script_handlers.py feeds a BiDi preload script, so those were
checked for equivalence against the pre-change code over a generated input
corpus before being applied. bazel test --test_size_filters=small //py:unit
passes 32/32; //py:ruff-check and //py:ruff-format --check are clean.

Deliberately not adopted here: FURB110 (18, if-exp -> or, a readability
regression as often as not) and the rest of PERF (PERF401/403 comprehension
rewrites, PERF203 try-except-in-loop), which are loop-shape changes rather
than mechanical ones.
@AutomatedTester
[py] settle shebang/exec-bit consistency and enforce ruff EXE (#17989)
5eaefa7 
Tranche 1 of #17989, split out from the mechanical-rewrite commit because it is
the only part that changes git file modes.

Six files disagreed with themselves about whether they are directly
executable. All six are py_binary srcs, invoked through Bazel
(./go update_cddl -> bazel run //scripts:update_cddl) or with an explicit
interpreter (py/tox.ini uses {envpython} ./generate_api_module_listing.py), so
none of them ever relies on its shebang. Resolved all six the same way -- not
directly executable:

- chmod -x (100755 -> 100644), which clears EXE002:
  py/generate_api_module_listing.py, py/generate_bidi.py, py/run_mypy.py,
  py/run_sphinx_autogen.py
- drop the now-unused shebang, which clears EXE001:
  scripts/update_cddl.py, scripts/update_docfx.py

//scripts:update_cddl, //scripts:update_docfx, //py:generate-api-listing,
//py:generate_bidi, //py:mypy and //py:sphinx-autogen all still build.
@AutomatedTester
[py] replace the assert 1 == 0 idiom with pytest.raises and enforce P…
b9c9cd2 
…LR0133 (#17989)

Tranche 1 of #17989. All four PLR0133 (comparison-of-constant) findings are the
same shape in visibility_tests.py:

    try:
        element.click()
        assert 1 == 0, "should have thrown an exception"
    except (ElementNotVisibleException, ElementNotInteractableException):
        pass

which is pytest.raises spelled the long way round, and reports "assert 1 == 0"
rather than "DID NOT RAISE" when the expectation is not met. Converted all four
to a with pytest.raises(...) block. PT011 and PT012 are already in
extend-ignore, so the tuple form and the trailing assert in the send_keys test
need no further exemption.

Adds PLR0133 as an individual rule rather than the PLR family: whole-family PLR
is 332 findings (251 of them PLR2004 magic-value-comparison), and it also
carries PLR0913/PLR0917 (too-many-arguments / -positional-arguments, 45 hits)
which land on public constructor signatures the API-compatibility invariant
protects.

//py:test/selenium/webdriver/common/visibility_tests-chrome passes 15/15, with
all four converted tests confirmed as run rather than skipped.
@AutomatedTester
[py] enforce ruff PIE and PLE (#17989)
028466a 
Tranche 1 of #17989. Both families are small and clean at whole-family scope,
so they go in as bare family names.

PIE790 x5 -- dropped the unnecessary `pass` after a docstring in cdp.py,
webdriver.py (start_client/stop_client), webelement.py (BaseWebElement) and
the test webserver. Safe autofix.

The remaining four findings are false positives, suppressed with a `noqa` and
a reason rather than "fixed":

- PIE796 x3, generate_bidi.py CddlType: the duplicate Enum values are
  deliberate aliases (TEXT->TSTR, INT/NINT->UINT), and get_annotation already
  resolves them through `__members__`, which includes aliases. The bug this
  rule would have caught was fixed in 9d0ae9e; what is left is the intended
  pattern, so the comment now says so explicitly.
- PLE0605 x1, webdriver/__init__.py: `__all__ = sorted(_LAZY_IMPORTS.keys())`.
  sorted() returns a list, so this is correct at runtime; the rule only accepts
  a literal. The usual argument for a literal is static readability, but
  __init__.pyi already re-exports every name explicitly with `as` aliases, so
  type checkers never read this line. Deriving __all__ from _LAZY_IMPORTS is
  what stops the two from drifting at runtime, so it stays.

Note for reviewers: nothing currently asserts that __init__.pyi's re-export
list and _LAZY_IMPORTS agree. That gap is worth its own change; it is not
introduced here.

Verified: regenerating the BiDi sources with this generator produces output
byte-identical to trunk's across all 22 files, apart from the one
_script_handlers.py line changed deliberately in the mechanical commit.
//py:unit passes 32/32, including BaseWebElement's ABC subclass/register
behaviour and the start_client/stop_client no-op hooks whose `pass` was
removed.
@AutomatedTester
[py] enforce ruff bugbear (B) with carve-outs (#17989)
f905272 
Tranche 2 of #17989. Adopts B as a family rather than a list of rule codes, so
that new bugbear rules in a future ruff release surface as one review decision
instead of arriving silently. Whole-family B is 59 findings against the 33 ruff
0.16 enables by default, so four rules are listed in extend-ignore.

Fixed (7):
- B009 x5 / B010 x1 -- constant getattr/setattr in print_page_options.py,
  proxy.py and timeouts.py. Safe autofix. proxy.py's is inside a descriptor's
  __set__; proxyType is a plain class attribute rather than another descriptor,
  so direct assignment is equivalent, and the dynamic setattr(obj, self.name, ...)
  on the next line is left alone.
- B006 x1 -- javascript/private/gen_file.py `def main(argv=[])`. The default is
  unreachable in practice (the only caller is `main(sys.argv)`) and would
  IndexError on argv[1] if it were ever used, so it is dropped rather than
  replaced with a None sentinel.
- B018 x1 -- click_scrolling_tests.py accessed `.size` for its side effect to
  prove that reading it does not scroll; now `_ = ...` so the intent is explicit.

Carved out via per-file-ignores (18):
B018 under py/test/** and in py/conftest.py. 17 of the 19 findings are a
property access inside `with pytest.raises(...)`, which is the idiomatic way to
assert that reading a property raises; the 19th is the driver-liveness probe in
conftest.py. Note that conftest.py is not under py/test/**, so the pattern
suggested on the issue would have missed it. Both globs resolve against the
working directory rather than this file, which is why they are spelled
"py/..." -- there is now a comment saying so, because they silently stop
matching if ruff is invoked from inside py/.

Deferred via extend-ignore:
- B904 (17) raise-without-from-inside-except. This is the highest-value rule in
  the whole 184-finding set for a library -- it materially improves user
  tracebacks -- but it is 16 findings in py/selenium and deserves its own change
  rather than being bundled into a lint-config commit.
- B017 (7) pytest.raises(Exception). Narrowing each site needs the actual
  exception type confirmed against a live browser (three are BiDi tests, where
  8cbf4bf's typed errors make narrowing most valuable), so it is deferred
  rather than guessed.
- B028 (4), B007 (3), B024 (2).

//py:unit passes 32/32. //javascript/chrome-driver:header, :source and
//javascript/ie-driver:header build clean, covering the gen_file.py change.
@AutomatedTester
[py] narrow pytest.raises(Exception) and enforce B017 (#17989)
075bc72 
Tranche 2 of #17989, completing the bugbear adoption by dropping B017 from
extend-ignore. Each exception type was confirmed by running the Bazel test
target with a deliberately wrong assertion and reading the type back out of the
failure, rather than by guessing:

- proxy_tests.py x2 -> ValueError, from Proxy._verify_proxy_type_compatibility.
- executing_javascript_tests.py -> TypeError. Passing the driver as a script
  argument is rejected by json.dumps client-side, before any command is sent.
- _bidi/browsing_context_tests.py x2 -> NoSuchFrameException, for get_tree on a
  closed context.
- _bidi/script_tests.py -> NoSuchHandleException, for call_function on a
  disowned handle.

The two BiDi files are where this is most worth having: 8cbf4bf added typed
errors for BiDi wire error codes, so these assertions now pin the exact code
the spec requires instead of accepting anything at all.

quit_tests.py needed a tuple. Which error surfaces after driver.quit() depends
on what is left listening -- against a local driver the process is gone and
urllib3 raises MaxRetryError, while against a Grid the server is still up and
the session lookup fails with InvalidSessionIdException. Both were confirmed by
running quit_tests-chrome and quit_tests-chrome-remote. Worth noting for a
separate change: MaxRetryError is a raw urllib3 error escaping the public API,
and arguably ought to be wrapped in a WebDriverException.

Verified: proxy_tests-chrome, quit_tests-chrome, quit_tests-chrome-remote and
executing_javascript_tests-chrome all pass. In the BiDi suites the four
narrowed tests passed 6/6 across repeat runs; the one failure in those files is
test_activate_browsing_context, which fails roughly one run in two both here
and on trunk (asynchronous focus handover) and is unrelated to this change.
//py:ruff-check clean, //py:unit 32/32.
@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AutomatedTester @selenium-ci