[rb] implement toggle for BiDi and Classic implementations

[titusfortner]

User description

This is a much simpler implementation of, and replaces #13126

Description

• Instead of extending the bridge dynamically, it uses a subclassed bridge
• We can do this by picking the bridge before starting the session, based on the capabilities being passed in; sending "webSocketUrl": true to the driver should give the right failure message if it is not supported, bindings don't have to work around it
• All BiDi usages in bridge are now in the BiDiBridge
• Calling #bidi on Bridge gives a warning that web_socket_url must be used to access BiDi (this is the real reason to pass the bridge to the BiDi classes and not just the bidi instance; the warning is in one place and works everywhere)

Motivation and Context

This is a prerequisite for #13995

---

PR Type

Enhancement

---

Description

• Introduced a new BiDiBridge class to handle BiDi-specific logic.
• Modified create_bridge method to use BiDiBridge when webSocketUrl capability is true.
• Added autoload for BiDiBridge in the remote module.
• Refactored Bridge class to remove BiDi-specific logic and updated the bidi method to raise an error if web_socket_url is not enabled.

---

Changes walkthrough 📝

Relevant files

Enhancement

driver.rb Use `BiDiBridge` subclass based on `webSocketUrl` capability rb/lib/selenium/webdriver/common/driver.rb Modified create_bridge method to use BiDiBridge subclass when webSocketUrl is true. +2/-1      remote.rb Autoload `BiDiBridge` in remote module                                      rb/lib/selenium/webdriver/remote.rb Added autoload for BiDiBridge. +1/-0      bidi_bridge.rb Implement `BiDiBridge` class with session management          rb/lib/selenium/webdriver/remote/bidi_bridge.rb Added new BiDiBridge class inheriting from Bridge. Implemented create_session, quit, and close methods in BiDiBridge. +44/-0    bridge.rb Refactor `Bridge` class to remove BiDi logic                          rb/lib/selenium/webdriver/remote/bridge.rb Removed BiDi-specific logic from Bridge class. Updated bidi method to raise an error if web_socket_url is not enabled. +3/-7

---

💡 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 🔍

⏱️ Estimated effort to review [1-5]	3, because the changes involve significant architectural modifications including the introduction of a new class and refactoring of existing methods. Understanding the impact of these changes on the overall system requires a good grasp of the existing architecture and the new requirements.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The create_session method in BiDiBridge assumes that @capabilities[:web_socket_url] is always present and valid. If it's not provided or incorrect, it could lead to runtime errors.
	Error Handling: The quit and close methods in BiDiBridge call bidi.close without checking if bidi has been successfully initialized (i.e., not nil). This could potentially lead to a NoMethodError if create_session fails or does not set @bidi.
🔒 Security concerns	No

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add a check to ensure the presence of the webSocketUrl key in caps to avoid potential errors Consider adding a check to ensure that caps contains the webSocketUrl key before accessing it to avoid potential KeyError. rb/lib/selenium/webdriver/common/driver.rb [321] -klass = caps['webSocketUrl'] ? Remote::BiDiBridge : Remote::Bridge +klass = caps.key?('webSocketUrl') ? Remote::BiDiBridge : Remote::Bridge Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potential KeyError when accessing a key directly in a hash without checking its existence. It's a significant improvement for robustness.	8
	Add a check to ensure socket_url is not nil before initializing Selenium::WebDriver::BiDi Add a check to ensure socket_url is not nil before initializing Selenium::WebDriver::BiDi to prevent potential errors. rb/lib/selenium/webdriver/remote/bidi_bridge.rb [28-29] socket_url = @capabilities[:web_socket_url] -@bidi = Selenium::WebDriver::BiDi.new(url: socket_url) +if socket_url + @bidi = Selenium::WebDriver::BiDi.new(url: socket_url) +else + raise(WebDriver::Error::WebDriverError, 'WebSocket URL is missing') +end Suggestion importance[1-10]: 8 Why: This suggestion correctly addresses a potential issue where socket_url could be nil, which would lead to errors during the initialization of Selenium::WebDriver::BiDi. It significantly improves error handling and robustness.	8
	Add a safe navigation operator to prevent potential NoMethodError when calling close on bidi Add a check to ensure @bidi is not nil before calling close in the quit method to prevent potential NoMethodError. rb/lib/selenium/webdriver/remote/bidi_bridge.rb [34-35] ensure - bidi.close + bidi&.close Suggestion importance[1-10]: 7 Why: The suggestion to use the safe navigation operator is valid and improves the code by preventing possible runtime errors if bidi is nil.	7
Maintainability	Remove the redundant bidi method definition from the Bridge class Remove the redundant bidi method definition since it is now handled in the BiDiBridge class. rb/lib/selenium/webdriver/remote/bridge.rb [605-607] -def bidi - msg = 'BiDi must be enabled by setting #web_socket_url to true in options class' - raise(WebDriver::Error::WebDriverError, msg) -end +# Method removed as it is redundant Suggestion importance[1-10]: 6 Why: Removing redundant code improves maintainability. However, the suggestion does not consider that the bidi method in Bridge might still be needed for error handling and messaging, thus it's not entirely redundant.	6

[qodo-merge-pro]

CI Failure Feedback 🧐

(Checks updated until commit b41d57f)

Action: Ruby / Remote Tests (edge, windows) / Remote Tests (edge, windows)

Failed stage: Run Bazel [❌]

Failure summary: The action failed because the protoc.exe command encountered an error during the generation of the proto_library for the target @io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto. The specific error code was Exit -1073741819. This caused the build to not complete successfully, and as a result, 1 test failed to build and 27 tests were skipped.

Relevant error logs: 1: ##[group]Operating System 2: Microsoft Windows Server 2022 ... 619: �[32m[1 / 1]�[0m checking cached actions 620: �[32mAnalyzing:�[0m 28 targets (680 packages loaded, 11617 targets configured) 621: �[32m[1 / 1]�[0m checking cached actions 622: �[32mAnalyzing:�[0m 28 targets (680 packages loaded, 38612 targets configured) 623: �[32m[1 / 1]�[0m checking cached actions 624: �[32mAnalyzing:�[0m 28 targets (685 packages loaded, 40121 targets configured) 625: �[32m[1 / 1]�[0m checking cached actions 626: �[32mAnalyzing:�[0m 28 targets (685 packages loaded, 40151 targets configured) 627: �[32m[1 / 17]�[0m [Prepa] Writing repo mapping manifest for //rb/spec/integration/selenium/webdriver:error-edge-remote ... 736: �[32m[1,992 / 3,109]�[0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 6s local, disk-cache ... (3 actions, 1 running) 737: �[32m[2,081 / 3,109]�[0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 7s local, disk-cache ... (3 actions, 1 running) 738: �[32mINFO: �[0mFrom Building external/protobuf~/java/core/liblite_runtime_only.jar (91 source files) [for tool]: 739: external\protobuf~\java\core\src\main\java\com\google\protobuf\UnsafeUtil.java:293: warning: [removal] AccessController in java.security has been deprecated and marked for removal 740: AccessController.doPrivileged( 741: ^ 742: �[32m[2,182 / 3,109]�[0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 8s local, disk-cache ... (3 actions, 1 running) 743: �[32mINFO: �[0mFrom Building java/src/org/openqa/selenium/remote/libapi-class.jar (66 source files): 744: java\src\org\openqa\selenium\remote\ErrorHandler.java:46: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 745: private final ErrorCodes errorCodes; 746: ^ 747: java\src\org\openqa\selenium\remote\ErrorHandler.java:60: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 748: this.errorCodes = new ErrorCodes(); 749: ^ 750: java\src\org\openqa\selenium\remote\ErrorHandler.java:68: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 751: public ErrorHandler(ErrorCodes codes, boolean includeServerErrors) { 752: ^ 753: java\src\org\openqa\selenium\remote\Response.java:97: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 754: ErrorCodes errorCodes = new ErrorCodes(); 755: ^ 756: java\src\org\openqa\selenium\remote\Response.java:97: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 757: ErrorCodes errorCodes = new ErrorCodes(); 758: ^ 759: java\src\org\openqa\selenium\remote\ProtocolHandshake.java:181: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 760: response.setStatus(ErrorCodes.SUCCESS); 761: ^ 762: java\src\org\openqa\selenium\remote\ProtocolHandshake.java:182: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 763: response.setState(ErrorCodes.SUCCESS_STRING); 764: ^ 765: java\src\org\openqa\selenium\remote\W3CHandshakeResponse.java:53: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 766: new ErrorCodes().toStatus((String) rawError, Optional.of(tuple.getStatusCode()))); 767: ^ 768: java\src\org\openqa\selenium\remote\W3CHandshakeResponse.java:56: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 769: new ErrorCodes().getExceptionType((String) rawError); 770: ^ 771: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:44: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 772: private final ErrorCodes errorCodes = new ErrorCodes(); 773: ^ 774: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:44: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 775: private final ErrorCodes errorCodes = new ErrorCodes(); 776: ^ 777: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:55: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 778: int status = response.getStatus() == ErrorCodes.SUCCESS ? HTTP_OK : HTTP_INTERNAL_ERROR; 779: ^ 780: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:101: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 781: response.setStatus(ErrorCodes.UNKNOWN_COMMAND); 782: ^ 783: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:103: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 784: response.setStatus(ErrorCodes.UNHANDLED_ERROR); 785: ^ 786: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:124: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 787: response.setStatus(ErrorCodes.SUCCESS); 788: ^ 789: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:125: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 790: response.setState(errorCodes.toState(ErrorCodes.SUCCESS)); 791: ^ 792: java\src\org\openqa\selenium\remote\codec\AbstractHttpResponseCodec.java:131: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 793: response.setState(errorCodes.toState(ErrorCodes.SUCCESS)); 794: ^ 795: java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:70: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 796: private final ErrorCodes errorCodes = new ErrorCodes(); 797: ^ 798: java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:70: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 799: private final ErrorCodes errorCodes = new ErrorCodes(); 800: ^ 801: java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:93: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 802: response.setStatus(ErrorCodes.UNKNOWN_COMMAND); 803: ^ 804: java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:98: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 805: response.setStatus(ErrorCodes.UNHANDLED_ERROR); 806: ^ 807: java\src\org\openqa\selenium\remote\codec\w3c\W3CHttpResponseCodec.java:145: warning: [removal] ErrorCodes in org.openqa.selenium.remote has been deprecated and marked for removal 808: response.setStatus(ErrorCodes.SUCCESS); ... 813: C:\Users\RUNNER~1\AppData\Local\Temp\Bazel.runfiles_pue6bkac\runfiles\rules_python~~python~python_3_8_x86_64-pc-windows-msvc\lib\zipfile.py:1517: UserWarning: Duplicate name: 'grid-ui/' 814: return self._open_to_write(zinfo, force_zip64=force_zip64) 815: �[32m[2,200 / 3,109]�[0m Installing external/rules_ruby~~ruby~bundle/rb/vendor/cache/bundler-2.5.10.gem (@bundle//:bundler-2.5.10); 2s local, disk-cache ... (4 actions, 2 running) 816: �[32mINFO: �[0mFrom Installing external/rules_ruby~~ruby~bundle/rb/vendor/cache/bundler-2.5.10.gem (@@rules_ruby~~ruby~bundle//:bundler-2.5.10): 817: Successfully installed bundler-2.5.10 818: 1 gem installed 819: �[32m[2,201 / 3,109]�[0m Linking external/protobuf~/protoc.exe [for tool]; 1s local, disk-cache ... (4 actions, 2 running) 820: �[32m[2,202 / 3,109]�[0m RunBinary rb/lib/selenium/devtools/v124.rb; 2s disk-cache ... (4 actions, 2 running) 821: �[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) 822: cd /d D:/_bazel/execroot/_main 823: SET PATH=C:\Program Files\Git\bin;C:\Program Files\Git\usr\bin;C:\Windows;C:\Windows\System32;C:\Windows\System32\WindowsPowerShell\v1.0 824: 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 825: # Configuration: 3f62d58e1724b33c4c5a838ec5dd37be29e6f58351fe7900d780cb5e8258345c 826: # Execution platform: @@local_config_platform//:host 827: �[32m[2,206 / 3,109]�[0m 1 / 28 tests, �[31m�[1m1 failed�[0m;�[0m checking cached actions 828: �[32mINFO: �[0mElapsed time: 300.395s, Critical Path: 18.44s 829: �[32mINFO: �[0m1968 processes: 558 disk cache hit, 1227 internal, 183 local. 830: �[31m�[1mERROR: �[0mBuild did NOT complete successfully 831: //rb/spec/integration/selenium/webdriver:action_builder-edge-remote �[0m�[31m�[1mNO STATUS�[0m 832: //rb/spec/integration/selenium/webdriver:bidi-edge-remote �[0m�[31m�[1mNO STATUS�[0m 833: //rb/spec/integration/selenium/webdriver:devtools-edge-remote �[0m�[31m�[1mNO STATUS�[0m 834: //rb/spec/integration/selenium/webdriver:driver-edge-remote �[0m�[31m�[1mNO STATUS�[0m 835: //rb/spec/integration/selenium/webdriver:element-edge-remote �[0m�[31m�[1mNO STATUS�[0m 836: //rb/spec/integration/selenium/webdriver:error-edge-remote �[0m�[31m�[1mNO STATUS�[0m ... 850: //rb/spec/integration/selenium/webdriver/bidi:log_inspector-edge-remote �[0m�[31m�[1mNO STATUS�[0m 851: //rb/spec/integration/selenium/webdriver/bidi:script-edge-remote �[0m�[31m�[1mNO STATUS�[0m 852: //rb/spec/integration/selenium/webdriver/edge:driver-edge-remote �[0m�[31m�[1mNO STATUS�[0m 853: //rb/spec/integration/selenium/webdriver/edge:options-edge-remote �[0m�[31m�[1mNO STATUS�[0m 854: //rb/spec/integration/selenium/webdriver/edge:profile-edge-remote �[0m�[31m�[1mNO STATUS�[0m 855: //rb/spec/integration/selenium/webdriver/edge:service-edge-remote �[0m�[31m�[1mNO STATUS�[0m 856: //rb/spec/integration/selenium/webdriver/remote:driver-edge-remote �[0m�[31m�[1mNO STATUS�[0m 857: //rb/spec/integration/selenium/webdriver/remote:element-edge-remote �[0m�[31m�[1mNO STATUS�[0m 858: //rb/spec/integration/selenium/webdriver:virtual_authenticator-edge-remote �[0m�[31m�[1mFAILED TO BUILD�[0m 859: Executed 0 out of 28 tests: �[0m�[31m�[1m1 fails to build�[0m and �[0m�[35m27 were skipped�[0m. 860: �[0m 861: ##[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]

These test failures are weird protoc compile issues that don't make sense and definitely don't impact this PR (and intermittently pass on rerun 🤷). The RBE tests are passing is the important piece.