Skip to content

Rb make bidi timeout configurable #16053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: trunk
Choose a base branch
from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jul 14, 2025

User description

🔗 Related Issues

This aims to fix #15620 until a timeout implementation for BiDi happens

💥 What does this PR do?

This PR adds support for changing the WebSocket timeout through options.

🔧 Implementation Notes

For now, the implementation is temporary, and I'm trying to find the best way to make it work. I will update this before finishing this draft

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Enhancement


Description

  • Add configurable WebSocket timeout options for BiDi connections

  • Support web_socket_timeout and web_socket_interval parameters

  • Pass timeout configuration through options chain

  • Add integration test for timeout behavior


Diagram Walkthrough

flowchart LR
  Options["Options Class"] -- "adds timeout options" --> BiDiBridge["BiDi Bridge"]
  BiDiBridge -- "passes ws_options" --> BiDi["BiDi Class"]
  BiDi -- "forwards options" --> WebSocket["WebSocket Connection"]
  WebSocket -- "configures timeout" --> Wait["Wait Object"]
Loading

File Walkthrough

Relevant files

@selenium-ci selenium-ci added the C-rb Ruby Bindings label Jul 14, 2025
@aguspe aguspe self-assigned this Jul 19, 2025
@aguspe aguspe marked this pull request as ready for review July 19, 2025 15:42
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Naming Inconsistency

The capability keys use 'ws:responseTimeout' and 'ws:responseInterval' but the options hash uses 'response_timeout' and 'response_interval'. This inconsistency could lead to configuration not being passed correctly.

ws_options = {
  response_timeout: capabilities['ws:responseTimeout'],
  response_interval: capabilities['ws:responseInterval']
}.compact
Key Mismatch

The WebSocket options are mapped to 'ws:response_timeout' and 'ws:response_interval' in as_json method, but BiDiBridge expects 'ws:responseTimeout' and 'ws:responseInterval' with different casing.

options['ws:response_timeout'] = options.delete(:web_socket_timeout) if options[:web_socket_timeout]
options['ws:response_interval'] = options.delete(:web_socket_interval) if options[:web_socket_interval]
Test Logic Issue

The test sets web_socket_interval to 1 second but web_socket_timeout to 0.1 seconds, which means the timeout will occur before the first interval check. This may not properly test the timeout functionality.

reset_driver!(web_socket_url: true, web_socket_timeout: 0.1, web_socket_interval: 1) do |driver|
  expect {
    driver.navigate.to url_for('sleep?time=0.2')
  }.to raise_error(Selenium::WebDriver::Error::TimeoutError)
end

Copy link
Contributor

qodo-merge-pro bot commented Jul 19, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix capability key mismatch

The capability keys should match the ones defined in the options mapping. The
keys should be 'ws:response_timeout' and 'ws:response_interval' to match the
mapping in as_json method.

rb/lib/selenium/webdriver/remote/bidi_bridge.rb [30-33]

 ws_options = {
-  response_timeout: capabilities['ws:responseTimeout'],
-  response_interval: capabilities['ws:responseInterval']
+  response_timeout: capabilities['ws:response_timeout'],
+  response_interval: capabilities['ws:response_interval']
 }.compact
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a bug where the capability keys being read (ws:responseTimeout) do not match the keys being set in options.rb (ws:response_timeout), which would cause the new timeout feature to fail.

High
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [rb] BiDi page loads longer than 30 seconds raise Selenium::WebDriver::Error::TimeoutError
3 participants