[build] Add Bazel bidi flag

[titusfortner]

User description

I was going to just commit the Ruby pieces directly, but it doesn't really make sense to do that until we know how we are running the tests with Bazel.

So, I'm happy with the Ruby code, but I need feedback on the bazel toggle and how we want to start running both modes in our test suite

Description

• Creates toggle for enabling BiDi in Ruby
• Creates guard for Ruby tests to run or not run when BiDi is enabled
• Updated code to minimize execution time when a bunch of tests are skipped
• Created ability to toggle on the Environment variable which toggles on Ruby tests with --bidi

Motivation and Context

This matters as we start to implement Selenium features with both WebDriver classic and WebDriver bidi. We want to make sure that both modes continue to work for people.

---

PR Type

Enhancement, Tests

---

Description

• Added exclusive conditions for BiDi in various test files to ensure tests are run only when BiDi is enabled.
• Modified after hooks in several test files to reset the driver based on the example.
• Refactored tests to remove redundant reset_driver! calls.
• Added BiDi condition to guards and reset_driver! method.
• Updated Bazel configurations to include BiDi flag and settings.

---

Changes walkthrough 📝

	Relevant files
Tests	36 files action_builder_spec.rb Add BiDi exclusive condition to ActionBuilder tests            rb/spec/integration/selenium/webdriver/action_builder_spec.rb Added exclusive condition for BiDi in ActionBuilder tests. +1/-1      browsing_context_spec.rb Add BiDi exclusive condition and modify after hook in BrowsingContext tests rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb Added exclusive condition for BiDi in BrowsingContext tests. Modified after hook to reset driver based on example. +2/-3      log_inspector_spec.rb Add BiDi exclusive condition and refactor LogInspector tests rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb Added exclusive condition for BiDi in LogInspector tests. Refactored tests to remove redundant reset_driver! calls. +161/-185 script_spec.rb Add BiDi exclusive condition and modify Script tests          rb/spec/integration/selenium/webdriver/bidi/script_spec.rb Added exclusive condition for BiDi in Script tests. Modified after hook to reset driver based on example. Updated error message for removing non-existent handler. +5/-6      bidi_spec.rb Add BiDi exclusive condition and modify BiDi tests              rb/spec/integration/selenium/webdriver/bidi_spec.rb Added exclusive condition for BiDi in BiDi tests. Modified after hook to reset driver based on example. +3/-9      driver_spec.rb Add BiDi exclusive condition to Chrome Driver tests            rb/spec/integration/selenium/webdriver/chrome/driver_spec.rb Added exclusive condition for BiDi in Chrome::Driver tests. +1/-1      options_spec.rb Add BiDi exclusive condition to Chrome Options tests          rb/spec/integration/selenium/webdriver/chrome/options_spec.rb Added exclusive condition for BiDi in Chrome::Options tests. +1/-1      profile_spec.rb Add BiDi exclusive condition to Chrome Profile tests          rb/spec/integration/selenium/webdriver/chrome/profile_spec.rb Added exclusive condition for BiDi in Chrome::Profile tests. +1/-1      service_spec.rb Add BiDi exclusive condition to Chrome Service tests          rb/spec/integration/selenium/webdriver/chrome/service_spec.rb Added exclusive condition for BiDi in Chrome::Service tests. +1/-1      devtools_spec.rb Add BiDi exclusive condition and modify DevTools tests      rb/spec/integration/selenium/webdriver/devtools_spec.rb Added exclusive condition for BiDi in DevTools tests. Modified after hook to reset driver based on example. +3/-2      driver_spec.rb Add BiDi exclusive condition to Driver tests                          rb/spec/integration/selenium/webdriver/driver_spec.rb Added exclusive condition for BiDi in Driver tests. +1/-1      driver_spec.rb Add BiDi exclusive condition to Edge Driver tests                rb/spec/integration/selenium/webdriver/edge/driver_spec.rb Added exclusive condition for BiDi in Edge::Driver tests. +1/-1      options_spec.rb Add BiDi exclusive condition to Edge Options tests              rb/spec/integration/selenium/webdriver/edge/options_spec.rb Added exclusive condition for BiDi in Edge::Options tests. +1/-1      profile_spec.rb Add BiDi exclusive condition to Edge Profile tests              rb/spec/integration/selenium/webdriver/edge/profile_spec.rb Added exclusive condition for BiDi in Edge::Profile tests. +1/-1      service_spec.rb Add BiDi exclusive condition to Edge Service tests              rb/spec/integration/selenium/webdriver/edge/service_spec.rb Added exclusive condition for BiDi in Edge::Service tests. +1/-1      element_spec.rb Add BiDi exclusive condition to Element tests                        rb/spec/integration/selenium/webdriver/element_spec.rb Added exclusive condition for BiDi in Element tests. +1/-1      error_spec.rb Add BiDi exclusive condition to Error tests                            rb/spec/integration/selenium/webdriver/error_spec.rb Added exclusive condition for BiDi in Error tests. +1/-1      driver_spec.rb Add BiDi exclusive condition to Firefox Driver tests          rb/spec/integration/selenium/webdriver/firefox/driver_spec.rb Added exclusive condition for BiDi in Firefox::Driver tests. +1/-1      profile_spec.rb Add BiDi exclusive condition to Firefox Profile tests        rb/spec/integration/selenium/webdriver/firefox/profile_spec.rb Added exclusive condition for BiDi in Firefox::Profile tests. +1/-1      service_spec.rb Add BiDi exclusive condition to Firefox Service tests        rb/spec/integration/selenium/webdriver/firefox/service_spec.rb Added exclusive condition for BiDi in Firefox::Service tests. +1/-1      guard_spec.rb Add BiDi exclusive condition to Guards tests                          rb/spec/integration/selenium/webdriver/guard_spec.rb Added exclusive condition for BiDi in Guards tests. +1/-1      listener_spec.rb Add BiDi exclusive condition to Listener tests                      rb/spec/integration/selenium/webdriver/listener_spec.rb Added exclusive condition for BiDi in Listener tests. +1/-1      manager_spec.rb Add BiDi exclusive condition to Manager tests                        rb/spec/integration/selenium/webdriver/manager_spec.rb Added exclusive condition for BiDi in Manager tests. +1/-1      navigation_spec.rb Add BiDi exclusive condition to Navigation tests                  rb/spec/integration/selenium/webdriver/navigation_spec.rb Added exclusive condition for BiDi in Navigation tests. +1/-1      driver_spec.rb Add BiDi exclusive condition to Remote Driver tests            rb/spec/integration/selenium/webdriver/remote/driver_spec.rb Added exclusive condition for BiDi in Remote::Driver tests. +1/-1      element_spec.rb Add BiDi exclusive condition to Remote Element tests          rb/spec/integration/selenium/webdriver/remote/element_spec.rb Added exclusive condition for BiDi in Remote::Element tests. +2/-4      driver_spec.rb Add BiDi exclusive condition to Safari Driver tests            rb/spec/integration/selenium/webdriver/safari/driver_spec.rb Added exclusive condition for BiDi in Safari::Driver tests. +2/-1      select_spec.rb Add BiDi exclusive condition to Select tests                          rb/spec/integration/selenium/webdriver/select_spec.rb Added exclusive condition for BiDi in Select tests. +1/-1      shadow_root_spec.rb Add BiDi exclusive condition to ShadowRoot tests                  rb/spec/integration/selenium/webdriver/shadow_root_spec.rb Added exclusive condition for BiDi in ShadowRoot tests. +2/-1      storage_spec.rb Add BiDi exclusive condition to HasWebStorage tests            rb/spec/integration/selenium/webdriver/storage_spec.rb Added exclusive condition for BiDi in HasWebStorage tests. +1/-1      takes_screenshot_spec.rb Add BiDi exclusive condition and refactor TakesScreenshot tests rb/spec/integration/selenium/webdriver/takes_screenshot_spec.rb Added exclusive condition for BiDi in TakesScreenshot tests. Refactored after hook to remove redundant code. +2/-4      target_locator_spec.rb Add BiDi exclusive condition and modify after hook in TargetLocator tests rb/spec/integration/selenium/webdriver/target_locator_spec.rb Added exclusive condition for BiDi in TargetLocator tests. Modified after hook to reset driver based on example. +2/-2      timeout_spec.rb Add BiDi exclusive condition to Timeouts tests                      rb/spec/integration/selenium/webdriver/timeout_spec.rb Added exclusive condition for BiDi in Timeouts tests. +1/-1      virtual_authenticator_spec.rb Add BiDi exclusive condition to VirtualAuthenticator tests rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb Added exclusive condition for BiDi in VirtualAuthenticator tests. +2/-1      window_spec.rb Add BiDi exclusive condition to Window tests                          rb/spec/integration/selenium/webdriver/window_spec.rb Added exclusive condition for BiDi in Window tests. +1/-1      zipper_spec.rb Add BiDi exclusive condition and refactor Zipper tests      rb/spec/integration/selenium/webdriver/zipper_spec.rb Added exclusive condition for BiDi in Zipper tests. Refactored after hook to remove redundant code. +2/-4
Enhancement	2 files spec_helper.rb Add BiDi condition to guards and modify after hook              rb/spec/integration/selenium/webdriver/spec_helper.rb Added BiDi condition to guards. Modified after hook to reset driver based on example. +2/-1      test_environment.rb Add BiDi condition to reset_driver and build_options          rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb Added BiDi condition to reset_driver! method. Modified build_options to set web_socket_url based on environment variable. +10/-0
Configuration changes	3 files tests.bzl Add BiDi condition to Bazel test configurations                    rb/spec/tests.bzl Added BiDi condition to Bazel test configurations. +12/-0    .bazelrc Add BiDi flag alias to Bazel configuration                              .bazelrc Added BiDi flag alias to Bazel configuration. +1/-0      BUILD.bazel Add BiDi flag and configuration settings to Bazel build    common/BUILD.bazel Added BiDi flag and configuration settings. +12/-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 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR introduces a significant number of changes across multiple test files and configurations. The changes involve adding exclusive conditions for BiDi in various test files and modifying after hooks. Additionally, the Bazel configuration updates to include BiDi flags increase the complexity. Reviewing this PR requires a good understanding of the existing test suite and the implications of the BiDi feature on it.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The exclusive condition {bidi: false, reason: 'Not yest implemented with BiDi'} is used in multiple places. There's a typo in the word "yet". This should be corrected to maintain professionalism and clarity in the codebase.
	Redundancy Concern: The PR includes a lot of repetitive changes for adding BiDi exclusivity across various test files. It might be beneficial to abstract some of this functionality or configuration to reduce redundancy and improve maintainability.
🔒 Security concerns	No

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Add a description to the bool_flag definition for clarity and consistency To ensure consistency and avoid potential issues, consider adding a description to the bool_flag definition for the bidi flag. This will help other developers understand its purpose. common/BUILD.bazel [29-32] bool_flag( name = "bidi", build_setting_default = False, + doc = "Flag to enable WebDriver BiDi support", ) Suggestion importance[1-10]: 8 Why: Adding a description to the bool_flag definition is a best practice that aids in understanding the purpose of the flag, directly improving code documentation and clarity.	8
	Add an after block to reset or quit the log_inspector to prevent side effects between tests To ensure that the log_inspector is properly cleaned up after each test, consider adding an after block to reset or quit the log_inspector. This will help prevent any potential side effects between tests. rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb [30] log_inspector = described_class.new(driver) +after { log_inspector.quit } Suggestion importance[1-10]: 5 Why: Adding an after block to clean up resources is a good practice, but the suggestion assumes log_inspector has a quit method which may not exist, hence the moderate score.	5
Possible issue	Add a timeout value to wait.until calls to make the tests more robust and provide clearer error messages To avoid potential issues with wait.until timing out, consider adding a timeout value to the wait.until calls. This will make the tests more robust and provide clearer error messages if the expected conditions are not met within the specified time. rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb [35] -log_entry = wait.until { log_entries.find { _1.text == 'Hello, world!' } } +log_entry = wait.until(timeout: 10) { log_entries.find { _1.text == 'Hello, world!' } } Suggestion importance[1-10]: 7 Why: Adding a timeout to wait.until calls is a good practice to avoid indefinite waiting and provide clearer error messages, enhancing test reliability.	7
Maintainability	Group similar select statements together to improve readability and maintainability To improve readability and maintainability, consider grouping the select statements for similar configurations together. This will make it easier to understand the different configurations being applied. rb/spec/tests.bzl [33-38] }) | select({ "@selenium//common:use_webdriver_bidi": {"WEBDRIVER_BIDI": "true"}, - "//conditions:default": {}, + "@selenium//common:use_headless_browser": {"HEADLESS": "true"}, + "//conditions:default": {"NO_SANDBOX": "true"}, }) | select({ "@platforms//os:windows": {}, - "//conditions:default": {"NO_SANDBOX": "true"}, + "//conditions:default": {}, Suggestion importance[1-10]: 7 Why: The suggestion to group similar select statements is valid and would enhance readability and maintainability, but the exact improved code provided does not match the PR context perfectly.	7
	Extract repeated log entry attributes into a shared method or variable to reduce duplication To improve readability and maintainability, consider extracting the repeated log entry attributes into a shared method or variable. This will reduce duplication and make it easier to update the expected attributes in the future. rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb [37-46] -expect(log_entry).to have_attributes( +expected_attributes = { text: 'Hello, world!', realm: nil, type: 'console', level: LogInspector::LOG_LEVEL[:INFO], method: 'log', args: [{ 'type' => 'string', 'value' => 'Hello, world!' }] -) +} +expect(log_entry).to have_attributes(expected_attributes) Suggestion importance[1-10]: 6 Why: Extracting repeated attributes into a shared method or variable reduces code duplication and improves maintainability, though it's not a critical change.	6
Typo	Correct a typo in the reason for the exclusive flag The reason for the exclusive flag contains a typo: "Not yest implemented with BiDi". It should be corrected to "Not yet implemented with BiDi". rb/spec/integration/selenium/webdriver/target_locator_spec.rb [24] -describe TargetLocator, exclusive: {bidi: false, reason: 'Not yest implemented with BiDi'} do +describe TargetLocator, exclusive: {bidi: false, reason: 'Not yet implemented with BiDi'} do Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies and fixes a typo in the code, which improves code clarity and professionalism.	7
Enhancement	Place the new bidi flag alias in alphabetical order for better readability To maintain alphabetical order and improve readability, consider placing the new bidi flag alias in the correct position relative to the existing flag aliases. .bazelrc [18-20] +build --flag_alias=bidi=//common:bidi +build --flag_alias=headless=//common:headless build --flag_alias=pin_browsers=//common:pin_browsers -build --flag_alias=headless=//common:headless -build --flag_alias=bidi=//common:bidi Suggestion importance[1-10]: 6 Why: Maintaining alphabetical order for flag aliases improves readability and maintainability, though it's a minor enhancement in the context of the entire project.	6
Performance	Add a guard clause to the reset_driver! method to check if the driver is already reset, improving performance To avoid potential issues with the reset_driver! method being called unnecessarily, consider adding a guard clause to check if the driver is already reset. This will improve performance and prevent redundant operations. rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb [74-76] +return if driver_already_reset? quit_driver sleep time driver_instance(**opts, &block) Suggestion importance[1-10]: 4 Why: Adding a guard clause to check if the driver is already reset could improve performance, but the suggestion lacks context on how driver_already_reset? would be implemented or if it's feasible.	4

[qodo-merge-pro]

CI Failure Feedback 🧐

(Checks updated until commit e153eb5)

Action: Format / Check format script run

Failed stage: Run Bazel [❌]

Failed test name: it 'is skipped without provided reason', exclusive: {foo: true}

Failure summary: The action failed because the test it 'is skipped without provided reason', exclusive: {foo: true} failed. The failure occurred due to an expectation mismatch in the guards.disposition value.

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 975: Package 'php-symfony-debug-bundle' is not installed, so not removed 976: Package 'php-symfony-dependency-injection' is not installed, so not removed 977: Package 'php-symfony-deprecation-contracts' is not installed, so not removed 978: Package 'php-symfony-discord-notifier' is not installed, so not removed 979: Package 'php-symfony-doctrine-bridge' is not installed, so not removed 980: Package 'php-symfony-doctrine-messenger' is not installed, so not removed 981: Package 'php-symfony-dom-crawler' is not installed, so not removed 982: Package 'php-symfony-dotenv' is not installed, so not removed 983: Package 'php-symfony-error-handler' is not installed, so not removed ... 1800: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/exec.js 7ms (unchanged) 1801: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/index.js 15ms (unchanged) 1802: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/zip.js 11ms (unchanged) 1803: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/jsdoc_conf.json 3ms (unchanged) 1804: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/atoms/make-atoms-module.js 2ms (unchanged) 1805: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/by.js 18ms (unchanged) 1806: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/capabilities.js 15ms (unchanged) 1807: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/command.js 9ms (unchanged) 1808: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/error.js 30ms (unchanged) ... 1879: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/http/util_test.js 11ms (unchanged) 1880: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/ie/options_test.js 7ms (unchanged) 1881: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/io_test.js 29ms (unchanged) 1882: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/zip_test.js 17ms (unchanged) 1883: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/api_test.js 4ms (unchanged) 1884: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/by_test.js 12ms (unchanged) 1885: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/capabilities_test.js 14ms (unchanged) 1886: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/credentials_test.js 12ms (unchanged) 1887: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/error_test.js 16ms (unchanged) ... 2224: @@ -77,7 +77,8 @@ module Selenium 2225: guards = described_class.new(example) 2226: guards.add_condition(:foo, false) 2227: - expect(guards.disposition).to eq [:pending, 'Test guarded; Guarded by {:foo=>false, :reason=>"No reason given"};'] 2228: + expect(guards.disposition).to eq [:pending, 2229: + 'Test guarded; Guarded by {:foo=>false, :reason=>"No reason given"};'] 2230: end 2231: it 'is skipped without provided reason', exclusive: {foo: true} do |example| 2232: ##[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.

[p0deje]

I think it's a great start, but it might be tricky to incorporate that into RBE because whenever you turn on/off --bidi - it will invalidate the cache.

A better approach would be to generate extra -bidi test targets that will force running tests with BiDi enabled. Something like this:

# rb/spec/tests.bzl
# inside `rb_integration_test`

        # .. some code above ..

        # Generate a test target for bidi browser execution.
        rb_test(
            name = "{}-{}-bidi".format(name, browser),
            size = "large",
            srcs = srcs,
            args = ["rb/spec/"],
            data = BROWSERS[browser]["data"] + data + ["//common/src/web"],
            env = BROWSERS[browser]["env"] | {"WEBDRIVER_BIDI": "true"},
            main = "@bundle//bin:rspec",
            tags = COMMON_TAGS + BROWSERS[browser]["tags"] + tags + ["%s-bidi" % browser],
            # below we need to depend on bidi package in rb tree
            deps = ["//rb/spec/integration/selenium/webdriver:spec_helper"] + BROWSERS[browser]["deps"] + deps,
            visibility = ["//rb:__subpackages__"],
            target_compatible_with = BROWSERS[browser]["target_compatible_with"],
        )

This way you'd have the new targets like this:

• //rb/spec/integration/selenium/webdriver:driver-edge
• //rb/spec/integration/selenium/webdriver:driver-edge-bidi

Each target will be executed separately and cached.

If you only want to run bidi tests, you'd use :

bazel test --test_tag_filters edge-bidi //rb/...

[titusfortner]

this should be good now. Looks much better, thanks