[rb] ensure ruby tests are properly linted

[titusfortner]

💥 What does this PR do?

• RuboCop now lints every Ruby file under rb/spec/ — several specs had silently escaped it.

🔧 Implementation Notes

• Removed the hand-maintained list of test targets, because it wasn't getting updated and it wasn't obvious it needed to be updated.
• A single glob can't replace the list: Bazel's glob() stops at package boundaries, and each spec directory with its own BUILD is a separate package, so each such BUILD defines its own recursive-glob filegroup, staged as data since filegroups aren't Ruby libraries and can't be deps.
• Because of the glob, any new .rb file added under such a package is linted automatically.
• Existing offenses autocorrected; no spec behavior changed.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code (Claude Opus)
  • What was generated: the //rb/spec:spec filegroup restructure and the RuboCop autocorrections
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• Coverage of new spec packages isn't enforced: they must include an all_srcs filegroup and be added to //rb/spec:spec's data list.

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Bazel: glob all rb/spec sources into //rb/spec:spec for RuboCop

🐞 Bug fix ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Stage all rb/spec/**.rb sources into //rb/spec:spec so RuboCop lints everything.
• Replace the hand-maintained spec target list with per-package all_srcs filegroups.
• Auto-correct existing RuboCop offenses in integration specs (no behavior changes).

Diagram

graph TD
  A["New spec file (*.rb)"] --> B["Spec package BUILD"] --> C["all_srcs (glob)"] --> D["//rb/spec:spec (data)"] --> E["RuboCop lint"] --> F["Offenses reported"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Collapse subpackages (remove nested BUILD files)

• ➕ Single top-level glob would cover everything automatically with no per-package list.
• ➕ Less repetition (no per-directory all_srcs targets).
• ➖ High blast radius: changes Bazel package boundaries and potentially target labels/visibility.
• ➖ May require reorganizing existing test rules/macros that currently rely on subpackages.

2. Standardize via a Starlark macro + enforcement

• ➕ Keeps current package structure but reduces copy/paste for defining all_srcs.
• ➕ Can add a presubmit/buildifier-style check to ensure new packages define all_srcs and are wired into //rb/spec:spec.
• ➖ Still cannot eliminate the need to reference each package from the aggregator (Bazel glob boundary limitation).
• ➖ Requires introducing/maintaining shared macro and enforcement wiring.

Recommendation: The chosen approach (per-package all_srcs + //rb/spec:spec staging them via data) is the lowest-risk way to make RuboCop coverage complete while respecting Bazel package boundaries. If new spec packages are expected to grow, consider a small macro to standardize all_srcs definitions and a presubmit check to prevent forgetting to add the new package to //rb/spec:spec.

Files changed (12) +80 / -43

Refactor (3) +10 / -8

driver_finder_spec.rbRuboCop formatting: wrap pending_if metadata cleanly +3/-2

RuboCop formatting: wrap pending_if metadata cleanly

• Applies RuboCop-driven line wrapping/indentation to the 'pending_if'/'skip_unless' metadata arrays without changing test semantics.

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb

fedcm_spec.rbRuboCop formatting: reflow long describe and pending_if lines +3/-2

RuboCop formatting: reflow long describe and pending_if lines

• Reflows long 'describe' and 'pending_if' argument lists to satisfy linting and improve readability; behavior remains unchanged.

rb/spec/integration/selenium/webdriver/fedcm_spec.rb

network_spec.rbRuboCop formatting: reorder keyword args and align indentation +4/-4

RuboCop formatting: reorder keyword args and align indentation

• Reorders 'pending_if'/'skip_unless' keyword arguments and adjusts indentation to match RuboCop style expectations; no behavioral changes.

rb/spec/integration/selenium/webdriver/network_spec.rb

Other (9) +70 / -35

BUILD.bazelStage spec sources for RuboCop via all_srcs data +14/-35

Stage spec sources for RuboCop via all_srcs data

• Replaces a large, hand-maintained list of spec rule deps with a 'data' list of per-package ':all_srcs' filegroups. Adds inline documentation explaining Bazel package-boundary limitations and how new packages must be added.

rb/spec/BUILD.bazel

BUILD.bazelAdd integration all_srcs filegroup +7/-0

Add integration all_srcs filegroup

• Introduces 'filegroup(name = "all_srcs")' using 'glob(["**/*.rb"])' so the top-level spec aggregator can stage integration sources for linting.

rb/spec/integration/BUILD.bazel

BUILD.bazelExpose webdriver package Ruby sources via all_srcs +7/-0

Expose webdriver package Ruby sources via all_srcs

• Adds an 'all_srcs' filegroup (globbed Ruby files) for the webdriver integration package so RuboCop can lint sources in this Bazel package.

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

BUILD.bazelExpose BiDi integration Ruby sources via all_srcs +7/-0

Expose BiDi integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' to enable cross-package source staging for RuboCop.

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

BUILD.bazelExpose Chrome integration Ruby sources via all_srcs +7/-0

Expose Chrome integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' so linting includes Chrome-specific integration specs.

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

BUILD.bazelExpose Edge integration Ruby sources via all_srcs +7/-0

Expose Edge integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' so linting includes Edge-specific integration specs.

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

BUILD.bazelExpose Firefox integration Ruby sources via all_srcs +7/-0

Expose Firefox integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' so linting includes Firefox-specific integration specs.

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

BUILD.bazelExpose Remote integration Ruby sources via all_srcs +7/-0

Expose Remote integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' so linting includes Remote/Grid integration specs.

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

BUILD.bazelExpose Safari integration Ruby sources via all_srcs +7/-0

Expose Safari integration Ruby sources via all_srcs

• Adds a per-package 'all_srcs' filegroup with 'glob(["**/*.rb"])' so linting includes Safari-specific integration specs.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Stale RuboCop comment ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

rb/spec/tests.bzl still states that rb_integration_test generates rb_library targets used by
//rb/spec:spec to expose tests to //rb:rubocop, but this PR changes //rb/spec:spec to stage
sources via data filegroups (:all_srcs) instead. This mismatch will mislead maintainers working
on the Bazel/RuboCop integration.

Code

rb/spec/BUILD.bazel[R10-26]

+# RuboCop lints only what Bazel stages for it, so this bundles every spec's
+# sources. glob() can't cross package boundaries, so each spec package exposes an
+# :all_srcs filegroup; new files in a listed package are covered automatically,
+# a new package needs a line added here.
rb_library(
    name = "spec",
    testonly = True,
+    data = [
+        "//rb/spec/integration:all_srcs",
+        "//rb/spec/integration/selenium/webdriver:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/bidi:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/chrome:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/edge:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/firefox:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/remote:all_srcs",
+        "//rb/spec/integration/selenium/webdriver/safari:all_srcs",
+    ],

Evidence

//rb/spec:spec was changed in this PR to stage spec sources through data filegroups
(:all_srcs), while rb_integration_test still claims its generated rb_library is used by
//rb/spec:spec for RuboCop—this is no longer true after the PR change.

rb/spec/BUILD.bazel[10-26]
rb/spec/tests.bzl[193-198]

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/spec/tests.bzl` contains a comment describing the old mechanism for exposing spec sources to RuboCop via generated `rb_library` targets and `//rb/spec:spec` deps. After this PR, `//rb/spec:spec` stages spec sources via per-package `:all_srcs` `filegroup`s added to `data`, so the comment is no longer accurate.

## Issue Context
- `//rb/spec:spec` now collects spec sources using `data = ["//rb/spec/...:all_srcs", ...]`.
- `rb_integration_test` still documents that it creates an `rb_library` “used by //rb/spec:spec to expose all tests to //rb:rubocop”, which is now misleading.

## Fix Focus Areas
- rb/spec/tests.bzl[193-198]
- rb/spec/BUILD.bazel[10-26]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

Code review by qodo was updated up to the latest commit 046b0ad