You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This matches what is being done in other languages (#13991)
In general it isn't user friendly that that web_socket_url value to the driver is a boolean, but the response is a string. This provides a more obvious name for users to set the property.
This allows Options class to use #enable_bidi! and #bidi? as the booleans for the options.
Capabilities will return driver.capabilities[:web_socket_url] with either nil or the value of the url.
We ok with my use of bang and predicate here?
PR Type
Enhancement, Tests
Description
Added enable_bidi! method to Selenium::WebDriver::Options to set web_socket_url to true.
Added bidi? method to check if web_socket_url is enabled.
Added integration test for enabling bidi in Chrome options, verifying web_socket_url and bidi? method behavior.
Added unit test for enable_bidi! method, verifying setting and querying of bidi option.
Changes walkthrough 📝
Relevant files
Enhancement
options.rb
Add methods to enable and check bidi in options
rb/lib/selenium/webdriver/common/options.rb
Added enable_bidi! method to set web_socket_url to true.
Added bidi? method to check if web_socket_url is enabled.
Code Clarity The use of the bang (!) in enable_bidi! suggests that the method could raise an exception or has a dangerous side effect, which does not seem to be the case here. Consider renaming to enable_bidi if there are no such implications.
Redundant Code The double negation in !!@options[:web_socket_url] is redundant. Ruby implicitly treats nil as false in boolean contexts, so @options[:web_socket_url] is sufficient.
Update the enable_bidi! method to accept and set a URL instead of a boolean
The method enable_bidi! sets @options[:web_socket_url] to true, which seems incorrect as it should likely store a WebSocket URL string instead of a boolean. Consider updating this method to accept a URL parameter or fetch it from a configuration.
-def enable_bidi!- @options[:web_socket_url] = true+def enable_bidi!(url)+ @options[:web_socket_url] = url
end
Apply this suggestion
Suggestion importance[1-10]: 9
Why: This suggestion addresses a potential bug by ensuring that the enable_bidi! method sets a WebSocket URL instead of a boolean, which aligns with the expected behavior of handling a URL.
9
Enhancement
Modify the bidi? method to correctly handle URL strings
The bidi? method uses a double negation (!!) to convert the value to a boolean. This is unnecessary if @options[:web_socket_url] is always a boolean. If @options[:web_socket_url] is intended to be a URL string, adjust the method to check for nil or an empty string instead.
def bidi?
- !!@options[:web_socket_url]+ !@options[:web_socket_url].nil? && !@options[:web_socket_url].empty?
end
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This enhancement ensures that the bidi? method correctly checks for the presence of a WebSocket URL, improving the robustness of the code.
8
Possible issue
Update the test to check for a valid WebSocket URL format
The test case for enable_bidi! assumes web_socket_url to be a boolean, which contradicts the expected behavior of handling a URL. Update the test to check for a valid URL format instead of a boolean.
-expect(options.web_socket_url).to be true+expect(options.web_socket_url).to match(/\Aws:\/\/.+/)
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion improves the test case by ensuring it checks for a valid WebSocket URL format, which aligns with the expected behavior of the enable_bidi! method.
8
Adjust the unit test to validate that web_socket_url is a properly formatted URL
Similar to the integration test, the unit test for enable_bidi! should verify that web_socket_url is set to a valid URL, not just a boolean. Adjust the expectation to validate the format of the URL.
-expect(options.web_socket_url).to be true+expect(options.web_socket_url).to match(/\Aws:\/\/.+/)
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion enhances the unit test by verifying that web_socket_url is a valid URL, ensuring consistency with the expected behavior of the enable_bidi! method.
Failed test name: Selenium::WebDriver::Edge::Options enables bidi
Failure summary:
The action failed due to two specific test failures in the Selenium WebDriver integration tests for Edge:
The test Selenium::WebDriver::Edge::Options enables bidi failed because it attempted to load a non-existent file selenium/webdriver/chrome, which resulted in a LoadError. The correct file might be selenium/webdriver/chromium.
The test Selenium::WebDriver::Edge::Options enables BiDi on initialization also failed due to a LoadError when trying to initialize the WebDriver for Edge with certain options.
The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:
Failed stage
Failed test name
Failure summary
Relevant error logs
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.
Configuration options
enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.
See more information about the checks tool in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
This matches what is being done in other languages (#13991)
In general it isn't user friendly that that
web_socket_urlvalue to the driver is a boolean, but the response is a string. This provides a more obvious name for users to set the property.This allows Options class to use
#enable_bidi!and#bidi?as the booleans for the options.Capabilities will return
driver.capabilities[:web_socket_url]with either nil or the value of the url.We ok with my use of bang and predicate here?
PR Type
Enhancement, Tests
Description
enable_bidi!method toSelenium::WebDriver::Optionsto setweb_socket_urlto true.bidi?method to check ifweb_socket_urlis enabled.web_socket_urlandbidi?method behavior.enable_bidi!method, verifying setting and querying of bidi option.Changes walkthrough 📝
options.rb
Add methods to enable and check bidi in optionsrb/lib/selenium/webdriver/common/options.rb
enable_bidi!method to setweb_socket_urlto true.bidi?method to check ifweb_socket_urlis enabled.options_spec.rb
Add integration test for enabling bidi in Chrome optionsrb/spec/integration/selenium/webdriver/chrome/options_spec.rb
web_socket_urlandbidi?method behavior.options_spec.rb
Add unit test for enable_bidi! method in Chrome optionsrb/spec/unit/selenium/webdriver/chrome/options_spec.rb
enable_bidi!method.