[rb] raise typed WebDriver errors for BiDi from a generated error-code map - #17855
Open
titusfortner wants to merge 4 commits into
Open
[rb] raise typed WebDriver errors for BiDi from a generated error-code map#17855titusfortner wants to merge 4 commits into
titusfortner wants to merge 4 commits into
Conversation
selenium-ci
added
C-rb
Member
|
Thank you, @titusfortner for this code suggestion. The support packages contain example code that many users find helpful, but they do not necessarily represent After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks. |
Contributor
PR Summary by QodoRaise typed BiDi WebDriver errors via generated error-code map
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit 056b5d2 |
Contributor
|
Code review by qodo was updated up to the latest commit 39c008b |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Ruby BiDi “generated Protocol” transport to raise typed Selenium::WebDriver::Error subclasses based on the BiDi wire error code, using a generated wire-code → Ruby-class mapping so the set stays in sync with the shared schema.
Changes:
- Generate
BiDi::Protocol::ErrorCode::CLASS_NAMES(wire error code → Ruby exception class name) from the BiDi schema, plus matching RBS signatures. - Register BiDi-only error codes as
Selenium::WebDriver::Error::WebDriverErrorsubclasses while reusing existing classic error classes for shared codes. - Update
BiDi::Transportto raise a typed exception resolved viaProtocol::ErrorCode.for, with unit coverage for shared, BiDi-only, and unknown codes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rb/spec/unit/selenium/webdriver/bidi/transport_spec.rb | Asserts typed exceptions for shared codes, BiDi-only codes, and fallback behavior. |
| rb/sig/lib/selenium/webdriver/bidi/transport.rbs | Updates signature to reflect error_for returning an exception instance. |
| rb/sig/lib/selenium/webdriver/bidi/protocol/error_code.rbs | Adds generated RBS for the ErrorCode map and BiDi-only error subclasses. |
| rb/sig/lib/selenium/webdriver/bidi/error.rbs | Declares the Protocol::ErrorCode.for method for typed error resolution. |
| rb/lib/selenium/webdriver/BUILD.bazel | Wires new templates/data into the generator and verification test. |
| rb/lib/selenium/webdriver/bidi/transport.rb | Raises typed errors via Protocol::ErrorCode.for instead of a generic WebDriverError. |
| rb/lib/selenium/webdriver/bidi/support/templates/error_code.rbs.erb | Template for generated RBS for error-code map + BiDi-only error classes. |
| rb/lib/selenium/webdriver/bidi/support/templates/error_code.rb.erb | Template for generated Ruby error-code map. |
| rb/lib/selenium/webdriver/bidi/support/check_generated.rb | Extends generated-code freshness checks to include error_code.rb. |
| rb/lib/selenium/webdriver/bidi/support/bidi_generate.rb | Adds schema → error-code map generation and emits protocol/error_code.{rb,rbs}. |
| rb/lib/selenium/webdriver/bidi/protocol/error_code.rb | Generated wire-code → Ruby-class mapping. |
| rb/lib/selenium/webdriver/bidi/protocol.rb | Requires classic errors + new BiDi error-code map + BiDi error registrations. |
| rb/lib/selenium/webdriver/bidi/error.rb | Registers BiDi-only error subclasses and implements Protocol::ErrorCode.for. |
| class BiDi | ||
| module Protocol | ||
| module ErrorCode | ||
| def self.for: (String? code) -> singleton(::Selenium::WebDriver::Error::WebDriverError) |
Comment on lines
+44
to
+52
| # Whether the checked-in protocol/error_code.rb matches what the generator would render now. | ||
| def self.error_module_current?(schema, protocol_dir) | ||
| codes = error_code_map(schema) | ||
| path = File.join(protocol_dir, 'error_code.rb') | ||
| return codes.empty? unless File.exist?(path) | ||
|
|
||
| mod = ErrorModule.new(filename: 'error_code', codes: codes) | ||
| File.read(path) == render(mod, File.join(__dir__, 'templates', 'error_code.rb.erb')) | ||
| end |
Comment on lines
+44
to
+52
| # Whether the checked-in protocol/error_code.rb matches what the generator would render now. With | ||
| # no error codes nothing is generated, so an existing file is stale and must be removed. | ||
| def self.error_module_current?(schema, protocol_dir) | ||
| codes = error_code_map(schema) | ||
| path = File.join(protocol_dir, 'error_code.rb') | ||
| return !File.exist?(path) if codes.empty? | ||
|
|
||
| mod = ErrorModule.new(filename: 'error_code', codes: codes) | ||
| File.exist?(path) && File.read(path) == render(mod, File.join(__dir__, 'templates', 'error_code.rb.erb')) |
Contributor
There was a problem hiding this comment.
1. Unfixable stale-check guidance 🐞 Bug ☼ Reliability
If the schema ever has no ErrorCode values, error_module_current? treats any existing protocol/error_code.rb as stale, but check! still instructs only regeneration; regeneration won’t remove the file because the generator skips emitting the error module when codes is empty. This can leave developers stuck in a failing generated-code check until they manually delete stale error_code.rb (and likely the corresponding .rbs).
Agent Prompt
### Issue description
When the schema’s `ErrorCode` enum becomes empty, `check_generated` will fail if a previously-generated `protocol/error_code.rb` still exists, but the script only prints a regeneration command. The generator explicitly returns early when there are no codes, so regeneration cannot remove the stale files.
### Issue Context
This manifests when the schema changes from having `ErrorCode` values to having none (or when a stale `error_code.rb`/`error_code.rbs` is present). The check should either:
- explicitly instruct the user to delete the stale artifacts, and/or
- have the generator proactively remove previously-generated artifacts when `codes.empty?`.
### Fix Focus Areas
- rb/lib/selenium/webdriver/bidi/support/check_generated.rb[32-41]
- rb/lib/selenium/webdriver/bidi/support/check_generated.rb[44-53]
- rb/lib/selenium/webdriver/bidi/support/bidi_generate.rb[1146-1155]
### Suggested change
1) In `check!`, if `error_code_map(schema).empty?` and `protocol/error_code.rb` exists, print an additional warning like: “Schema has no ErrorCode values; delete protocol/error_code.rb (and sig/.../error_code.rbs)”.
AND/OR
2) In `emit_error_module`, when `codes.empty?`, delete any existing generated `error_code.rb` and `error_code.rbs` at the target locations so `bazel run ...:bidi-generate` fully remediates the state.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 8ec33ab |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 Related Issues
💥 What does this PR do?
WebDriverError.🔧 Implementation Notes
Protocol::ErrorCodemap (wire code → Ruby class name) from the CDDLErrorCodeenum; the generated file is pure data and references nothing else.no such node,no such handle, …) as aWebDriverErrorsubclass inSelenium::WebDriver::Error; codes shared with the classic protocol (no such element,invalid argument, …) reuse the existing classic class, so arescueis identical whether a session ran over BiDi or classic — which matters because the transport is selected silently.Protocol::ErrorCode.for, falling back toWebDriverErrorfor anything outside the closed set.🤖 AI assistance
💡 Additional Considerations
browser/browsing_context/network) still talk raw JSON viasend_cmdand don't yet benefit from the typed errors; migrating them onto the generated Protocol layer is follow-up work.🔄 Types of changes