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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from
Open

[rb] make bidi timeout configurable #16053

wants to merge 18 commits into from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jul 14, 2025 •
edited
Loading

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

We need to make a decision if we want to go with this solution or try an alternative since the BiDi specification doesn't have timeouts yet

🔄 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
@qodo-merge-pro Qodo Merge Pro
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

@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

qodo-merge-pro bot commented Jul 19, 2025 •
edited
Loading

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

@aguspe aguspe changed the title Rb make bidi timeout configurable [rb] make bidi timeout configurable Aug 2, 2025
@diemol diemol added the A-needs decision TLC needs to discuss and agree label Aug 5, 2025
This was referenced Aug 28, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aguspe aguspe

Labels
A-needs decision TLC needs to discuss and agree C-rb Ruby Bindings Review effort 3/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [rb] BiDi page loads longer than 30 seconds raise Selenium::WebDriver::Error::TimeoutError
4 participants
@aguspe @selenium-ci @diemol @titusfortner