Add timeout and tests for curb, also added the gem curb that was not part of selenium

[aguspe]

User description

Description

Based on #14121 we wanted to regain the ability to set timeout in our curb clients, to do this I added the attribute accessor timeout

Besides this tests were added and the gem curb was added as a dependency because it was not present in Selenium

Motivation and Context

It's important to provide users with different clients that they can set up and configure with the correct timeouts

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Added timeout attribute accessor to Curb HTTP client.
• Modified initialize method in Curb to accept timeout.
• Updated client method in Curb to set timeout if provided.
• Added curb gem as a dependency in the gemspec.
• Added unit tests for Curb HTTP client, including tests for default and custom timeout values.
• Added type signatures for Curl::Easy class and Curb class methods and attributes.

---

Changes walkthrough 📝

	Relevant files
Enhancement	remote.rb Add autoload for Curb in Http module                                          rb/lib/selenium/webdriver/remote.rb Added autoload for Curb in the Http module. +2/-1      curb.rb Add timeout handling to Curb HTTP client                                  rb/lib/selenium/webdriver/remote/http/curb.rb Added timeout attribute accessor. Modified initialize method to accept timeout. Updated client method to set timeout if provided. +14/-6    curl.rbs Add type signature for Curl::Easy class                                    rb/sig/gems/curb/curl.rbs Added type signature for Curl::Easy class. +5/-0      curb.rbs Add type signatures for Curb class                                              rb/sig/lib/selenium/webdriver/remote/http/curb.rbs Added type signatures for Curb class methods and attributes. +7/-3
Dependencies	selenium-webdriver.gemspec Add curb gem dependency                                                                    rb/selenium-webdriver.gemspec Added curb gem as a dependency. +1/-0
Tests	curb_spec.rb Add unit tests for Curb HTTP client                                            rb/spec/unit/selenium/webdriver/remote/http/curb_spec.rb Added unit tests for Curb HTTP client. Tested default and custom timeout values. +51/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[codiumai-pr-agent-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Inconsistent Initialization The initialize method in Curb class allows setting a timeout, but does not ensure it's a non-negative value. Consider adding a validation to ensure the timeout is not negative. Possible Bug The client method sets c.timeout only if timeout is provided. This might lead to unexpected behavior if timeout is nil or not set, as Curl::Easy might have its own default timeout that differs from the expected default of Curb.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Error handling	Implement error handling for Curl::Easy initialization and property settings Add error handling for the Curl::Easy initialization and setting properties to ensure that any exceptions raised by the Curl library are caught and handled appropriately. rb/lib/selenium/webdriver/remote/http/curb.rb [87-94] @client ||= begin - c = Curl::Easy.new - c.max_redirects = MAX_REDIRECTS - c.follow_location = true - c.timeout = timeout if timeout - c.verbose = WebDriver.logger.debug? - c + begin + c = Curl::Easy.new + c.max_redirects = MAX_REDIRECTS + c.follow_location = true + c.timeout = timeout if timeout + c.verbose = WebDriver.logger.debug? + c + rescue Curl::Err::CurlError => e + WebDriver.logger.error("Failed to initialize Curl::Easy: #{e.message}") + nil + end end Apply this suggestion Suggestion importance[1-10]: 10 Why: Adding error handling is crucial for robustness. This suggestion ensures that any exceptions raised during the initialization of Curl::Easy are caught and logged, preventing potential crashes and aiding in debugging.	10
Maintainability	Enhance the initialize method to accept additional keyword arguments for future compatibility **Ensure that the initialize method in the Curb class also accepts additional keyword arguments (kwargs) and passes them to super. This will maintain compatibility with the superclass Common if it is updated to accept additional initialization parameters. rb/lib/selenium/webdriver/remote/http/curb.rb [42-44] -def initialize(timeout: nil) +def initialize(timeout: nil, **kwargs) @timeout = timeout - super() + super(**kwargs) end Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion improves maintainability by ensuring that the initialize method remains compatible with potential future changes in the superclass Common. It is a forward-thinking enhancement that prevents possible issues with future updates.	8
	Refactor the creation of Curl::Easy into a separate method for better readability and maintainability Refactor the @client caching mechanism to separate the creation of a new Curl::Easy instance into a dedicated method. This improves code readability and maintainability. rb/lib/selenium/webdriver/remote/http/curb.rb [87-94] -@client ||= begin +def create_curl_easy c = Curl::Easy.new c.max_redirects = MAX_REDIRECTS c.follow_location = true c.timeout = timeout if timeout c.verbose = WebDriver.logger.debug? c end +@client ||= create_curl_easy + Apply this suggestion Suggestion importance[1-10]: 6 Why: This refactoring improves code readability and maintainability by separating concerns. While beneficial, it is a minor enhancement compared to error handling and forward compatibility improvements.	6
Enhancement	Set a default timeout value to ensure consistent behavior Consider setting a default value for the timeout attribute in the Curb class to ensure consistent behavior when the timeout is not explicitly set during initialization. rb/lib/selenium/webdriver/remote/http/curb.rb [42-44] -def initialize(timeout: nil) +def initialize(timeout: 30) # Default timeout set to 30 seconds @timeout = timeout super() end Apply this suggestion Suggestion importance[1-10]: 7 Why: Setting a default timeout value improves the consistency of the class behavior. However, it is less critical than error handling and forward compatibility, hence a slightly lower score.	7

[diemol]

Seems something is failing in the build process?
https://github.com/SeleniumHQ/selenium/actions/runs/10305049245/job/28524950266?pr=14285#step:15:2484

[aguspe]

Details

I will look into the failed-to-build tests tomorrow and also I will add the checksum update that I'm missing, thank you for executing the pipeline!

[aguspe]

@p0deje I ran bazel build @bundlebut I cannot find where the checksums of the gems are printed, do you know where should I be looking for them?

[p0deje]

@aguspe Can you run the following and share the output? Note that it's going to take a while.

bazel clean --expunge # clear all Bazel caches
bazel build @bundle

[aguspe]

bazel clean --expunge # clear all Bazel caches

Thank you so much @p0deje, that made the trick, the check sums are updated, so it's ready for re-review

[aguspe]

Thank you for the quick review @p0deje, ready for re-review

[aguspe]

@p0deje ready for re-review

[p0deje]

@p0deje ready for re-review

Could you look at #14285 (comment)?

[aguspe]

rb/lib/selenium/webdriver/remote/http/curb.rb

I added the required back @p0deje thank you for spotting that

[aguspe]

This Java test is failing, I can see that it is a new failure that was not there before, should I address it on this PR?

//java/test/org/openqa/selenium/bidi/input:DefaultKeyboardTest-chrome    FAILED in 2 out of 2 in 20.8s
  Stats over 2 runs: max = 20.8s, min = 20.1s, avg = 20.5s, dev = 0.3s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest-chrome/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest-chrome/test_attempts/attempt_1.log
//rb/spec/unit/selenium/webdriver/remote/http:curb                       FAILED in 2 out of 2 in 8.3s
  Stats over 2 runs: max = 8.3s, min = 7.9s, avg = 8.1s, dev = 0.2s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/rb/spec/unit/selenium/webdriver/remote/http/curb/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/rb/spec/unit/selenium/webdriver/remote/http/curb/test_attempts/attempt_1.log

[p0deje]

@aguspe You don't need to fix the Java test, but let's fix Ruby test.

[aguspe]

@aguspe You don't need to fix the Java test, but let's fix Ruby test.

So the ruby test is failing due to be running on Jruby:

An error occurred while loading ./rb/spec/unit/selenium/webdriver/remote/http/curb_spec.rb.Failure/Error: Unable to find org/jruby/RubyKernel.java to read failed lineLoadError:  no such file to load -- curb# ./rb/lib/selenium/webdriver/remote/http/curb.rb:20:in `<main>'# ./rb/spec/unit/selenium/webdriver/remote/http/curb_spec.rb:21:in `<main>'All examples were filtered out; ignoring {}No examples found.Finished in 0.00033 seconds (files took 1.98 seconds to load)0 examples, 0 failures, 1 error occurred outside of examples

So I just added an unless for Jruby since in windows and linux now we have curl installed @p0deje

[aguspe]

I tried with Ruby engine locally, and the test gets ignore, is odd that it failed at the same point with Ruby Platform @p0deje

[p0deje]

I tried with Ruby engine locally, and the test gets ignore, is odd that it failed at the same point with Ruby Platform @p0deje

Looks like you need to guard against TruffleRuby as well. Can you try the following in spec:

require File.expand_path('../../spec_helper', __dir__)
return if Selenium::WebDriver::Platform.jruby? || Selenium::WebDriver::Platform.truffleruby?

[aguspe]

require File.expand_path('../../spec_helper', __dir__)
return if Selenium::WebDriver::Platform.jruby? || Selenium::WebDriver::Platform.truffleruby?

Updated to support truffle ruby

[aguspe]

Now the only test failing is the java one:

//java/test/org/openqa/selenium/bidi/input:DefaultKeyboardTest-chrome    FAILED in 2 out of 2 in 19.7s
  Stats over 2 runs: max = 19.7s, min = 19.2s, avg = 19.4s, dev = 0.3s
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest-chrome/test.log
  /home/runner/.bazel/execroot/_main/bazel-out/k8-fastbuild/testlogs/java/test/org/openqa/selenium/bidi/input/DefaultKeyboardTest-chrome/test_attempts/attempt_1.log

Executed 2 out of 2076 tests: 2075 tests pass and 1 fails remotely.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

[aguspe]

Thanks for the help @p0deje !

[p0deje]

@aguspe Thank you for working it out!