[java][bidi] Add BiDi code generator - #17777
Open
pujagani wants to merge 57 commits into
Open
Conversation
selenium-ci
added
C-java
Contributor
PR Summary by QodoAdd Bazel-based Java BiDi protocol code generator
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
This was referenced Jul 15, 2026
Member
|
I started reviewing this and ended up creating #17786 |
Contributor
Code Review by Qodo
1. Void events never dispatched
|
Contributor
|
Code review by qodo was updated up to the latest commit 7b9799f |
Contributor
|
Code review by qodo was updated up to the latest commit 1760de4 |
Contributor
|
Code review by qodo was updated up to the latest commit 1f4ec87 |
| ], | ||
| ) | ||
|
|
||
| genrule( |
Contributor
There was a problem hiding this comment.
1. Unquoted genrule arguments 🐞 Bug ☼ Reliability
The new :generate-bidi genrule builds a shell command without quoting the tool path, schema path, or output path, so the action can fail when any of those paths contain spaces or shell-special characters. This makes BiDi generation brittle and can break Bazel builds depending on workspace/execroot location.
Agent Prompt
### Issue description
The `genrule(name = "generate-bidi")` command concatenates `$(execpath ...)`, `$(location ...)`, and `$@` into a shell command without quoting. If any expanded path contains whitespace or shell-special characters, the generator will receive split/incorrect arguments and the build will fail.
### Issue Context
This genrule is new in this PR and is used to generate `bidi-generated.srcjar` during the build.
### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BUILD.bazel[59-65]
### Suggested fix
Update `cmd` to quote each argument as a single shell token, e.g.:
```bzl
cmd = "\"$(execpath :bidi-client-generator)\" \"$(location //javascript/selenium-webdriver:create-bidi-src_schema)\" \"$@\"",
```
(or equivalent quoting per the repo’s Bazel shell conventions), so paths with spaces are handled correctly.
ⓘ 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 3d4a143 |
Member
|
Good progress. Heads up in case you missed it, #17784 added the preserveExtras signal to the projected schema (alongside the existing extensible), so extensibility is now fully derivable per-type: extensible marks the open types, and preserveExtras is pre-scoped to the ones that are both extensible and re-sendable so you don't have to work out that scoping in the generator. |
pujagani
force-pushed
the
java-bidi-codegen
branch
from
July 29, 2026 12:07
3d4a143 to
a2fa95c
Compare
| handle.unsubscribe(subscriptionId); | ||
| } | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
1. Missing trailing newline 🐞 Bug ⚙ Maintainability
Module.java now ends without a final newline, which can violate standard source formatting conventions and can trip formatting/lint checks in some environments.
Agent Prompt
### Issue description
`java/src/org/openqa/selenium/bidi/Module.java` ends without a trailing newline.
### Issue Context
The PR diff explicitly indicates `No newline at end of file` on the final `}`.
### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[68-68]
ⓘ 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 7e5e00c |
…17863) * [rb] capture log matcher output under SE_DEBUG and keep service specs env-agnostic * [rb] drop the SE_DEBUG output lock in the spec log-capture helper
… schema (SeleniumHQ#17864) [bidi] derive per-type inbound/outbound directionality in the shared schema
Co-authored-by: Navin Chandra <navinchandra772@gmail.com>
…#17866) * [build] test JavaScript on oldest and newest supported Node and publish on npm 11 * [build] run JavaScript unit tests on pull requests
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com>
…7870) [py] accept By in find_element/find_elements type hints (SeleniumHQ#17867)
* [build] regenerate CDP in the daily browser workflow if chrome stable version missing directory * [build] run full Ruby tests on major browser PRs
…eniumHQ#17874) * [js] wait for async BiDi log/mutation events and window resize in flaky tests * [js] skip window resize polling on IE where getSize is approximate
…umHQ#17871) * [dotnet][java][js][rb] deprecate dead Firefox profile members and add reuse specs
Update pinned browser versions Co-authored-by: Selenium CI Bot <selenium-ci@users.noreply.github.com> Co-authored-by: Titus Fortner <titusfortner@users.noreply.github.com>
… pod-wait + relocation (SeleniumHQ#17876)
…leniumHQ#17757) * fix(rust): prevent empty driver version from being cached in metadata Edge and Firefox cached empty driver_version in metadata when a version-less HTTP response was received (e.g. misconfigured driver-mirror-url or intercepting proxy). Chrome already guarded against this with `!driver_version.is_empty()`. This change applies the same guard to Edge and Firefox. Also adds unit tests for previously untested pure logic: - parse_version (version string parsing, error short-circuit, ESR/snap) - get_index_version (major/minor/patch extraction, error cases) - Version predicates (is_stable, is_beta, is_dev, is_nightly, is_esr, is_version_specific) - get_major_version, get_major_browser_version - str_to_os, OS::is, ARCH::is (config parsing) - get_env_name (env var name generation) - find_latest_from_cache, collect_files_from_cache - is_driver_and_matches_browser_version (including empty-major bug) - find_best_driver_from_cache (match, fallback, empty cache) Tests document latent bugs: lexical sort misorders across digit boundaries (9 vs 10), and starts_with("") matches all cached drivers when major version is empty. Fixes SeleniumHQ#17641 * fix: address Copilot review feedback - Use manager's actual get_platform_label() instead of hardcoded values, so tests work on Apple Silicon (mac-arm64) and 32-bit Windows (win32) - Rename get_major_version_errors_on_garbage to get_major_version_returns_first_component_for_non_numeric since the test asserts Ok, not Err * fix(rust): address qodo review — add regression test for empty driver guard, fix i686 arch classification - Add empty_driver_version_is_not_cached_in_metadata regression test that fails if the !driver_version.is_empty() guard is removed from edge.rs/firefox.rs - Move i686 from ARCH::X64 to ARCH::X32 aliases in config.rs so get_normalized_arch() correctly returns ARCH_X86 for 32-bit systems - Update config_unit_tests.rs to expect i686 under X32 and verify X64 does not match i686 * extract should_cache_driver_version helper for testable regression guard * add doc comment to should_cache_driver_version * fix(rust): address Qodo feedback - explicit as_str, remove unused rstest import, add ARCH docs * fix: resolve CI formatting and test failures - Fix rustfmt formatting in chrome.rs, edge.rs, firefox.rs (if-statement wrapping) - Fix import ordering in config_unit_tests.rs and version_unit_tests.rs - Fix collect_files_from_cache to filter files only, not directories - Fix get_index_version to return error on empty string input * style: apply go format changes to test files - Reformat Apache license headers in cache_unit_tests.rs, config_unit_tests.rs, version_unit_tests.rs - Reorder imports and collapse function signatures in cache_unit_tests.rs - Collapse fs::write calls to single line - Reformat get_driver_version_from_metadata call
…ions (SeleniumHQ#17875) * [dotnet] support SE_*DRIVER environment variables to set driver locations * [dotnet] make env-var driver path internal, log env-var driver source * [dotnet] test env-var driver path via DriverProcessStarting event --------- Co-authored-by: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com>
SeleniumHQ#17815) * [java] Fix By.className()/By.id() misescaping non-ASCII leading digits PreW3CLocator.cssEscape (used by By.className/By.id, duplicated in W3CHttpCommandCodec) used Character.isDigit() to detect a leading digit needing CSS escaping. That accepts any Unicode decimal digit, not just ASCII 0-9, so a leading non-ASCII digit (e.g. Arabic-Indic U+0665) got rewritten using its numeric value as if it were the ASCII digit of the same value, producing the same selector as an unrelated ASCII-digit class/id (By.className("٥foo") collided with By.className("5foo")). Per the CSS Syntax spec, "digit" is ASCII 0-9 only; non-ASCII code points are already valid identifier-start characters and need no escaping, so the fix narrows the check to the ASCII range instead of adding new escaping logic. * [java] Add direct regression coverage for W3CHttpCommandCodec.cssEscape W3CHttpCommandCodec carries a byte-identical copy of the leading-digit CSS-escape logic fixed in By.java, but had no test file exercising it at all. ByTest/RemotableByTest only cover the By.java copy, since By.toJson() pre-escapes client-side before anything reaches a command codec, so a regression in this copy alone (e.g. from a future de-duplication refactor) would go unnoticed. Add W3CHttpCommandCodecTest exercising encode() for the "class name"/ "id" locator strategies directly, mirroring the ByTest cases. Verified these tests fail against the pre-fix implementation and reproduce the exact collision from the original report. * [java] Use HttpRequest#contentAsString() instead of deprecated Contents.string in test --------- Co-authored-by: Titus Fortner <titus.fortner@gmail.com>
Comment on lines
+1
to
+4
| diff --git a/ruby/private/utils.bzl b/ruby/private/utils.bzl | ||
| index bfc02aba..f10b2abf 100644 | ||
| --- a/ruby/private/utils.bzl | ||
| +++ b/ruby/private/utils.bzl |
Contributor
There was a problem hiding this comment.
1. New file under third_party/ 📘 Rule violation § Compliance
The PR adds third_party/bazel/rules_ruby_windows_batch_crlf.patch, which violates the policy forbidding direct changes under third_party/. This increases maintenance and supply-chain risk because third-party content should be updated only via the approved vendoring mechanism.
Agent Prompt
## Issue description
A new file was added under `third_party/`, which is disallowed by the compliance checklist.
## Issue Context
Third-party changes should be performed via the repo’s vendoring/update tooling (or moved out of `third_party/` into a project-owned location), rather than committing direct edits/additions under `third_party/`.
## Fix Focus Areas
- third_party/bazel/rules_ruby_windows_batch_crlf.patch[1-39]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
24
to
+25
| describe DevTools, skip_unless: [{bidi: false, reason: 'Not yet implemented with BiDi'}, | ||
| {browser: %i[chrome edge]}] do | ||
| {browser_family: :chromium}] do |
Contributor
There was a problem hiding this comment.
2. Missing reason in skip_unless 📘 Rule violation ▣ Testability
New/modified integration-spec guards use skip_unless without providing an explicit reason, making it harder to audit and diagnose why specs are skipped. Add a non-empty reason (string or issue id per project convention) to each guard condition that currently omits it.
Agent Prompt
## Issue description
Several RSpec integration test guards were changed to include conditions without an explicit `reason`, which violates the requirement to always include a reason on integration test guards.
## Issue Context
These specs already pass `reason:` for some guard conditions (e.g., `bidi: false`), so the guard mechanism clearly supports it; consistency and auditability require all guard entries to include a reason.
## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
- rb/spec/integration/selenium/webdriver/driver_finder_spec.rb[70-72]
- rb/spec/integration/selenium/webdriver/fedcm_spec.rb[25-26]
- rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb[24-25]
ⓘ 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 c72ff7f |
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
8 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.
🔗 Related Issues
💥 What does this PR do?
Adds the BiDi Java client generator: reads the shared, binding-neutral BiDi schema and emits the full typed Java BiDi protocol layer — module classes (one per domain, with commands and events) plus supporting POJOs, enums, and discriminated unions — across all 14 domains (~79 commands, 27 events, 298 generated classes total), on top of the hand-written seam (Module base class + Handle) that makes it usable.
Shape of a generated module class (real output, network.Network trimmed to 2 events + 2 commands — the full file has 5 events and 13 commands):
Usage is symmetric regardless of whether a command has a result:
🔧 Implementation Notes
Autogenerated code and not checked in
The build compiles the genrule's srcjar directly:
No generated
.javais ever committed.Trade-off accepted knowingly: less reviewable diff surface for generator changes. Given Java has a lot of generated classes, strict typing means one
.javafile per record/enum/union, not a handful of dynamically-typed modules like the JS or Python bindings. The actual generated code never appears in this PR's diff, only the generator that produces it.How to see the generated output since it's not in the diff, pull it out locally:
bazel build //java/src/org/openqa/selenium/bidi:bidi-generated mkdir -p /tmp/bidi-generated-src unzip -o -q bazel-bin/java/src/org/openqa/selenium/bidi/bidi-generated.srcjar -d /tmp/bidi-generated-src open /tmp/bidi-generated-src # or just browse/grep it298 files across 14 domains will land there. A few worth spot-checking first, each demonstrating a specific decision from the Implementation Notes above:
org/openqa/selenium/bidi/protocol/module/Network.java— a representative module class (commands + events)org/openqa/selenium/bidi/protocol/browsingcontext/SetViewportParameters.java— explicit-null vs. omitted-key handling side by side (viewport/devicePixelRatioare nullable,context/userContextsaren't)org/openqa/selenium/bidi/protocol/emulation/SetTimezoneOverrideParameters.java— required-but-nullable field on a sender typeorg/openqa/selenium/bidi/protocol/script/PrimitiveProtocolValue.javaandStringValue.java— theinterface extends/implementschain for multi-parent unionsIf you'd rather regenerate directly without the Bazel packaging step (useful for diffing output after a generator change):
bazel build //java/src/org/openqa/selenium/bidi:bidi-client-generator bazel run //java/src/org/openqa/selenium/bidi:bidi-client-generator -- \ "$(bazel info bazel-genfiles)/javascript/selenium-webdriver/create-bidi-src_schema.json" \ /tmp/bidi-out.srcjar unzip -o -q /tmp/bidi-out.srcjar -d /tmp/bidi-generated-src(Direct invocation of the built binary without
bazel runfails withCannot locate runfiles directory— it needsbazel runto set up its runfiles tree.)Explicit-null vs. omitted-key tracking
Tracked per-field for fields the schema marks nullable, so
toMap()can distinguish:"field": null→ clear itThis is gated strictly on the schema's own
nullableflag, not applied blanket to every optional field.Required-but-nullable fields
@Nullableon their constructor parameter (boxed, if the field would otherwise be a Java primitive).ConstructorCoercerwas extended to honor jspecify's@Nullable(previously onlyOptional<T>was treated as nullable).🤖 AI assistance
💡 Additional Considerations
Test coverage is not uniform across domains.
Only network, log, script, and browsingContext have dedicated generated-code tests (unit and/or browser-integration) added in this PR.
session, browser, emulation, storage, input, webExtension, permissions, speculation, bluetooth, userAgentClientHints have none yet.
Essentially, the modules that are needed for implementing high-level APIs for Selenium 5 have tests.
Extensible/unknown-key round-tripping (unrecognized JSON keys silently dropped) is a known, scoped gap, not implemented in this PR. Not implemented because ConstructorCoercer maps JSON keys 1:1 to constructor parameters and silently drops anything unmatched — supporting round-trip needs a genuinely different deserialization path (capturing unrecognized keys into a side bag alongside the typed fields), which is new mechanism work, and fairly big. So this is something we might not support in Java at all. We can see how much this is required and decide later.
Rest, will be added in a follow up PR.
🔄 Types of changes