Skip to content

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented Oct 21, 2025

User description

🔗 Related Issues

The goal is to run existing api through webdriver-bidi and webdriver-classic settings while we transition, but there is no need to run everything with webdriver-bidi when it hasn't been implemented yet

💥 What does this PR do?

reduces how many tests are run to only those affected

🔧 Implementation Notes

I was going to do this with Ruby guards, but that would be harder to deal with

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Minimize Ruby BiDi test targets by only creating -bidi variants for tests explicitly tagged with "bidi"

  • Remove unnecessary BiDi test targets from skipped tests list

  • Consolidate BiDi-specific tests into dedicated test files with proper tagging

  • Simplify build configuration by eliminating redundant BiDi target generation


Diagram Walkthrough

flowchart LR
  A["rb_integration_test function"] -->|Check for bidi tag| B["Create -bidi target?"]
  B -->|bidi tag present| C["Generate -bidi variant"]
  B -->|bidi tag absent| D["Skip -bidi generation"]
  E["BiDi test files"] -->|Tag with bidi| F["Selective target creation"]
  G[".skipped-tests"] -->|Remove obsolete entries| H["Cleaner skip list"]
Loading

File Walkthrough

Relevant files
Enhancement
tests.bzl
Add conditional BiDi target generation based on tags         

rb/spec/tests.bzl

  • Added conditional check to skip -bidi target creation for tests not
    tagged with "bidi"
  • Prevents unnecessary BiDi test variants from being generated during
    build
  • Reduces build complexity by only creating BiDi targets when explicitly
    needed
+3/-0     
BUILD.bazel
Consolidate BiDi tests with selective tagging                       

rb/spec/integration/selenium/webdriver/BUILD.bazel

  • Moved bidi_spec.rb from standalone target to list-based generation
    with "bidi" tag
  • Added navigation_spec.rb and network_spec.rb to BiDi-specific test
    files
  • Converted manual rb_integration_test definition to loop-based
    generation for BiDi tests
  • Added network_spec.rb to exclusion list in glob pattern
+15/-9   
BUILD.bazel
Tag BiDi tests with bidi identifier                                           

rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel

  • Added "bidi" tag to all BiDi test targets alongside existing
    "exclusive-if-local" tag
  • Ensures BiDi tests are properly identified for selective target
    generation
+4/-1     
Cleanup
.skipped-tests
Remove obsolete BiDi test skip entries                                     

.skipped-tests

  • Removed 7 Ruby BiDi test targets that are no longer generated
  • Entries like service-chrome-bidi, driver-firefox-beta-bidi,
    element-chrome-bidi removed
  • Cleaned up obsolete skip list entries due to new selective BiDi target
    generation
+0/-6     
BUILD.bazel
Remove unused BiDi dependency                                                       

rb/spec/BUILD.bazel

  • Removed dependency on //rb/spec/integration/selenium/webdriver:bidi
    target
  • Simplifies spec library dependencies by eliminating now-unused BiDi
    library reference
+0/-1     

@titusfortner titusfortner requested a review from p0deje October 21, 2025 00:26
@selenium-ci selenium-ci added C-rb Ruby Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Oct 21, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve repeated "Error: ConnectFailure (Connection refused)" when
instantiating multiple ChromeDriver sessions after the first one on Ubuntu 16.04 with
Chrome 65/Chromedriver 2.35 and Selenium 3.9.0.
Provide a fix or guidance so that subsequent ChromeDriver instantiations do not produce
ConnectFailure errors.
Validate behavior specifically for Chrome on the stated environment, ensuring reliability
across multiple driver instances.
🟡
🎫 #1234
🔴 Ensure click() triggers JavaScript in an anchor's href as in version 2.47.1 for Firefox
42.0, addressing regression in 2.48.0/2.48.2.
Provide tests or fixes to confirm alert is triggered as expected.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix off-by-one error in slicing
Suggestion Impact:The commit updated the name slicing from f[:-7] to f[:-8] in the rb_integration_test block, implementing the suggested fix.

code diff:

 [
     rb_integration_test(
-        name = f[:-7],
+        name = f[:-8],
         srcs = [f],
         tags = ["bidi"],
     )

Correct the string slicing from f[:-7] to f[:-8] to properly remove the _spec.rb
suffix from test file names.

rb/spec/integration/selenium/webdriver/BUILD.bazel [56-63]

 [
     rb_integration_test(
-        name = f[:-7],
+        name = f[:-8],
         srcs = [f],
         tags = ["bidi"],
     )
     for f in _BIDI_FILES
 ]

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a functional bug in the build script. The incorrect string slicing f[:-7] would generate malformed test target names, likely causing build failures or tests not being correctly identified.

High
Learned
best practice
Remove early macro return
Suggestion Impact:The early return was removed and replaced with a conditional that only creates the BiDi test target when "bidi" is in tags, ensuring other targets are unaffected.

code diff:

-        # Generate a test target for bidi browser execution.
-        if "bidi" not in tags:
-            return  # don't create -bidi targets for non-BiDi tests
-
-        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 + ["{}-bidi".format(browser)],
-            deps = depset(
-                ["//rb/spec/integration/selenium/webdriver:spec_helper", "//rb/lib/selenium/webdriver:bidi"] +
-                BROWSERS[browser]["deps"] +
-                deps,
-            ),
-            visibility = ["//rb:__subpackages__"],
-            target_compatible_with = BROWSERS[browser]["target_compatible_with"],
-        )
+        # Generate a test target for bidi browser execution if there is a matching tag
+        if "bidi" in tags:
+          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 + ["{}-bidi".format(browser)],
+              deps = depset(
+                  ["//rb/spec/integration/selenium/webdriver:spec_helper", "//rb/lib/selenium/webdriver:bidi"] +
+                  BROWSERS[browser]["deps"] +
+                  deps,
+              ),
+              visibility = ["//rb:__subpackages__"],
+              target_compatible_with = BROWSERS[browser]["target_compatible_with"],
+          )

Avoid returning early from the macro; guard only the BiDi target creation with a
conditional so other targets remain unaffected and macro flow stays predictable.

rb/spec/tests.bzl [218-219]

-if "bidi" not in tags:
-    return  # don't create -bidi targets for non-BiDi tests
+if "bidi" in tags:
+    rb_test(
+        name = "{}-{}-bidi".format(name, browser),
+        size = "large",
+        srcs = srcs,
+        # ...
+    )

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Keep build macros pure and predictable by avoiding early returns that skip subsequent target generation unexpectedly; prefer structured conditionals so all intended targets are considered.

Low
  • Update

@titusfortner titusfortner merged commit b3b23f5 into trunk Oct 21, 2025
31 checks passed
@titusfortner titusfortner deleted the rb_bidi_targets branch October 21, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related C-rb Ruby Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants