Skip to content

[py] raise typed WebDriver errors for BiDi wire error codes - #17952

Open
titusfortner wants to merge 2 commits into
SeleniumHQ:trunkfrom
titusfortner:py-bidi-generated-error-classes
Open

[py] raise typed WebDriver errors for BiDi wire error codes#17952
titusfortner wants to merge 2 commits into
SeleniumHQ:trunkfrom
titusfortner:py-bidi-generated-error-classes

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

Python counterpart of #17855, which did this for Ruby.

💥 What does this PR do?

Note that this is for the BiDi implementation generated from the schema not the code currently in use off the driver.

  • BiDi wire errors now raise a typed exception instead of a bare WebDriverException: codes the classic error handler already types keep that class, and BiDi-only codes get their own.
  • Backwards compatible with classic error handling — except NoSuchElementException catches a BiDi failure and a classic one alike.

🔧 Implementation Notes

  • The exception classes are generated from the schema's ErrorCode enum as real class statements, so they type-check, autocomplete and document like every other exception in the bindings.
  • Reconciling with the classic exceptions reads errorhandler's own ErrorCode/ExceptionMapping tables at generation time rather than keeping a second copy, so a class the handler retypes later follows on the next build — this is why the generator now depends on :remote.
  • Codes the handler resolves to bare WebDriverException (unknown error, unsupported operation) get a declared subclass instead: strictly more specific, still caught by anyone catching the base.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code (Opus 5)
    • What was generated: the generator change, the tests, and this description
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

  • Alternatives considered:
    • Minting at import with type() behind a module __getattr__, which is what the first pass did — mypy resolves any name off such a module, so a misspelled exception name imports clean and every class narrows only to type[WebDriverException], and the classes never reach Sphinx autodoc or dir(). It also let the generated table and the runtime surface disagree: NoSuchAlertException and UnableToCaptureScreenException were listed but raised AttributeError, because those two codes resolve to NoAlertPresentException and ScreenshotException.
    • Hand-writing the classes in selenium/common/exceptions.py — they would be public and could carry real docstrings and a hierarchy, but the list drifts from the schema on every spec bump and needs a coverage test to stay honest.
    • Generating a .pyi stub next to the minted classes — cannot be made correct from the schema alone: a stub built from the name table would declare the two classes above, which do not exist, and would type the ten shared codes as distinct from their classic counterparts, breaking except matching.
    • Two-stage generation, keeping the schema generator selenium-free and adding a second selenium-aware tool — same output for twice the build wiring, and generation depends on errorhandler either way, so the separation is organizational rather than real.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

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

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Raise typed WebDriver exceptions for BiDi wire errors

✨ Enhancement 🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Generate typed BiDi exceptions from schema while reusing classic WebDriver mappings.
• Raise mapped exceptions with remote messages and stack traces from BiDi replies.
• Cover shared, BiDi-only, unknown, and message-less error responses.
Diagram

graph TD
  Schema["BiDi schema"] --> Generator["Protocol generator"] --> Errors["Error registry"] --> Exception["Typed exception"]
  Classic["Classic mappings"] --> Generator
  Browser["BiDi reply"] --> Transport["BiDi transport"] --> Errors
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Create classes dynamically at import time
  • ➕ Avoids generated class declarations
  • ➕ Keeps runtime mapping compact
  • ➖ Weakens static typing and autocomplete
  • ➖ Hides classes from autodoc and dir()
  • ➖ Can let declared names diverge from raised classes
2. Hand-write public exception classes
  • ➕ Allows curated documentation and inheritance
  • ➕ Keeps exception definitions straightforward
  • ➖ Can drift whenever the BiDi schema changes
  • ➖ Requires separate synchronization coverage
3. Generate type stubs for dynamic classes
  • ➕ Could improve editor and type-checker visibility
  • ➕ Retains dynamic runtime creation
  • ➖ Cannot accurately model classic exception aliases from schema alone
  • ➖ Risks stubs declaring classes absent at runtime

Recommendation: The PR's generation-time reconciliation is the best approach: it emits real, discoverable classes while deriving shared mappings from the classic handler's authoritative tables. This avoids schema drift and preserves cross-protocol exception compatibility without dynamic-runtime or stub inconsistencies.

Files changed (5) +139 / -11

Enhancement (1) +83 / -2
generate_bidi_protocol.pyGenerate typed BiDi exception classes and lookup table +83/-2

Generate typed BiDi exception classes and lookup table

• Generates concrete exception classes for BiDi-only or otherwise untyped schema codes while importing existing classic exception classes for shared codes. Adds a typed lookup with a WebDriverException fallback and includes errors.py in all generated outputs.

py/generate_bidi_protocol.py

Bug fix (1) +11 / -3
transport.pyRaise mapped exceptions for BiDi error replies +11/-3

Raise mapped exceptions for BiDi error replies

• Routes wire error codes through the generated exception registry instead of always raising WebDriverException. Preserves remote messages and converts newline-delimited wire stack traces into exception stack frames.

py/selenium/webdriver/common/_bidi/transport.py

Tests (1) +37 / -4
bidi_transport_tests.pyCover typed BiDi error handling and stack traces +37/-4

Cover typed BiDi error handling and stack traces

• Extends the test connection to emit stack traces and verifies classic mappings, BiDi-specific classes, unknown-code fallback, message fallback, and stack-trace propagation.

py/test/unit/selenium/webdriver/common/bidi_transport_tests.py

Other (2) +8 / -2
BUILD.bazelAdd classic error mappings to generator dependencies +4/-0

Add classic error mappings to generator dependencies

• Adds the remote WebDriver target to both BiDi protocol generator binaries. This allows generation to inspect the classic error handler's code-to-exception tables.

py/BUILD.bazel

generate_bidi_protocol.bzlDeclare the generated errors module +4/-2

Declare the generated errors module

• Adds errors.py to the Bazel rule's expected generated module set and updates comments to describe domain-less generated exceptions.

py/private/generate_bidi_protocol.bzl

@qodo-code-review

qodo-code-review Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. _error drops wire code ✗ Dismissed 📘 Rule violation ≡ Correctness
Description
When a remote message is present, _error now removes the BiDi wire error code from the exception
message, changing the existing observable error text from <code>: <message> to only <message>.
Callers that inspect or match exception messages can break on upgrade despite typed exceptions being
otherwise compatible.
Code

py/selenium/webdriver/common/_bidi/transport.py[R62-64]

+        # The class carries the code, so the message need not repeat it — except where the
+        # remote sent no message, which would otherwise leave nothing to read.
+        message = reply.get("message") or code
Evidence
PR Compliance ID 1 requires existing public behavior to remain compatible. The changed _error
implementation constructs the exception from reply.get("message") or code, so a reply containing
both error and message no longer includes the wire code in the exception message; the PR's
modified unit expectation at bidi_transport_tests.py[121-125] confirms this intentional output
change.

AGENTS.md: Preserve Public API and ABI Compatibility and Follow Deprecation Policy
py/selenium/webdriver/common/_bidi/transport.py[61-66]
py/test/unit/selenium/webdriver/common/bidi_transport_tests.py[121-125]

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 typed-exception implementation removes the BiDi wire error code from exception messages whenever the remote supplies a message, breaking the prior observable `<code>: <message>` format.

## Issue Context
Keep the new typed exception and stacktrace behavior, but preserve the existing message text so callers matching or recording errors remain compatible. Update the focused test to continue asserting the prior format.

## Fix Focus Areas
- py/selenium/webdriver/common/_bidi/transport.py[61-66]
- py/test/unit/selenium/webdriver/common/bidi_transport_tests.py[121-125]

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


Grey Divider

Context sources
Review mode: ⚖️ Balanced: This changes generated exception mapping, build-time dependencies, and BiDi runtime error propagation across multiple files; it has meaningful compatibility and API behavior risk, but not enough independent defect density to justify extended review.

Grey Divider

Tip of the day
💡 Did you know, you can start a comment with 'qodo' or '@qodo' to chat about any finding

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread py/selenium/webdriver/common/_bidi/transport.py
@titusfortner

Copy link
Copy Markdown
Member Author

@cgoldberg & @AutomatedTester & @navin772 can you help make sure this is the right way to do things for Python.
The constraints I used were:

  1. Schema generated stuff stays in _bidi module
  2. Handling the WebDriver classic error handles the equivalent WebDriver bidi error

But, this split wasn't quite as clean as what I did for Ruby.

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.

2 participants