[rb] implement toggle for BiDi and Classic implementations

titusfortner commented 3 weeks ago

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

codiumai-pr-agent-pro[bot] commented 3 weeks ago

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.
⚡ Possible issues	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

codiumai-pr-agent-pro[bot] commented 3 weeks ago

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]](https://github.com/SeleniumHQ/selenium/pull/14092/files#diff-ecf89969518aec5e73ce3aff22dc10e88185534845a8a17228952c92f1498cddR321-R321) ```diff -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]](https://github.com/SeleniumHQ/selenium/pull/14092/files#diff-812b66f6ce3688d462c12e405e4808ffc075e0b6b2b9804b236ca977539a46e7R28-R29) ```diff 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]](https://github.com/SeleniumHQ/selenium/pull/14092/files#diff-812b66f6ce3688d462c12e405e4808ffc075e0b6b2b9804b236ca977539a46e7R34-R35) ```diff 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]](https://github.com/SeleniumHQ/selenium/pull/14092/files#diff-42b562229083afc6ac8cc70a735fc1b24863402aa1404f08f60322b24cdfefc3R605-R607) ```diff -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

codiumai-pr-agent-pro[bot] commented 3 weeks ago

CI Failure Feedback 🧐

(Checks updated until commit https://github.com/SeleniumHQ/selenium/commit/b41d57f8b80cd98ee0ef8518863da6462130a5d0)

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

**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9402899887/job/25901202558) [❌]

**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:

```yaml 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](https://pr-agent-docs.codium.ai/tools/ci_feedback/).

titusfortner commented 3 weeks ago

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.

SeleniumHQ / selenium