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 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
Implements add and remove methods for websocket callers (there is definitely a better way to do this)
Deprecates BiDi#callbacks (is there another reason to keep this method?)
I think this is probably a replacement for LogInspector (which is not the high level API we want, and I don't think we need an intermediary implementation between the socket calls and the high level API)
Unlike LogInspector, this implementation doesn't do any type enforcement or structs or classes; return values are plain JSON. Do we want to add more structure and/or typing from that implementation into this?
using #object_id of the proc to return the id instead of tracking it separately
updated browser context implementation to avoid specifying browser context ids if they don't apply (which they don't for console logs)
Script class is going to implement things from both script and log domains. It makes sense to implement the domain specific functionality in separate, lower level classes and keep Script for the high level API, so I created LogHandler class for this.
Created a Struct subclass that will convert camel case JSON keys into snake case methods; using these Structs provides the user with a more straightforward access of declared parameters, without needing to maintain a lot of classes. If we dynamically generate classes in the future, it likely won't change the user's code
Deprecated Driver#script alias of Driver#exectue_script
Deprecated LogInspector - this should provide all of the required functionality of LogInspector
PR Type
Enhancement, Tests, Documentation
Description
Introduced LogHandler class for managing console and JavaScript log entries.
Deprecated BiDi#callbacks and LogInspector class.
Added add_callback and remove_callback methods to WebSocketConnection.
Created Script class to handle log entries and manage handlers.
Refactored subscribe and unsubscribe methods in Session class to use keyword arguments.
Added integration tests for Script class and BiDi error handling.
Updated type signatures for Driver and Script classes.
Changes walkthrough 📝
Relevant files
Enhancement
9 files
bidi.rb
Enhance BiDi with logging handlers and callback management
rb/lib/selenium/webdriver/bidi.rb
Added LogHandler and Struct autoloads.
Deprecated BiDi#callbacks method.
Introduced add_callback and remove_callback methods.
4, because the PR introduces significant changes across multiple files, including new classes and methods, deprecations, and modifications to existing functionalities. The complexity of the changes, especially around the new logging API and handling of callbacks, requires careful consideration to ensure compatibility and correctness.
🧪 Relevant tests
Yes
⚡ Possible issues
Possible Bug: The remove_message_handler method in LogHandler class might not properly handle the case where the id does not exist in the @callbacks hash, potentially leading to errors when trying to delete a non-existent entry.
🔒 Security concerns
No
Code feedback:
relevant file
rb/lib/selenium/webdriver/bidi/log_handler.rb
suggestion
Consider adding error handling in remove_message_handler to gracefully handle cases where the id provided does not exist in the @callbacks hash. This can prevent potential errors and improve the robustness of the method. [important]
Fix the remove_callback method to correctly use both event and id to fetch and remove the callback
The remove_callback method currently retrieves the callback using the id directly from @track_callbacks, which is incorrect. It should use the event and id to fetch the callback.
Why: The suggestion correctly identifies and fixes a bug in the method implementation, ensuring the callback is properly accessed and removed using both event and id.
10
Consistency
Ensure add_console_message_handler and add_javascript_error_handler return the handler ID for consistency
The add_console_message_handler and add_javascript_error_handler methods should return the handler ID to be consistent with the add_callback method.
+def add_console_message_handler+ subscribe_log_entry unless @log_entry_subscribed+ @bidi.add_callback('log.entryAdded') do |params|+ yield(params) if params['type'] == 'console'+ end+end+def add_javascript_error_handler+ subscribe_log_entry unless @log_entry_subscribed+ @bidi.add_callback('log.entryAdded') do |params|+ yield(params) if params['type'] == 'javascript'+ end+end
Suggestion importance[1-10]: 8
Why: This suggestion correctly identifies a consistency issue with the return values of handler methods, aligning them with similar methods. This is important for maintaining consistent API behavior.
8
Enhancement
Simplify the script method by using a ternary operator for better readability
The script method can be simplified by using a ternary operator to check if args is empty and return the appropriate result.
def script(*args) - if args.any?- execute_script(*args)- else- @script ||= WebDriver::Script.new(bridge)- end+ args.any? ? execute_script(*args) : (@script ||= WebDriver::Script.new(bridge)) end
Suggestion importance[1-10]: 6
Why: The suggestion improves readability by simplifying the method with a ternary operator. However, it's a minor enhancement rather than a critical change.
6
Maintainability
Correct the typo in the test method name for better readability
There is a typo in the test method name logs removes console message hander. It should be corrected to logs removes console message handler.
Failed test name: //rb/spec/integration/selenium/webdriver:devtools-firefox
Failure summary:
The action failed due to the following reasons:
The protoc.exe command failed to execute successfully, causing a critical build step to fail.
Specifically, the error occurred while generating the proto_library for the target @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto.
The failure of this build step resulted in the overall build not completing successfully.
Consequently, the test //rb/spec/integration/selenium/webdriver:devtools-firefox failed to build, leading to the failure of the action.
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.
(2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the /ask tool, you need to comment on a PR: /ask "<question content>". See the relevant documentation for each tool for more details.
Ensure add_message_handler returns the ID of the callback handler it adds
The add_message_handler method should return the ID of the callback handler it adds. This will make it consistent with the WebSocketConnection#add_callback method and allow the caller to remove the handler later.
+def add_message_handler(type)+ subscribe_log_entry unless @log_entry_subscribed+ @bidi.add_callback('log.entryAdded') do |params|+ if params['type'] == type+ log_entry_klass = type == 'console' ? ConsoleLogEntry : JavaScriptLogEntry+ yield(log_entry_klass.new(**params))+ end+ end+end-
Suggestion importance[1-10]: 10
Why: The suggestion correctly identifies a functional inconsistency and proposes a change that aligns with existing patterns in the codebase, enhancing usability and consistency.
10
Possible issue
Add a check to ensure the handler ID exists before attempting to remove it
Add a check in the remove_message_handler method to ensure the handler ID exists before attempting to remove it. This will prevent potential errors if an invalid ID is passed.
def remove_message_handler(id) - @bidi.remove_callback('log.entryAdded', id)- unsubscribe_log_entry if @bidi.callbacks['log.entryAdded'].empty?+ if @bidi.callbacks['log.entryAdded'].any? { |callback| callback.object_id == id }+ @bidi.remove_callback('log.entryAdded', id)+ unsubscribe_log_entry if @bidi.callbacks['log.entryAdded'].empty?+ else+ raise ArgumentError, "Handler ID not found"+ end end
Suggestion importance[1-10]: 8
Why: Adding a check for the existence of a handler ID before removal is crucial to prevent runtime errors and ensures robustness in method execution.
8
Best practice
Convert camel_to_snake to a class method for better reusability
The camel_to_snake method should be defined as a class method since it does not depend on instance-specific data. This will make it more reusable and consistent with its usage.
-WebDriver.logger.deprecate('`BiDi#callbacks << event` to add a callback to the list of callbacks',- 'BiDi#add_callback(event, &block)', id: :callbacks)+log_deprecation_warning+private++def log_deprecation_warning+ WebDriver.logger.deprecate('`BiDi#callbacks << event` to add a callback to the list of callbacks',+ 'BiDi#add_callback(event, &block)', id: :callbacks)+end+
Suggestion importance[1-10]: 6
Why: Extracting the deprecation logic into a separate method improves the readability and maintainability of the code, although it's not a critical change.
Return the handler ID from the add_message_handler method to allow users to remove the handler later
The add_message_handler method should return the handler ID to allow users to remove the handler later. This will make the method consistent with remove_message_handler which requires an ID.
def remove_callback(event, id) - return if callbacks[event].reject! { |callback| callback.object_id == id }+ removed = callbacks[event].reject! { |callback| callback.object_id == id }+ return true if removed ids = callbacks[event]&.map(&:object_id) raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}" end
Suggestion importance[1-10]: 7
Why: This suggestion enhances the method by providing clear feedback on the operation's success, which is useful for error handling.
7
Possible bug
Handle the case where the event does not exist in the callbacks hash in the remove_message_handler method
The remove_message_handler method should handle the case where the event does not exist in the callbacks hash to avoid potential errors.
def remove_message_handler(id) + return unless @bidi.callbacks.key?('log.entryAdded') @bidi.remove_callback('log.entryAdded', id) unsubscribe_log_entry if @log_entry_subscribed && @bidi.callbacks['log.entryAdded'].empty? end
Suggestion importance[1-10]: 7
Why: This suggestion correctly identifies a potential error scenario and improves robustness by handling missing events.
7
Best practice
Define camel_to_snake as a class method since it does not depend on instance state
The camel_to_snake method should be defined as a class method since it does not depend on instance state and can be reused without instantiating the class.
* add and remove logging handlers with BiDi * use #object_id instead of creating new ids to track callbacks * do not send browsing contexts to subscription if not needed * deprecate Driver#script & LogInspector * error if trying to remove an id that does not exist * do not unsubscribe if never subscribed in the first place * do not deprecate callbacks people don't have to care about getting the id back
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.
titusfortner added C-rb 
qodo-code-review bot added Review effort [1-5]: 4 and removed Review effort [1-5]: 3 labels 
User description
Description
Motivation and Context
Partial Ruby implementation of #13992
#13964
Updates
Based on feedback:
#object_idof the proc to return the id instead of tracking it separatelyScriptclass is going to implement things from both script and log domains. It makes sense to implement the domain specific functionality in separate, lower level classes and keepScriptfor the high level API, so I createdLogHandlerclass for this.Driver#scriptalias ofDriver#exectue_scriptLogInspector- this should provide all of the required functionality of LogInspectorPR Type
Enhancement, Tests, Documentation
Description
LogHandlerclass for managing console and JavaScript log entries.BiDi#callbacksandLogInspectorclass.add_callbackandremove_callbackmethods toWebSocketConnection.Scriptclass to handle log entries and manage handlers.subscribeandunsubscribemethods inSessionclass to use keyword arguments.Scriptclass and BiDi error handling.DriverandScriptclasses.Changes walkthrough 📝
9 files
bidi.rb
Enhance BiDi with logging handlers and callback managementrb/lib/selenium/webdriver/bidi.rb
LogHandlerandStructautoloads.BiDi#callbacksmethod.add_callbackandremove_callbackmethods.log_handler.rb
Introduce LogHandler class for managing log entriesrb/lib/selenium/webdriver/bidi/log_handler.rb
LogHandlerclass for managing log entries.log_inspector.rb
Deprecate LogInspector and update callback handlingrb/lib/selenium/webdriver/bidi/log_inspector.rb
LogInspectorclass.add_callbackinstead of direct callbackmanipulation.
session.rb
Refactor session subscription methods with keyword argumentsrb/lib/selenium/webdriver/bidi/session.rb
subscribeandunsubscribemethods to use keyword arguments.struct.rb
Add Struct class for camel to snake case conversionrb/lib/selenium/webdriver/bidi/struct.rb
Structclass to convert camel case JSON keys to snake casemethods.
common.rb
Require Script class in common modulerb/lib/selenium/webdriver/common.rb
require 'selenium/webdriver/common/script'.driver.rb
Deprecate Driver#script alias and add Script instance methodrb/lib/selenium/webdriver/common/driver.rb
Driver#scriptalias for#execute_script.Driver#scriptmethod for accessingScriptinstance.script.rb
Add Script class for handling log entriesrb/lib/selenium/webdriver/common/script.rb
Scriptclass to handle console and JavaScript log entries.websocket_connection.rb
Add callback management methods to WebSocketConnectionrb/lib/selenium/webdriver/common/websocket_connection.rb
add_callbackandremove_callbackmethods for managing WebSocketcallbacks.
3 files
script_spec.rb
Add integration tests for Script classrb/spec/integration/selenium/webdriver/bidi/script_spec.rb
Scriptclass.bidi_spec.rb
Add BiDi error handling and deprecation testsrb/spec/integration/selenium/webdriver/bidi_spec.rb
BiDi#callbacks.driver_spec.rb
Add test for Driver#script method with deprecation warningrb/spec/integration/selenium/webdriver/driver_spec.rb
Driver#scriptmethod with deprecation warning.2 files
driver.rbs
Update type signature for Driver classrb/sig/lib/selenium/webdriver/common/driver.rbs
@scriptinstance variable to type signature.script.rbs
Add type signatures for Script classrb/sig/selenium/web_driver/script.rbs
Scriptclass methods.