[java] Fix BiDi not initializing for RemoteWebDriver built via builder - #17778
[java] Fix BiDi not initializing for RemoteWebDriver built via builder#17778baflQA wants to merge 3 commits into
Conversation
PR Summary by QodoRemoteWebDriver: decide BiDi setup based on returned capabilities
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (Boolean.TRUE.equals(capabilities.getCapability("webSocketUrl"))) { | ||
| this.biDi = createBiDi(); | ||
| } | ||
| this.biDi = createBiDi(); |
There was a problem hiding this comment.
1. createbidi() called unconditionally 📘 Rule violation ≡ Correctness
RemoteWebDriver.startSession now unconditionally calls createBiDi(), which can eagerly establish a BiDi WebSocket connection whenever the server returns a string webSocketUrl, changing user-visible behavior even if the client never explicitly enabled BiDi. Compliance requires either cross-binding comparison evidence (since other bindings appear to initialize BiDi lazily/on first use) or clear documentation of any intentional divergence, as this can add session-start latency and trigger unexpected outbound WebSocket attempts/warnings.
Agent Prompt
## Issue description
Java `RemoteWebDriver.startSession` now eagerly initializes BiDi by unconditionally calling `createBiDi()`, which can immediately open a WebSocket whenever the server returns a string `webSocketUrl` in the *returned* capabilities. This can create unsolicited BiDi connections (even if the client never enabled BiDi), add session-start latency, and produce unexpected outbound WebSocket attempts/warnings; per compliance, you must either align behavior with other bindings (which appear to initialize BiDi lazily/on first use) or provide explicit cross-binding comparison evidence and document any intentional divergence.
## Issue Context
- `startSession` now assigns `this.biDi = createBiDi()` during session creation based solely on the returned `webSocketUrl` capability.
- `createBiDi()` opens a WebSocket immediately when `webSocketUrl` is present (through `new Connection(...)` and `Connection` invoking `client.openSocket(...)`).
- Elsewhere BiDi is treated as explicit opt-in: `enableBiDi()` sets `webSocketUrl: true`, and tests describe the expected flow as “client requests `webSocketUrl: true` … then connects”.
- Other Selenium language bindings in this repo (e.g., Python/JavaScript) appear to initialize BiDi lazily when BiDi APIs are accessed rather than during session creation.
- Compliance requires cross-binding comparison for user-visible behavior changes in a binding, or clear documentation/rationale near the change for any intended divergence (and reflected in tests if applicable).
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[292-296]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
createBiDi() returns an empty Optional unless the remote actually returned a webSocketUrl, so no connection is opened for browsers that didn't negotiate BiDi — and this only restores the eager behavior ChromiumDriver/FirefoxDriver already had before #17753 moved BiDi creation up into RemoteWebDriver."
| if (Boolean.TRUE.equals(capabilities.getCapability("webSocketUrl"))) { | ||
| this.biDi = createBiDi(); | ||
| } | ||
| this.biDi = createBiDi(); |
There was a problem hiding this comment.
2. Bidi regression test missing 📘 Rule violation ▣ Testability
This bug-fix change alters BiDi initialization behavior in startSession, but the provided change set includes no updated/new automated test that would fail if the change were reverted. The compliance checklist requires regression coverage for bug fixes affecting observable behavior.
Agent Prompt
## Issue description
A behavioral bug fix to BiDi initialization lacks an accompanying regression test update in the provided change set.
## Issue Context
The change modifies session startup behavior (`startSession`) to set `biDi` via `createBiDi()`. Per compliance, bug fixes and user-visible behavior changes should have an automated test that would fail under the pre-fix behavior.
## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[295-295]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit cd4e3ab |
diemol
left a comment
diemol
left a comment
There was a problem hiding this comment.
Why the need for this change? This will create a BiDi connection for every browser, even if it is not desired or the browser does not support it.
Not true. The createBiDi() method does all the required checks, returning an empty optional if many conditions are not fulfilled. If the webSocketUrl is not among the returned capabilities, or it does not match the ws url pattern, it won't be created. but since in the response it's not a boolean value, but the WS url, we would basically have to duplicate the checks that are already performed in the createBiDi method. |
|
@baflQA reasoning is correct here, and the solution is good. Can you add a test? |
titusfortner
changed the title
|
If i will be able to setup the workspace, I'll do my best. |
|
We should check if the websocketurl was set by the requested capabilities before creating a BiDi session i.e. the user requested for BiDi. That check should not be removed ideally. createBiDi() does checks on the returned capabilities and if that is a correct websocketURL to make the websocket connection, which ideally will happen only if BiDi was requested from end-user. That is how the code should look. For Firefox, when it does not support BiDi returns the same capability value back i.e. boolean value, hence the checks in createBiDi(). I am not sure why that breaks when using the builder. Can you please share a code snippet of how you are invoking the RemoteWebDriver builder that worked before the regression? |
|
Based on the discussion with @baflQA on Slack and reading the spec, the changes here should be fine. There shouldnt be scenario that websocketurl exists in returned caps without user requesting it based on the spec and my understanding of it. I will also help adding tests. Thank you @baflQA for your contribution! |
|
Code review by qodo was updated up to the latest commit f9e37e5 |
|
Closing this, I was unable to add tests to this branch. So I have same changes and commits in another PR with unit tests added #17792 |
5 participants
Related Issues
N/A
What does this PR do?
Fixes
RemoteWebDriverto checkthis.capabilities(response capabilities from server) instead of the localcapabilitiesparameter when detecting BiDi WebSocket URL support.Implementation Notes
capabilitieson line 293 is set to the response from the session — the server may returnwebSocketUrlin the response capabilities even if not in the requested ones. Using the local parameter instead ofthis.capabilitiesmissed this case.No alternatives considered — single-character fix to use correct variable.
AI assistance
Additional Considerations
None.
Types of changes