[rb] Implement High Level Logging API with BiDi

[titusfortner]

User description

Description

• 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?

Motivation and Context

Partial Ruby implementation of #13992
#13964

Updates

Based on feedback:

• 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. +14/-0    log_handler.rb Introduce LogHandler class for managing log entries            rb/lib/selenium/webdriver/bidi/log_handler.rb Created LogHandler class for managing log entries. Added methods to handle console and JavaScript log entries. Implemented subscription management for log entries. +63/-0    log_inspector.rb Deprecate LogInspector and update callback handling            rb/lib/selenium/webdriver/bidi/log_inspector.rb Deprecated LogInspector class. Updated method to use add_callback instead of direct callback manipulation. +5/-1      session.rb Refactor session subscription methods with keyword arguments rb/lib/selenium/webdriver/bidi/session.rb Refactored subscribe and unsubscribe methods to use keyword arguments. +6/-6      struct.rb Add Struct class for camel to snake case conversion            rb/lib/selenium/webdriver/bidi/struct.rb Created Struct class to convert camel case JSON keys to snake case methods. +40/-0    common.rb Require Script class in common module                                        rb/lib/selenium/webdriver/common.rb Added require 'selenium/webdriver/common/script'. +1/-0      driver.rb Deprecate Driver#script alias and add Script instance method rb/lib/selenium/webdriver/common/driver.rb Deprecated Driver#script alias for #execute_script. Introduced Driver#script method for accessing Script instance. +16/-6    script.rb Add Script class for handling log entries                                rb/lib/selenium/webdriver/common/script.rb Created Script class to handle console and JavaScript log entries. +45/-0    websocket_connection.rb Add callback management methods to WebSocketConnection      rb/lib/selenium/webdriver/common/websocket_connection.rb Added add_callback and remove_callback methods for managing WebSocket callbacks. +9/-0
Tests	3 files script_spec.rb Add integration tests for Script class                                      rb/spec/integration/selenium/webdriver/bidi/script_spec.rb Added integration tests for Script class. Tested console message and JavaScript error handling. +109/-0  bidi_spec.rb Add BiDi error handling and deprecation tests                        rb/spec/integration/selenium/webdriver/bidi_spec.rb Added test for BiDi error handling when not enabled. Added deprecation test for BiDi#callbacks. +10/-23  driver_spec.rb Add test for Driver#script method with deprecation warning rb/spec/integration/selenium/webdriver/driver_spec.rb Added test for Driver#script method with deprecation warning. +10/-1
Documentation	2 files driver.rbs Update type signature for Driver class                                      rb/sig/lib/selenium/webdriver/common/driver.rbs Added @script instance variable to type signature. +1/-0      script.rbs Add type signatures for Script class                                          rb/sig/selenium/web_driver/script.rbs Added type signatures for Script class methods. +20/-0

---

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

[qodo-merge-pro]

PR Review 🔍

(Review updated until commit aeb7b85)

⏱️ Estimated effort to review [1-5]	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]
relevant line	@bidi.remove_callback('log.entryAdded', id)

---

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	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. rb/lib/selenium/webdriver/bidi.rb [64-67] def remove_callback(event, id) - call = @track_callbacks[id] + call = @track_callbacks[event][id] @callbacks[event].delete(call) + @track_callbacks[event].delete(id) end Suggestion importance[1-10]: 10 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. rb/lib/selenium/webdriver/common/script.rb [28-42] +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. rb/lib/selenium/webdriver/common/driver.rb [107-113] 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. rb/spec/integration/selenium/webdriver/bidi/script_spec.rb [67] -it 'logs removes console message hander' do +it 'logs removes console message handler' do Suggestion importance[1-10]: 4 Why: Correcting typos improves code maintainability and readability, but it's a relatively minor issue.	4

[qodo-merge-pro]

CI Failure Feedback 🧐

(Checks updated until commit 4252b00)

Action: Ruby / Local Tests (firefox, windows) / Local Tests (firefox, windows)

Failed stage: Run Bazel [❌]

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.

Relevant error logs: 1: ##[group]Operating System 2: Microsoft Windows Server 2022 ... 353: ##[group]Restore cache for external-protobuf~ 354: Cache Size: ~7 MB (6846456 B) 355: [command]"C:\Program Files\Git\usr\bin\tar.exe" -xf D:/a/_temp/0fb72dd8-d190-43e5-b123-7d8f3e791412/cache.tzst -P -C D:/a/selenium/selenium --force-local --use-compress-program "zstd -d" 356: Received 5073488 of 5073488 (100.0%), 4.8 MBs/sec 357: Cache restored successfully 358: Successfully restored cache from setup-bazel-2-win32-external-protobuf~-9914fdedced4b237d23a421264c215c7ce93f92c099c523923a35f2748b6e88d 359: ##[endgroup] 360: ##[group]Restore cache for external-rules_buf~~ext~rules_buf_toolchains 361: Failed to restore external-rules_buf~~ext~rules_buf_toolchains cache ... 664: �[32m[1,066 / 1,864]�[0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 5s local, disk-cache ... (4 actions, 3 running) 665: �[32m[1,074 / 1,864]�[0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 6s local, disk-cache ... (2 actions, 1 running) 666: �[32m[1,076 / 1,864]�[0m [Prepa] RunBinary rb/lib/selenium/devtools/v124.rb ... (4 actions, 1 running) 667: �[32mINFO: �[0mFrom Installing external/rules_ruby~~ruby~bundle/rb/vendor/cache/bundler-2.5.10.gem (@@rules_ruby~~ruby~bundle//:bundler-2.5.10): 668: Successfully installed bundler-2.5.10 669: 1 gem installed 670: �[32m[1,077 / 1,864]�[0m Linking external/protobuf~/protoc.exe [for tool]; 1s local, disk-cache ... (4 actions, 1 running) 671: �[32m[1,078 / 1,864]�[0m RunBinary rb/lib/selenium/devtools/v85.rb; 1s disk-cache ... (4 actions, 2 running) 672: �[31m�[1mERROR: �[0mD:/_bazel/external/io_bazel_rules_closure/java/io/bazel/rules/closure/BUILD:64:14: Generating proto_library @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto [for tool] failed: (Exit -1073741819): protoc.exe failed: error executing GenProto command (from target @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto) 673: cd /d D:/_bazel/execroot/_main 674: SET PATH=C:\Program Files\Git\bin;C:\Program Files\Git\usr\bin;C:\Windows;C:\Windows\System32;C:\Windows\System32\WindowsPowerShell\v1.0 675: bazel-out\x64_windows-opt-exec-ST-13d3ddad9198\bin\external\protobuf~\protoc.exe --java_out=bazel-out/x64_windows-opt-exec-ST-13d3ddad9198/bin/external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info_proto-speed-src.jar -Iexternal/io_bazel_rules_closure -I. external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info.proto 676: # Configuration: 3f62d58e1724b33c4c5a838ec5dd37be29e6f58351fe7900d780cb5e8258345c 677: # Execution platform: @@local_config_platform//:host 678: �[32m[1,082 / 1,864]�[0m 1 / 27 tests, �[31m�[1m1 failed�[0m;�[0m checking cached actions 679: �[32mINFO: �[0mElapsed time: 266.804s, Critical Path: 16.05s 680: �[32mINFO: �[0m1082 processes: 33 disk cache hit, 872 internal, 177 local. 681: �[31m�[1mERROR: �[0mBuild did NOT complete successfully 682: //rb/spec/integration/selenium/webdriver:action_builder-firefox �[0m�[31m�[1mNO STATUS�[0m 683: //rb/spec/integration/selenium/webdriver:bidi-firefox �[0m�[31m�[1mNO STATUS�[0m 684: //rb/spec/integration/selenium/webdriver:driver-firefox �[0m�[31m�[1mNO STATUS�[0m 685: //rb/spec/integration/selenium/webdriver:element-firefox �[0m�[31m�[1mNO STATUS�[0m 686: //rb/spec/integration/selenium/webdriver:error-firefox �[0m�[31m�[1mNO STATUS�[0m ... 700: //rb/spec/integration/selenium/webdriver/bidi:browsing_context-firefox �[0m�[31m�[1mNO STATUS�[0m 701: //rb/spec/integration/selenium/webdriver/bidi:log_inspector-firefox �[0m�[31m�[1mNO STATUS�[0m 702: //rb/spec/integration/selenium/webdriver/bidi:script-firefox �[0m�[31m�[1mNO STATUS�[0m 703: //rb/spec/integration/selenium/webdriver/firefox:driver-firefox �[0m�[31m�[1mNO STATUS�[0m 704: //rb/spec/integration/selenium/webdriver/firefox:profile-firefox �[0m�[31m�[1mNO STATUS�[0m 705: //rb/spec/integration/selenium/webdriver/firefox:service-firefox �[0m�[31m�[1mNO STATUS�[0m 706: //rb/spec/integration/selenium/webdriver/remote:driver-firefox �[0m�[31m�[1mNO STATUS�[0m 707: //rb/spec/integration/selenium/webdriver/remote:element-firefox �[0m�[31m�[1mNO STATUS�[0m 708: //rb/spec/integration/selenium/webdriver:devtools-firefox �[0m�[31m�[1mFAILED TO BUILD�[0m 709: Executed 0 out of 27 tests: �[0m�[31m�[1m1 fails to build�[0m and �[0m�[35m26 were skipped�[0m. 710: �[0m 711: ##[error]Process completed with exit code 1.

---

✨ CI feedback usage guide:

---

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:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

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.

[titusfortner]

/help

[qodo-merge-pro]

PR Agent Walkthrough 🤖

Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more.

Here is a list of tools you can use to interact with the PR Agent:

Tool	Description	Trigger Interactively 💎
DESCRIBE	Generates PR description - title, type, summary, code walkthrough and labels	Run
REVIEW	Adjustable feedback about the PR, possible issues, security concerns, review effort and more	Run
IMPROVE	Code suggestions for improving the PR	Run
UPDATE CHANGELOG	Automatically updates the changelog	Run
ADD DOCS 💎	Generates documentation to methods/functions/classes that changed in the PR	Run
TEST 💎	Generates unit tests for a specific component, based on the PR code change	Run
IMPROVE COMPONENT 💎	Code suggestions for a specific component that changed in the PR	Run
ANALYZE 💎	Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component	Run
ASK	Answering free-text questions about the PR	[*]
GENERATE CUSTOM LABELS 💎	Generates custom labels for the PR, based on specific guidelines defined by the user	[*]
CI FEEDBACK 💎	Generates feedback and analysis for a failed CI job	[*]
CUSTOM PROMPT 💎	Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user	[*]
SIMILAR ISSUE	Automatically retrieves and presents similar issues	[*]

(1) Note that each tool be triggered automatically when a new PR is opened, or called manually by commenting on a PR.

(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.

[qodo-merge-pro]

Persistent review updated to latest commit aeb7b85

[qodo-merge-pro]

PR Description updated to latest commit (aeb7b85)

• Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	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. rb/lib/selenium/webdriver/bidi/log_handler.rb [33-41] +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. rb/lib/selenium/webdriver/bidi/log_handler.rb [44-47] 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. rb/lib/selenium/webdriver/bidi/struct.rb [34-36] -def camel_to_snake(camel_str) +def self.camel_to_snake(camel_str) camel_str.gsub(/([A-Z])/, '_\1').downcase end Suggestion importance[1-10]: 7 Why: Converting camel_to_snake to a class method is a good practice as it does not rely on instance state, making the method more reusable.	7
Maintainability	Extract deprecation warning logic into a separate private method for better readability To improve readability and maintainability, consider extracting the deprecation warning logic in the callbacks method into a separate private method. rb/lib/selenium/webdriver/bidi.rb [39-40] -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.	6

[titusfortner]

I have some more tweaks for this, but does it look good enough for now?

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	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. rb/lib/selenium/webdriver/bidi/log_handler.rb [33-41] +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]: 8 Why: This is a crucial improvement for consistency and usability, allowing users to manage handlers effectively.	8
	Return a boolean indicating whether the callback was successfully removed in the remove_callback method The remove_callback method should return a boolean indicating whether the callback was successfully removed to provide feedback to the caller. rb/lib/selenium/webdriver/common/websocket_connection.rb [60-64] 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. rb/lib/selenium/webdriver/bidi/log_handler.rb [44-47] 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. rb/lib/selenium/webdriver/bidi/struct.rb [34-36] -def camel_to_snake(camel_str) +def self.camel_to_snake(camel_str) camel_str.gsub(/([A-Z])/, '_\1').downcase end Suggestion importance[1-10]: 6 Why: Converting camel_to_snake to a class method is a good practice for utility methods that don't rely on instance state, enhancing reusability.	6