Skip to content

[rb] update bazel test tags#17558

Merged
titusfortner merged 5 commits into
trunkfrom
bazel_tags_rb
May 25, 2026
Merged

[rb] update bazel test tags#17558
titusfortner merged 5 commits into
trunkfrom
bazel_tags_rb

Conversation

@titusfortner
Copy link
Copy Markdown
Member

When I last reorganized the bazel tags I had a few different ideas conflated.

Ruby toggles bidi-enabled in the settings. We want all -bidi targets to enable Bidi and everything else not enabled, and not to have extra targets that are always skipped.

💥 What does this PR do?

  • bidi_only = True - enables bidi just for chrome-beta, firefox-beta and edge to minimize targets; does not create non-bidi targets that can't be run without a websocket
  • _BIDI_DUAL_TESTS are tests that operate differently in classic and bidi modes and need both targets
  • Created tags for OS Sensitive Tests to indicate which tests should be run on multiple platforms
  • Dropped redundant skip-rbe from ie/safari/safari-preview because we use target_compatible_with
  • Added unit test tag for filtering
  • removed unused dependencies

removes 84 no-op targets removed

🔧 Implementation Notes

  • We could add bidi to chrome and firefox as well, but seems like issues and fixes will happen in beta so that's where we should focus without expanding testing too much
  • opt-in instead of opt-out for supports_bidi is better control in our use case

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

@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 May 23, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Reorganize Bazel test tags with bidi opt-in and test categorization

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add bidi opt-in flag to chrome-beta, edge, and firefox-beta browsers
• Implement bidi_only parameter to generate bidi-only test targets
• Remove redundant skip-rbe tags from ie, safari, and safari-preview
• Reorganize test categorization with OS-sensitive, bidi-dual, and bidi-only test groups
• Update unit test tag from no-sandbox to unit for better filtering
Diagram
flowchart LR
  A["Browser Config"] -->|"Add bidi flag"| B["chrome-beta, edge, firefox-beta"]
  C["Test Generation"] -->|"bidi_only param"| D["Conditional target creation"]
  E["Tag Cleanup"] -->|"Remove skip-rbe"| F["Use target_compatible_with"]
  G["Test Organization"] -->|"Categorize tests"| H["OS-sensitive, Bidi-dual, Bidi-only"]

Loading

File Changes

1. rb/spec/tests.bzl ✨ Enhancement +45/-45

Add bidi opt-in and conditional test target generation

• Add bidi: True flag to chrome-beta, edge, and firefox-beta browser configurations
• Remove skip-rbe tags from ie, safari, and safari-preview (rely on target_compatible_with)
• Add bidi_only parameter to rb_integration_test function
• Conditionally generate classic and remote test targets only when bidi_only is False
• Update bidi target generation logic to check both bidi tag and browser's bidi flag
• Change unit test tag from no-sandbox to unit

rb/spec/tests.bzl


2. rb/spec/integration/selenium/webdriver/BUILD.bazel ✨ Enhancement +53/-32

Reorganize test targets with categorization and bidi handling

• Define test categorization lists: _OS_SENSITIVE_TESTS, _BIDI_DUAL_TESTS, _BIDI_ONLY_TESTS,
 _DEVTOOLS_TESTS
• Generate OS-sensitive test targets with os-sensitive tag
• Generate bidi-dual test targets with bidi tag (exercise both classic and bidi modes)
• Generate bidi-only test targets with bidi_only=True parameter
• Consolidate devtools and driver tests into loop-based generation
• Remove explicit devtools, driver, and element test definitions

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


3. rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel ✨ Enhancement +2/-5

Migrate bidi tests to bidi_only parameter

• Change from tags=["bidi", "exclusive-if-local"] to bidi_only=True parameter
• Remove devtools dependency, keep only webdriver:bidi
• Simplify tag configuration to only exclusive-if-local

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


View more (3)
4. rb/spec/integration/selenium/webdriver/chrome/BUILD.bazel ✨ Enhancement +1/-0

Add os-sensitive tag to chrome service test

• Add os-sensitive tag to chrome service test

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


5. rb/spec/integration/selenium/webdriver/edge/BUILD.bazel ✨ Enhancement +1/-0

Add os-sensitive tag to edge service test

• Add os-sensitive tag to edge service test

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


6. rb/spec/integration/selenium/webdriver/firefox/BUILD.bazel ✨ Enhancement +1/-0

Add os-sensitive tag to firefox service test

• Add os-sensitive tag to firefox service test

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


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 23, 2026

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (6)

Grey Divider


Action required

1. Unquoted rm -rf $path 📘 Rule violation ⛨ Security
Description
The new cleanup script calls sudo rm -rf -- $path without quoting/array handling, so paths
containing spaces or unexpected values can be split into multiple arguments and delete unintended
locations. This violates the safe argument-handling requirement for CI/scripts.
Code

scripts/github-actions/free-disk-space.sh[R15-18]

Evidence
PR Compliance ID 14 requires safe argument handling in scripts. The new script deletes using an
unquoted variable (sudo rm -rf -- $path), which can be split into multiple arguments and is not
robust for CI safety.

scripts/github-actions/free-disk-space.sh[15-18]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/github-actions/free-disk-space.sh` deletes paths using `sudo rm -rf -- $path` where `$path` is unquoted, allowing word-splitting (and potentially unintended deletions) when the value contains spaces or is otherwise malformed.

## Issue Context
The `clean` function receives values from both hardcoded paths and environment variables (e.g., `${CHROMEWEBDRIVER:-}`), so robust quoting/argument handling is required.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.sh[7-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. BiDi targets missing bidi tag 📘 Rule violation ≡ Correctness
Description
The new bidi_only test targets no longer carry the bidi tag, which can break existing Bazel
workflows/CI that select BiDi tests via --test_tag_filters=bidi. This is a backward-incompatible,
externally observable change in test target tagging/selection behavior.
Code

rb/spec/tests.bzl[R239-243]

Evidence
Compliance requires avoiding backward-incompatible externally observable behavior changes. The
rb_integration_test() macro’s -bidi target uses `tags = ... + universal_tags +
["{browser}-bidi"], so if callers use bidi_only = True without also including bidi in tags`,
the resulting BiDi targets will not be discoverable by existing bidi tag filters; this is shown in
the updated BUILD files that set bidi_only = True but do not include bidi in tags.

AGENTS.md: Maintain API/ABI compatibility for user-facing changes
rb/spec/tests.bzl[239-250]
rb/spec/integration/selenium/webdriver/BUILD.bazel[87-97]
rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-12]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rb_integration_test()` generates `-bidi` targets whose `tags` do not include `bidi` unless the caller explicitly passes `tags = ["bidi", ...]`. With the new `bidi_only = True` usage, callers often omit `tags`, so BiDi-only targets become untagged and may be skipped by existing tag-filtered CI runs.

## Issue Context
- `rb/spec/integration/selenium/webdriver/BUILD.bazel` creates BiDi-only test targets via `bidi_only = True` without `tags = ["bidi"]`.
- `rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel` similarly sets `bidi_only = True` and removes the `bidi` tag.
- `rb/spec/tests.bzl` currently builds the `-bidi` target tags from `universal_tags`, which may not include `bidi`.

## Fix Focus Areas
- rb/spec/tests.bzl[239-250]
- rb/spec/integration/selenium/webdriver/BUILD.bazel[87-97]
- rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. DevTools deps missing BiDi 🐞 Bug ≡ Correctness
Description
The devtools integration test target no longer depends on //rb/lib/selenium/webdriver:bidi, but
devtools_spec.rb references Selenium::WebDriver::DevTools which autoloads
selenium/webdriver/devtools from that library. This can cause devtools_spec targets to fail to load
at runtime (e.g., LoadError for selenium/webdriver/devtools) during integration runs.
Code

rb/spec/integration/selenium/webdriver/BUILD.bazel[R100-107]

Evidence
The DevTools spec references DevTools under Selenium::WebDriver, which is autoloaded from
selenium/webdriver/devtools. Bazel only includes that file in the
//rb/lib/selenium/webdriver:bidi library (not in :common), but the DevTools test target’s deps
list does not include :bidi, and spec_helper also doesn’t bring it in transitively;
additionally, //rb/lib/selenium/devtools doesn’t provide the webdriver devtools wrapper sources.

rb/spec/integration/selenium/webdriver/BUILD.bazel[99-109]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[22-30]
rb/lib/selenium/webdriver.rb[37-41]
rb/lib/selenium/webdriver/BUILD.bazel[18-28]
rb/spec/integration/selenium/webdriver/BUILD.bazel[17-27]
rb/lib/selenium/devtools/BUILD.bazel[12-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`devtools_spec.rb` uses `Selenium::WebDriver::DevTools`, which is autoloaded from `selenium/webdriver/devtools`. In Bazel, that Ruby source is provided by the `//rb/lib/selenium/webdriver:bidi` target, but the DevTools integration test targets no longer include this dependency, so the spec can fail to load.

### Issue Context
- The DevTools integration tests currently only add `//rb/lib/selenium/devtools`, which does not provide `selenium/webdriver/devtools.rb`.
- `//rb/spec/integration/selenium/webdriver:spec_helper` also does not depend on `//rb/lib/selenium/webdriver:bidi`, so the missing file is not brought in transitively.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/BUILD.bazel[99-109]

### Proposed fix
Update the `_DEVTOOLS_TESTS` `rb_integration_test(...)` deps to include `//rb/lib/selenium/webdriver:bidi` (keeping `//rb/lib/selenium/devtools` if still desired).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Safari *-bidi targets removed 📘 Rule violation ⚙ Maintainability ⭐ New
Description
The rb_integration_test macro now only generates *-bidi targets when BROWSERS[browser]["bidi"]
is set, which excludes safari and removes previously-generated *-safari-bidi targets. This is a
backward-incompatible change for any CI/scripts that reference or pattern-match those Bazel test
targets.
Code

rb/spec/tests.bzl[R239-240]

Evidence
PR Compliance ID 1 requires avoiding breaking changes to public interfaces. The change at
rb/spec/tests.bzl[239-240] adds a BROWSERS[browser].get("bidi", False) gate; since
DEFAULT_BROWSERS includes safari and the safari entry has no bidi flag, BiDi targets like
*-safari-bidi will no longer be generated for BiDi-only specs.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
rb/spec/tests.bzl[239-240]
rb/spec/tests.bzl[169-169]
rb/spec/tests.bzl[143-154]
rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-13]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rb_integration_test` now gates BiDi target generation behind `BROWSERS[browser].get("bidi", False)`, which removes `*-<browser>-bidi` targets for browsers like `safari` that previously had them generated whenever a test used the `bidi` tag.

## Issue Context
This can break downstream Bazel invocations/CI that explicitly run (or glob/pattern-match) `*-safari-bidi` (and similar) targets.

## Fix Focus Areas
- rb/spec/tests.bzl[239-257]
- rb/spec/tests.bzl[143-154]
- rb/spec/tests.bzl[169-169]
- rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-13]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. rb_unit_test tag changed 📘 Rule violation ⚙ Maintainability ⭐ New
Description
rb_unit_test now tags tests with unit instead of the previous no-sandbox tag, which can break
existing Bazel workflows that filter/select unit tests by tag. This is an externally observable
behavior change in the test macro interface.
Code

rb/spec/tests.bzl[R265-269]

Evidence
PR Compliance ID 1 requires avoiding breaking changes to public interfaces. The macro rb_unit_test
is a shared interface for defining tests, and changing its emitted tags alters how external Bazel
invocations can select tests.

AGENTS.md: Maintain API/ABI Compatibility for Public Interfaces
rb/spec/tests.bzl[259-270]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `rb_unit_test` macro changed its tag from `no-sandbox` to `unit`, which may break any existing `--test_tag_filters=no-sandbox` usage.

## Issue Context
This is a macro-level behavior change that can affect CI and developer workflows.

## Fix Focus Areas
- rb/spec/tests.bzl[259-270]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Local tags skipped on BiDi 🐞 Bug ≡ Correctness ⭐ New
Description
In rb_integration_test, filtered “local-only” tags are added to classic local targets via
local_tags, but BiDi targets omit local_tags even though BiDi targets are also local (non-Grid)
executions. This makes tag-based selection inconsistent between local and *-bidi targets for any tag
routed through _split_filtered_tags (e.g., os-sensitive).
Code

rb/spec/tests.bzl[R198-243]

Evidence
The macro explicitly computes local_tags and applies them to the classic local test target, but
the BiDi target’s tags list is built without local_tags. The BiDi target also uses the local
browser env/data pattern (no Grid jar, no WD_SPEC_DRIVER=remote), so omitting local_tags is
inconsistent with the stated “local-only” filtering intent.

rb/spec/tests.bzl[171-179]
rb/spec/tests.bzl[197-237]
rb/spec/tests.bzl[239-257]
rb/spec/integration/selenium/webdriver/BUILD.bazel[63-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rb_integration_test` splits tags into `universal_tags` and `local_tags`, and applies `local_tags` to the classic local target but not to the BiDi target. Since the BiDi target is also a local execution variant (it does not set `WD_SPEC_DRIVER=remote` or include Grid artifacts), any tag intentionally routed to `local_tags` (currently `os-sensitive`) will never be present on `*-bidi` targets.

## Issue Context
This PR introduces `_BROWSER_TAG_FILTERS`/`_split_filtered_tags` and uses `os-sensitive` tags in integration BUILD files.

## Fix Focus Areas
- rb/spec/tests.bzl[198-257]

## Suggested change
Update the BiDi target tag construction to include `local_tags` similarly to the classic local target, e.g.:

```starlark
# for BiDi target
tags = COMMON_TAGS + BROWSERS[browser]["tags"] + universal_tags + local_tags + ["{}-bidi".format(browser)]
```

If BiDi targets are intentionally excluded from “local-only” tags, clarify that intent in code comments and/or rename the filter concept to reflect that it applies only to the classic local target.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (5)
7. Cleanup script swallows delete errors 📘 Rule violation ☼ Reliability
Description
The new PowerShell cleanup script uses Remove-Item ... -ErrorAction SilentlyContinue, which can
hide failures during cleanup and make CI behavior less deterministic and harder to debug. This
conflicts with the guideline to avoid error-swallowing in build/CI scripts.
Code

scripts/github-actions/free-disk-space.ps1[R10-13]

Evidence
PR Compliance ID 14 calls out avoiding error-swallowing in CI/scripts. The newly added PowerShell
script explicitly suppresses errors with -ErrorAction SilentlyContinue during deletion, which can
conceal failures and complicate diagnosing CI disk issues.

scripts/github-actions/free-disk-space.ps1[10-13]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/github-actions/free-disk-space.ps1` uses `-ErrorAction SilentlyContinue` when deleting directories, masking unexpected failures and reducing determinism/debuggability.

## Issue Context
This script runs in GitHub Actions and its output is intended to explain disk cleanup behavior; hidden failures undermine that goal.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.ps1[10-18]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Safari retries permanently disabled 🐞 Bug ☼ Reliability
Description
In GlobalTestEnv#create_driver!, @safari_pairing_attempts is initialized with ||= and only reset on
successful driver creation, so if Safari hits the pairing error and exhausts retries, the counter
remains at the limit and subsequent Safari pairing errors will never retry within that process.
Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R205-210]

Evidence
The counter is only set with ||= at method entry and is reset to 0 only on the success path; the
retry helper blocks retries when the counter reaches the limit, and there is no reset on the final
failure path, so later invocations can start already "at limit".

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[202-226]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[252-265]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GlobalTestEnv#create_driver!` uses `@safari_pairing_attempts ||= 0` and increments it in `safari_pairing_retry?`. If driver creation ultimately fails (after retries are exhausted), the instance variable is never reset, so later calls to `create_driver!` start with `@safari_pairing_attempts >= SAFARI_PAIRING_RETRIES` and will not retry at all.

### Issue Context
This retry mechanism is meant to smooth over a transient SafariDriver error (`"instance is already paired"`). The counter should apply to retries *within one create attempt*, but should not poison subsequent driver-creation attempts.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[202-226]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[252-265]

### Suggested fix
After the `retry if safari_pairing_retry?(e)` line (i.e., on the non-retried failure path), reset the counter before recording/raising the error:

```rb
rescue StandardError => e
 retry if safari_pairing_retry?(e)
 @safari_pairing_attempts = 0
 @create_driver_error = e
 ...
 raise
end
```

(Alternative: track attempts in a local variable rather than an instance variable.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. DevTools dep not propagated 🐞 Bug ☼ Reliability
Description
After splitting selenium/webdriver/devtools into its own rb_library, the Chromium/Chrome/Edge Bazel
library targets do not depend on it, so calling DriverExtensions::HasDevTools#devtools can raise a
LoadError when autoload tries to resolve 'selenium/webdriver/devtools'. The top-level
//rb:selenium-webdriver gem target does include the new devtools library, but direct consumers of
//rb/lib/selenium/webdriver:{chromium,chrome,edge} won't get DevTools sources transitively.
Code

rb/lib/selenium/webdriver/BUILD.bazel[R20-32]

Evidence
Chromium::Driver installs the HasDevTools extension, which constructs Selenium::WebDriver::DevTools;
that constant is autoloaded from selenium/webdriver/devtools, but the Chromium Bazel target depends
only on :common and therefore does not guarantee devtools.rb is on the load path unless the caller
adds an explicit dependency (the PR only adds it at the gem target level).

rb/lib/selenium/webdriver/chromium/driver.rb[28-43]
rb/lib/selenium/webdriver/common/driver_extensions/has_devtools.rb[30-37]
rb/lib/selenium/webdriver.rb[37-41]
rb/lib/selenium/webdriver/BUILD.bazel[18-35]
rb/lib/selenium/webdriver/BUILD.bazel[37-44]
rb/BUILD.bazel[76-91]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Selenium::WebDriver::Chromium::Driver` includes `DriverExtensions::HasDevTools`, which instantiates `Selenium::WebDriver::DevTools` (autoloaded from `selenium/webdriver/devtools`). This PR split `selenium/webdriver/devtools` into a separate Bazel target (`//rb/lib/selenium/webdriver:devtools`), but `//rb/lib/selenium/webdriver:chromium` (and thus `:chrome`/`:edge`) does not depend on it, so Bazel-built Ruby code that depends directly on the browser libraries can hit `LoadError` when calling `driver.devtools`.

## Issue Context
The gem build target (`//rb:selenium-webdriver`) now includes `//rb/lib/selenium/webdriver:devtools`, which may mask this for gem consumers, but Bazel-style library consumption should remain correct/explicit.

## Fix Focus Areas
- Ensure Chromium-based libraries bring in the DevTools implementation transitively by adding `:devtools` as a dependency of `rb_library(name = "chromium", ...)` (or alternatively add it to both `:chrome` and `:edge`).

- rb/lib/selenium/webdriver/BUILD.bazel[18-44]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. BiDi network tests skipped 🐞 Bug ≡ Correctness
Description
With the new rb_integration_test BiDi gating, Ruby BiDi targets are only generated for browsers
opting into BiDi (currently chrome-beta/firefox-beta/edge), but network_spec.rb explicitly excludes
version 'beta', so it will be skipped on Chrome/Firefox BiDi runs and effectively only exercise
Edge.
Code

rb/spec/tests.bzl[R219-220]

Evidence
The PR changes BiDi target generation to require a browser opt-in flag; chrome-beta/firefox-beta opt
in and also set WD_BROWSER_VERSION=beta. The Network spec excludes version 'beta', and the spec
guard system keys :version off WD_BROWSER_VERSION, so Network will be skipped on the only
Chrome/Firefox BiDi targets this PR generates.

rb/spec/tests.bzl[36-60]
rb/spec/tests.bzl[108-131]
rb/spec/tests.bzl[171-237]
rb/spec/integration/selenium/webdriver/BUILD.bazel[41-97]
rb/spec/integration/selenium/webdriver/network_spec.rb[22-27]
rb/spec/integration/selenium/webdriver/spec_helper.rb[66-79]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[59-70]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`network_spec.rb` is configured to skip when `GlobalTestEnv.browser_version == 'beta'`, but this PR’s Bazel logic generates BiDi test targets only for browsers that opt into BiDi (chrome-beta/firefox-beta/edge). This combination makes the Network BiDi test unreachable on Chrome/Firefox (skipped at runtime) and reduces intended coverage.

### Issue Context
- BiDi targets are now gated by `BROWSERS[browser].get('bidi', False)` and enabled via `bidi_only`.
- `chrome-beta` and `firefox-beta` set `WD_BROWSER_VERSION=beta`, which is used by the RSpec guard system.

### How to fix
Pick one consistent intent and implement it:
1) If Network BiDi should run on beta (likely given this PR’s intent): remove/relax the beta exclusion in `rb/spec/integration/selenium/webdriver/network_spec.rb`.
2) If Network BiDi must not run on beta: ensure there is at least one non-beta BiDi target generated (e.g., opt `chrome`/`firefox` into BiDi in `rb/spec/tests.bzl`, or adjust the BiDi browser selection used for this spec).

### Fix Focus Areas
- rb/spec/tests.bzl[12-237]
- rb/spec/integration/selenium/webdriver/BUILD.bazel[41-97]
- rb/spec/integration/selenium/webdriver/network_spec.rb[20-30]
- rb/spec/integration/selenium/webdriver/spec_helper.rb[66-79]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[59-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


11. # tests require bidi comment 📘 Rule violation ⚙ Maintainability
Description
New comments in the BUILD file narrate what the lists contain (e.g., # tests require bidi enabled)
rather than documenting the rationale/constraints for the split. This reduces maintainability
because future readers won’t know the underlying intent (e.g., why these tests must be
separated/tagged) when modifying tags/targets.
Code

rb/spec/integration/selenium/webdriver/BUILD.bazel[R36-45]

Evidence
PR Compliance ID 9 requires comments to explain rationale/intent rather than restating behavior. The
added comments # tests exercise both the classic and bidi code paths, and `# tests require bidi
enabled` describe what the lists contain instead of why these tests are separated into special
target groups.

AGENTS.md: Comments should explain why, not what
rb/spec/integration/selenium/webdriver/BUILD.bazel[36-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Comments added in `rb/spec/integration/selenium/webdriver/BUILD.bazel` explain *what* the lists are (e.g., “tests require bidi enabled”) rather than *why* they need special handling.

## Issue Context
Compliance requires comments to capture rationale/intent so future changes don’t accidentally reintroduce redundant/no-op targets or mis-tag tests.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/BUILD.bazel[36-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 0d5a406

Results up to commit e29341c


🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)


Remediation recommended
1. # tests require bidi comment 📘 Rule violation ⚙ Maintainability
Description
New comments in the BUILD file narrate what the lists contain (e.g., # tests require bidi enabled)
rather than documenting the rationale/constraints for the split. This reduces maintainability
because future readers won’t know the underlying intent (e.g., why these tests must be
separated/tagged) when modifying tags/targets.
Code

rb/spec/integration/selenium/webdriver/BUILD.bazel[R36-45]

Evidence
PR Compliance ID 9 requires comments to explain rationale/intent rather than restating behavior. The
added comments # tests exercise both the classic and bidi code paths, and `# tests require bidi
enabled` describe what the lists contain instead of why these tests are separated into special
target groups.

AGENTS.md: Comments should explain why, not what
rb/spec/integration/selenium/webdriver/BUILD.bazel[36-45]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Comments added in `rb/spec/integration/selenium/webdriver/BUILD.bazel` explain *what* the lists are (e.g., “tests require bidi enabled”) rather than *why* they need special handling.

## Issue Context
Compliance requires comments to capture rationale/intent so future changes don’t accidentally reintroduce redundant/no-op targets or mis-tag tests.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/BUILD.bazel[36-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. BiDi network tests skipped 🐞 Bug ≡ Correctness
Description
With the new rb_integration_test BiDi gating, Ruby BiDi targets are only generated for browsers
opting into BiDi (currently chrome-beta/firefox-beta/edge), but network_spec.rb explicitly excludes
version 'beta', so it will be skipped on Chrome/Firefox BiDi runs and effectively only exercise
Edge.
Code

rb/spec/tests.bzl[R219-220]

Evidence
The PR changes BiDi target generation to require a browser opt-in flag; chrome-beta/firefox-beta opt
in and also set WD_BROWSER_VERSION=beta. The Network spec excludes version 'beta', and the spec
guard system keys :version off WD_BROWSER_VERSION, so Network will be skipped on the only
Chrome/Firefox BiDi targets this PR generates.

rb/spec/tests.bzl[36-60]
rb/spec/tests.bzl[108-131]
rb/spec/tests.bzl[171-237]
rb/spec/integration/selenium/webdriver/BUILD.bazel[41-97]
rb/spec/integration/selenium/webdriver/network_spec.rb[22-27]
rb/spec/integration/selenium/webdriver/spec_helper.rb[66-79]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[59-70]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`network_spec.rb` is configured to skip when `GlobalTestEnv.browser_version == 'beta'`, but this PR’s Bazel logic generates BiDi test targets only for browsers that opt into BiDi (chrome-beta/firefox-beta/edge). This combination makes the Network BiDi test unreachable on Chrome/Firefox (skipped at runtime) and reduces intended coverage.

### Issue Context
- BiDi targets are now gated by `BROWSERS[browser].get('bidi', False)` and enabled via `bidi_only`.
- `chrome-beta` and `firefox-beta` set `WD_BROWSER_VERSION=beta`, which is used by the RSpec guard system.

### How to fix
Pick one consistent intent and implement it:
1) If Network BiDi should run on beta (likely given this PR’s intent): remove/relax the beta exclusion in `rb/spec/integration/selenium/webdriver/network_spec.rb`.
2) If Network BiDi must not run on beta: ensure there is at least one non-beta BiDi target generated (e.g., opt `chrome`/`firefox` into BiDi in `rb/spec/tests.bzl`, or adjust the BiDi browser selection used for this spec).

### Fix Focus Areas
- rb/spec/tests.bzl[12-237]
- rb/spec/integration/selenium/webdriver/BUILD.bazel[41-97]
- rb/spec/integration/selenium/webdriver/network_spec.rb[20-30]
- rb/spec/integration/selenium/webdriver/spec_helper.rb[66-79]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[59-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 2e670fb


🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)


Action required
1. DevTools deps missing BiDi 🐞 Bug ≡ Correctness
Description
The devtools integration test target no longer depends on //rb/lib/selenium/webdriver:bidi, but
devtools_spec.rb references Selenium::WebDriver::DevTools which autoloads
selenium/webdriver/devtools from that library. This can cause devtools_spec targets to fail to load
at runtime (e.g., LoadError for selenium/webdriver/devtools) during integration runs.
Code

rb/spec/integration/selenium/webdriver/BUILD.bazel[R100-107]

Evidence
The DevTools spec references DevTools under Selenium::WebDriver, which is autoloaded from
selenium/webdriver/devtools. Bazel only includes that file in the
//rb/lib/selenium/webdriver:bidi library (not in :common), but the DevTools test target’s deps
list does not include :bidi, and spec_helper also doesn’t bring it in transitively;
additionally, //rb/lib/selenium/devtools doesn’t provide the webdriver devtools wrapper sources.

rb/spec/integration/selenium/webdriver/BUILD.bazel[99-109]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[22-30]
rb/lib/selenium/webdriver.rb[37-41]
rb/lib/selenium/webdriver/BUILD.bazel[18-28]
rb/spec/integration/selenium/webdriver/BUILD.bazel[17-27]
rb/lib/selenium/devtools/BUILD.bazel[12-16]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`devtools_spec.rb` uses `Selenium::WebDriver::DevTools`, which is autoloaded from `selenium/webdriver/devtools`. In Bazel, that Ruby source is provided by the `//rb/lib/selenium/webdriver:bidi` target, but the DevTools integration test targets no longer include this dependency, so the spec can fail to load.

### Issue Context
- The DevTools integration tests currently only add `//rb/lib/selenium/devtools`, which does not provide `selenium/webdriver/devtools.rb`.
- `//rb/spec/integration/selenium/webdriver:spec_helper` also does not depend on `//rb/lib/selenium/webdriver:bidi`, so the missing file is not brought in transitively.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/BUILD.bazel[99-109]

### Proposed fix
Update the `_DEVTOOLS_TESTS` `rb_integration_test(...)` deps to include `//rb/lib/selenium/webdriver:bidi` (keeping `//rb/lib/selenium/devtools` if still desired).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. BiDi targets missing bidi tag 📘 Rule violation ≡ Correctness
Description
The new bidi_only test targets no longer carry the bidi tag, which can break existing Bazel
workflows/CI that select BiDi tests via --test_tag_filters=bidi. This is a backward-incompatible,
externally observable change in test target tagging/selection behavior.
Code

rb/spec/tests.bzl[R239-243]

Evidence
Compliance requires avoiding backward-incompatible externally observable behavior changes. The
rb_integration_test() macro’s -bidi target uses `tags = ... + universal_tags +
["{browser}-bidi"], so if callers use bidi_only = True without also including bidi in tags`,
the resulting BiDi targets will not be discoverable by existing bidi tag filters; this is shown in
the updated BUILD files that set bidi_only = True but do not include bidi in tags.

AGENTS.md: Maintain API/ABI compatibility for user-facing changes
rb/spec/tests.bzl[239-250]
rb/spec/integration/selenium/webdriver/BUILD.bazel[87-97]
rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-12]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`rb_integration_test()` generates `-bidi` targets whose `tags` do not include `bidi` unless the caller explicitly passes `tags = ["bidi", ...]`. With the new `bidi_only = True` usage, callers often omit `tags`, so BiDi-only targets become untagged and may be skipped by existing tag-filtered CI runs.

## Issue Context
- `rb/spec/integration/selenium/webdriver/BUILD.bazel` creates BiDi-only test targets via `bidi_only = True` without `tags = ["bidi"]`.
- `rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel` similarly sets `bidi_only = True` and removes the `bidi` tag.
- `rb/spec/tests.bzl` currently builds the `-bidi` target tags from `universal_tags`, which may not include `bidi`.

## Fix Focus Areas
- rb/spec/tests.bzl[239-250]
- rb/spec/integration/selenium/webdriver/BUILD.bazel[87-97]
- rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel[3-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 30e20fb


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)


Remediation recommended
1. DevTools dep not propagated 🐞 Bug ☼ Reliability
Description
After splitting selenium/webdriver/devtools into its own rb_library, the Chromium/Chrome/Edge Bazel
library targets do not depend on it, so calling DriverExtensions::HasDevTools#devtools can raise a
LoadError when autoload tries to resolve 'selenium/webdriver/devtools'. The top-level
//rb:selenium-webdriver gem target does include the new devtools library, but direct consumers of
//rb/lib/selenium/webdriver:{chromium,chrome,edge} won't get DevTools sources transitively.
Code

rb/lib/selenium/webdriver/BUILD.bazel[R20-32]

Evidence
Chromium::Driver installs the HasDevTools extension, which constructs Selenium::WebDriver::DevTools;
that constant is autoloaded from selenium/webdriver/devtools, but the Chromium Bazel target depends
only on :common and therefore does not guarantee devtools.rb is on the load path unless the caller
adds an explicit dependency (the PR only adds it at the gem target level).

rb/lib/selenium/webdriver/chromium/driver.rb[28-43]
rb/lib/selenium/webdriver/common/driver_extensions/has_devtools.rb[30-37]
rb/lib/selenium/webdriver.rb[37-41]
rb/lib/selenium/webdriver/BUILD.bazel[18-35]
rb/lib/selenium/webdriver/BUILD.bazel[37-44]
rb/BUILD.bazel[76-91]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Selenium::WebDriver::Chromium::Driver` includes `DriverExtensions::HasDevTools`, which instantiates `Selenium::WebDriver::DevTools` (autoloaded from `selenium/webdriver/devtools`). This PR split `selenium/webdriver/devtools` into a separate Bazel target (`//rb/lib/selenium/webdriver:devtools`), but `//rb/lib/selenium/webdriver:chromium` (and thus `:chrome`/`:edge`) does not depend on it, so Bazel-built Ruby code that depends directly on the browser libraries can hit `LoadError` when calling `driver.devtools`.

## Issue Context
The gem build target (`//rb:selenium-webdriver`) now includes `//rb/lib/selenium/webdriver:devtools`, which may mask this for gem consumers, but Bazel-style library consumption should remain correct/explicit.

## Fix Focus Areas
- Ensure Chromium-based libraries bring in the DevTools implementation transitively by adding `:devtools` as a dependency of `rb_library(name = "chromium", ...)` (or alternatively add it to both `:chrome` and `:edge`).

- rb/lib/selenium/webdriver/BUILD.bazel[18-44]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 6a4c0ba


🐞 Bugs (1) 📘 Rule violations (2) 📎 Requirement gaps (0)


Action required
1. Unquoted rm -rf $path 📘 Rule violation ⛨ Security
Description
The new cleanup script calls sudo rm -rf -- $path without quoting/array handling, so paths
containing spaces or unexpected values can be split into multiple arguments and delete unintended
locations. This violates the safe argument-handling requirement for CI/scripts.
Code

scripts/github-actions/free-disk-space.sh[R15-18]

Evidence
PR Compliance ID 14 requires safe argument handling in scripts. The new script deletes using an
unquoted variable (sudo rm -rf -- $path), which can be split into multiple arguments and is not
robust for CI safety.

scripts/github-actions/free-disk-space.sh[15-18]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/github-actions/free-disk-space.sh` deletes paths using `sudo rm -rf -- $path` where `$path` is unquoted, allowing word-splitting (and potentially unintended deletions) when the value contains spaces or is otherwise malformed.

## Issue Context
The `clean` function receives values from both hardcoded paths and environment variables (e.g., `${CHROMEWEBDRIVER:-}`), so robust quoting/argument handling is required.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.sh[7-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
2. Safari retries permanently disabled 🐞 Bug ☼ Reliability
Description
In GlobalTestEnv#create_driver!, @safari_pairing_attempts is initialized with ||= and only reset on
successful driver creation, so if Safari hits the pairing error and exhausts retries, the counter
remains at the limit and subsequent Safari pairing errors will never retry within that process.
Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R205-210]

Evidence
The counter is only set with ||= at method entry and is reset to 0 only on the success path; the
retry helper blocks retries when the counter reaches the limit, and there is no reset on the final
failure path, so later invocations can start already "at limit".

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[202-226]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[252-265]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GlobalTestEnv#create_driver!` uses `@safari_pairing_attempts ||= 0` and increments it in `safari_pairing_retry?`. If driver creation ultimately fails (after retries are exhausted), the instance variable is never reset, so later calls to `create_driver!` start with `@safari_pairing_attempts >= SAFARI_PAIRING_RETRIES` and will not retry at all.

### Issue Context
This retry mechanism is meant to smooth over a transient SafariDriver error (`"instance is already paired"`). The counter should apply to retries *within one create attempt*, but should not poison subsequent driver-creation attempts.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[202-226]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[252-265]

### Suggested fix
After the `retry if safari_pairing_retry?(e)` line (i.e., on the non-retried failure path), reset the counter before recording/raising the error:

```rb
rescue StandardError => e
 retry if safari_pairing_retry?(e)
 @safari_pairing_attempts = 0
 @create_driver_error = e
 ...
 raise
end
```

(Alternative: track attempts in a local variable rather than an instance variable.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Cleanup script swallows delete errors 📘 Rule violation ☼ Reliability
Description
The new PowerShell cleanup script uses Remove-Item ... -ErrorAction SilentlyContinue, which can
hide failures during cleanup and make CI behavior less deterministic and harder to debug. This
conflicts with the guideline to avoid error-swallowing in build/CI scripts.
Code

scripts/github-actions/free-disk-space.ps1[R10-13]

Evidence
PR Compliance ID 14 calls out avoiding error-swallowing in CI/scripts. The newly added PowerShell
script explicitly suppresses errors with -ErrorAction SilentlyContinue during deletion, which can
conceal failures and complicate diagnosing CI disk issues.

scripts/github-actions/free-disk-space.ps1[10-13]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`scripts/github-actions/free-disk-space.ps1` uses `-ErrorAction SilentlyContinue` when deleting directories, masking unexpected failures and reducing determinism/debuggability.

## Issue Context
This script runs in GitHub Actions and its output is intended to explain disk cleanup behavior; hidden failures undermine that goal.

## Fix Focus Areas
- scripts/github-actions/free-disk-space.ps1[10-18]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors Ruby Bazel test tagging and target generation to better separate classic vs BiDi execution, reduce no-op targets, and introduce new tags for OS-sensitive and unit tests.

Changes:

  • Adds BiDi opt-in metadata per browser and a bidi_only option to generate only BiDi targets where applicable.
  • Introduces os-sensitive tagging for selected integration specs and marks certain service specs as OS-sensitive.
  • Replaces Ruby unit test tagging from no-sandbox to unit, and removes redundant skip-rbe tags where target_compatible_with already prevents execution.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
rb/spec/tests.bzl Adds bidi_only behavior and browser BiDi opt-in; adjusts unit test tags
rb/spec/integration/selenium/webdriver/firefox/BUILD.bazel Adds os-sensitive tag to Firefox service integration test
rb/spec/integration/selenium/webdriver/edge/BUILD.bazel Adds os-sensitive tag to Edge service integration test
rb/spec/integration/selenium/webdriver/chrome/BUILD.bazel Adds os-sensitive tag to Chrome service integration test
rb/spec/integration/selenium/webdriver/BUILD.bazel Splits integration specs into OS-sensitive, BiDi-dual, BiDi-only, and devtools groups
rb/spec/integration/selenium/webdriver/bidi/BUILD.bazel Switches BiDi integration specs to bidi_only targets

Comment thread rb/spec/tests.bzl
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 23, 2026

Persistent review updated to latest commit 2e670fb

Comment thread rb/spec/tests.bzl
Comment thread rb/spec/integration/selenium/webdriver/BUILD.bazel
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 24, 2026

Persistent review updated to latest commit 30e20fb

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 25, 2026

Persistent review updated to latest commit 6a4c0ba

Comment thread scripts/github-actions/free-disk-space.sh
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 25, 2026

Persistent review updated to latest commit 0d5a406

@titusfortner titusfortner merged commit 3b1c490 into trunk May 25, 2026
38 checks passed
@titusfortner titusfortner deleted the bazel_tags_rb branch May 25, 2026 14:30
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants