Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 30, 2025

User description

🔗 Related Issues

Ruby implementation of #16809

We are mostly relying on RBE to tell us if there is a problem, this is supposed to be extra information that strikes a good balance between information and execution time.

💥 What does this PR do?

  • ci.yml kicks off on every commit. It runs the new and improved check-bazel-targets.sh

  • Instead of just checking to see if there are any rb targets and running the whole ci-ruby workflow, the unique set of applicable test targets are passed from the script to ci-ruby.yml and browser tests are only run on those

  • Additionally, docs/steep/unit tests are only run if the PR includes "[rb]" or is executed manually (passed into ci-ruby.yml from ci.yml with a run-full-suite argument.

  • Even if tagged [rb], if only a subset of ruby tests are matched, only those will be run

  • Removed build instead of gating all tests on it. Most of the build is executed by the applicable tests as necessary. If there is something weird that breaks building, it'll get detected in nightly run

  • It's now 2 jobs instead of 6, but effective changes are:

    • Changes firefox/windows to firefox-beta/windows
    • Changes chrome/windows to chrome-beta/windows
    • Adds chrome-beta-remote/windows
    • Adds firefox-beta-remote/windows
    • Removes Edge Windows
    • Removes Edge Remote Windows
    • Removes Chrome Mac
    • Removes Chrome Firefox

Update: fixed so it run edge remote like previously and doesn't run chrome/firefox remote

🔧 Implementation Notes

I'm picking betas instead of production browsers as a way to get additional information earlier. Failures in new browser versions will still be managed in a PR updating them, not in trunk.

💡 Additional Considerations

This probably doesn't decrease the total amount of Ruby testing by enough, so we probably need to add more filters, but I think this is a good place to start.


PR Type

Enhancement, Tests


Description

  • Implement selective Ruby test filtering based on changed files

  • Limit test universe to binding roots for faster CI execution

  • Remove build job and consolidate to two browser test jobs

  • Filter test targets by [rb] tag and file changes


Diagram Walkthrough

flowchart LR
  A["Changed Files"] -->|"bazel query"| B["Affected Targets"]
  B -->|"Filter to //rb"| C["Ruby Targets"]
  C -->|"Check [rb] tag"| D["Run Full Suite"]
  D -->|"Yes"| E["Docs + Steep + Unit Tests"]
  D -->|"No"| F["Browser Tests Only"]
  E --> G["Filter Targets Job"]
  F --> G
  G -->|"//rb targets"| H["Browser Tests<br/>chrome-beta/firefox-beta"]
Loading

File Walkthrough

Relevant files
Enhancement
check-bazel-targets.sh
Restrict target universe and improve query logic                 

scripts/github-actions/check-bazel-targets.sh

  • Add strict error handling with set -euo pipefail
  • Limit bazel query universe to binding roots only
  • Filter out manual test tags from results
  • Return unique sorted set of test targets
  • Fix bazel query syntax to properly handle file-to-target mapping
+23/-14 
Configuration changes
ci-ruby.yml
Refactor workflow with selective test filtering                   

.github/workflows/ci-ruby.yml

  • Remove build job dependency from all test jobs
  • Add workflow inputs for targets and run-full-suite parameters
  • Introduce new filter-targets job to filter Ruby-specific targets
  • Consolidate browser tests to chrome-beta and firefox-beta on Windows
    only
  • Gate docs, steep, and unit tests on run-full-suite input
  • Update integration tests to use filtered targets output
+40/-52 
ci.yml
Wire target filtering to Ruby workflow inputs                       

.github/workflows/ci.yml

  • Pass filtered targets from check job to Ruby workflow
  • Add run-full-suite logic based on [rb] tag and event type
  • Remove commit message check for [rb] tag
  • Enable full suite for scheduled, manual, and workflow_call events
+9/-1     

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 30, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 30, 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 fix the repeated ChromeDriver instantiation issue that results in "Error:
ConnectFailure (Connection refused)" on subsequent driver creations (Ubuntu 16.04 /
Selenium 3.9.0 / Chrome 65 / ChromeDriver 2.35).
🟡
🎫 #1234
🔴 Restore/ensure that click() triggers JavaScript in a link's href for Firefox (regression
from 2.47.1 to 2.48.x) as shown in the provided reproduction.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Incorrect query input: The script attempts to map changed files to Bazel targets but calls bazel query with the
literal string file (and suppresses errors), which can silently produce incorrect/empty
target sets and cause missed test execution.

Referred Code
for file in $affected_files; do
  if query_output=$(bazel query --keep_going --noshow_progress "file" 2>/dev/null); then
    bazel_targets+=(${query_output})
  fi

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated workflow input: The workflow accepts inputs.targets and passes it into a shell loop without validation or
strict parsing, which may allow unexpected values to influence which Bazel targets are
executed.

Referred Code
targets="${{ inputs.targets }}"
filtered=()

for t in $targets; do
  [[ "$t" == //rb* ]] && filtered+=("$t")
done

if [ ${#filtered[@]} -eq 0 ]; then
  echo "targets=//rb/spec/..." >> "$GITHUB_OUTPUT"
else
  echo "targets=${filtered[*]}" >> "$GITHUB_OUTPUT"
fi

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 30, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix bazel query variable use

Fix the bazel query by using the $file variable to query the correct changed
file, instead of the literal string "file".

scripts/github-actions/check-bazel-targets.sh [18]

-if query_output=$(bazel query --keep_going --noshow_progress "file" 2>/dev/null); then
+if query_output=$(bazel query --keep_going --noshow_progress "${file}" 2>/dev/null); then
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: This suggestion identifies a critical bug where the script queries the literal string "file" instead of the filename variable, which would cause the target detection to fail.

High
Fix incorrect GitHub Actions output

Remove the single quotes when setting the bazel-targets output to prevent it
from being treated as a single string by downstream jobs.

scripts/github-actions/check-bazel-targets.sh [44]

-echo "bazel-targets='${bazel_targets[*]}'" | tee -a "$GITHUB_OUTPUT"
+echo "bazel-targets=${bazel_targets[*]}" >> "$GITHUB_OUTPUT"
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies and fixes a critical bug where single quotes in the GitHub Action output would break downstream jobs by passing the target list as a single string.

High
Consolidate and quote query string

Consolidate the multi-line bazel query into a single variable before execution
to improve readability and prevent potential syntax issues.

scripts/github-actions/check-bazel-targets.sh [33-37]

-if query_output=$(bazel query \
-  --keep_going \
-  --noshow_progress \
-  "kind(test, rdeps(${BINDINGS_UNIVERSE}, set(${bazel_targets[@]}))) \
-   except attr('tags','manual', ${BINDINGS_UNIVERSE})" 2>/dev/null); then
+query_expr="kind(test, rdeps(${BINDINGS_UNIVERSE}, set(${bazel_targets[*]}))) except attr('tags','manual', ${BINDINGS_UNIVERSE})"
+if query_output=$(bazel query --keep_going --noshow_progress "$query_expr" 2>/dev/null); then
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion improves code readability by consolidating a multi-line bazel query into a variable, but the original code is functionally correct.

Low
Learned
best practice
Validate and normalize workflow input

Normalize and validate inputs.targets before iterating (trim, handle newlines,
and avoid unsafe word-splitting) so empty/odd input formats don’t break
filtering.

.github/workflows/ci-ruby.yml [90-102]

 run: |
   targets="${{ inputs.targets }}"
   filtered=()
+
+  # Normalize whitespace/newlines into spaces
+  targets="$(printf '%s' "$targets" | tr '\n\t' '  ')"
 
   for t in $targets; do
     [[ "$t" == //rb* ]] && filtered+=("$t")
   done
 
   if [ ${#filtered[@]} -eq 0 ]; then
     echo "targets=//rb/spec/..." >> "$GITHUB_OUTPUT"
   else
     echo "targets=${filtered[*]}" >> "$GITHUB_OUTPUT"
   fi
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add explicit validation and null/availability guards at integration boundaries by trimming inputs and checking presence before use.

Low
  • Update

@titusfortner titusfortner force-pushed the rb_filter_tests branch 3 times, most recently from 311fdeb to eb0f5e2 Compare January 5, 2026 18:49
@titusfortner titusfortner marked this pull request as draft January 9, 2026 16:26
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 Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants