-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[rb] allow setting socket timeout and interval from http client #16472
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
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
|
That would be better, but I don't have it in my head how we'd implement it. It would be a breaking change. |
5173a9f to
e1a06da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have those options separate from HTTP because they really have nothing to do with HTTP clients. We can add a new class ClientConfig and allow to pass it to the Driver constructor. This won't be a breaking change and spare us from deprecating these fields in HTTP in the future.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
|
I thought the plan was to remove the ability to pass in a custom http client entirely, in which case it wouldn't matter what we added since it would all end up private api? Trying to figure out what the middle ground is to get what I want without needing to do all of 16269 |
|
Converting to draft until after #16486 |
|
actually closing this; I will push something new that uses the client config |
User description
🔗 Related Issues
fixes #15620
💥 What does this PR do?
Allows setting socket timeout and interval values on the default http client.
🔧 Implementation Notes
This is an alternative to #16053
I'm not sure "default http config" is the perfect place for this, kind of wish we just had a config file like others, but the superclass gets the http client instance, so easy enough to grab it and send it to the ws socket class.
💡 Additional Considerations
🔄 Types of changes
PR Type
Enhancement
Description
Allow configuring socket timeout and interval via HTTP client
Pass HTTP client instance to BiDi and WebSocket connections
Replace hardcoded timeout constants with configurable values
Add socket timeout parameter to test environment setup
Diagram Walkthrough
File Walkthrough
bidi.rb
Add HTTP client parameter to BiDi initializationrb/lib/selenium/webdriver/bidi.rb
initializemethod to accepthttpparameterhttpparameter toWebSocketConnectionconstructorwebsocket_connection.rb
Make socket timeout and interval configurablerb/lib/selenium/webdriver/common/websocket_connection.rb
initializeto accept optionalhttpparametersocket_timeoutandsocket_intervalfrom HTTP client withdefaults
RESPONSE_WAIT_TIMEOUTandRESPONSE_WAIT_INTERVALconstants with instance variables
waitmethod to use configurable timeout and interval valuesbidi_bridge.rb
Pass HTTP client to BiDi initializationrb/lib/selenium/webdriver/remote/bidi_bridge.rb
@httpclient instance toBiDi.newconstructordefault.rb
Add socket timeout and interval configurationrb/lib/selenium/webdriver/remote/http/default.rb
socket_timeoutandsocket_intervalas accessor attributessocket_timeoutandsocket_intervalparameters toinitializemethodwith defaults (30 and 0.1)
test_environment.rb
Enable BiDi and add socket timeout test supportrb/spec/integration/selenium/webdriver/spec_support/test_environment.rb
ENV['WEBDRIVER_BIDI']create_driver!to accept and handlesocket_timeoutoptionhttp_clientparameter to driver creation methodsdefault_spec.rb
Add socket timeout and interval test coveragerb/spec/unit/selenium/webdriver/remote/http/default_spec.rb
socket_timeout(30) andsocket_interval(0.1)
initialization
websocket_connection.rbs
Add socket timeout and interval type signaturesrb/sig/lib/selenium/webdriver/common/websocket_connection.rbs
@socket_interval(float) and@socket_timeout(int) instance variables
default.rbs
Add socket timeout and interval type signaturesrb/sig/lib/selenium/webdriver/remote/http/default.rbs
socket_timeout(int) andsocket_interval(float) accessors