Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 30, 2025

User description

🔗 Related Issues

JS portion of #16809

💥 What does this PR do?

  • ci.yml sends the output of needs.check.outputs.targets to ci-javascript.yml
  • ci-javascript.yml:
    • filter job to get what applies to javascript; (defaults to everything if targets not found)
    • runs just those targets on Windows
  • check-bazel-targets.sh:
    • ensure errors are managed
    • limit where we are looking for test targets to just one of the bindings (this helps with external-cache)
    • ensure results are unique

💡 Additional Considerations

  • Ideally these get filtered further (chrome-only, or "smoke tests"), but this is the simplest place to start
  • the check job is generating all the external-cache, even if no job actually uses the things it is caching.
  • We can minimize cache generated without changing targets by using cquery and a bunch of other flags (--//common:pin_browsers=false --keep_going --output=starlark --starlark:expr='target.label' --notool_deps --implicit_deps=false --include_aspects=false), but this is simplest to start. Even better, running cquery job on windows instead of linux would dramatically reduce cache issues.

PR Type

Enhancement


Description

  • Add JavaScript CI workflow to run applicable tests on Windows

  • Filter JavaScript targets from check job output before execution

  • Improve bazel-targets script with error handling and deduplication

  • Add skip-rbe tag to JavaScript lint tests for RBE optimization


Diagram Walkthrough

flowchart LR
  A["check job<br/>generates targets"] -->|"outputs.targets"| B["ci-javascript.yml<br/>filter-targets"]
  B -->|"filtered targets"| C["browser-tests job<br/>Windows"]
  D["check-bazel-targets.sh<br/>improvements"] -->|"unique targets"| A
  E["BUILD.bazel<br/>skip-rbe tags"] -->|"optimization"| C
Loading

File Walkthrough

Relevant files
Enhancement
check-bazel-targets.sh
Improve target detection with error handling and deduplication

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

  • Add error handling with set -euo pipefail for robust script execution
  • Limit bazel target queries to binding universe roots to reduce
    external cache overhead
  • Exclude manual test tags from results
  • Deduplicate target list using sort and mapfile
+23/-14 
ci-javascript.yml
New JavaScript CI workflow with target filtering                 

.github/workflows/ci-javascript.yml

  • New workflow file for JavaScript CI triggered via workflow_call
  • Filter targets to only include //javascript/selenium-webdriver/* paths
  • Default to full JavaScript universe if no targets provided
  • Run browser tests on Windows with bazel test command
+54/-0   
ci.yml
Integrate JavaScript workflow into main CI pipeline           

.github/workflows/ci.yml

  • Add JavaScript job that calls ci-javascript.yml workflow
  • Pass check job targets output to JavaScript workflow
  • Trigger JavaScript job on schedule, dispatch, workflow_call, or
    JavaScript-related changes
+13/-0   
Configuration changes
BUILD.bazel
Add skip-rbe tags to lint tests                                                   

javascript/selenium-webdriver/BUILD.bazel

  • Add skip-rbe tag to eslint_test target for RBE optimization
  • Add skip-rbe tag to prettier_test target for RBE optimization
+2/-0     

@selenium-ci selenium-ci added C-nodejs JavaScript Bindings B-build Includes scripting, bazel and CI integrations labels Dec 30, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 30, 2025

PR Compliance Guide 🔍

(Compliance updated until commit 2f3f991)

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
🔴 Prevent or resolve the repeated ChromeDriver instantiation console error "Error:
ConnectFailure (Connection refused)" after the first instance.
Provide a fix or guidance applicable to the reported environment (Ubuntu 16.04.4, Selenium
3.9.0, Chrome 65, ChromeDriver 2.35).
🟡
🎫 #1234
🔴 Ensure that click() triggers JavaScript contained in a link's href as it did in 2.47.1
(regression in 2.48.x) on Firefox (notably Firefox 42).
Verify/fix behavior using the provided reproduction artifacts (linked videos/test case).
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: Robust Error Handling and Edge Case Management

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

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: Security-First Input Validation and Data Handling

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

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:
Verbose driver logging: Expanding logging.Type.DRIVER to logging.Level.ALL under SELENIUM_VERBOSE may emit
additional unstructured driver logs to CI output that should be reviewed to ensure no
sensitive data can appear in logs.

Referred Code
  logging.getLogger(logging.Type.DRIVER).setLevel(logging.Level.ALL)
}

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

Previous compliance checks

Compliance check up to commit ee34970
Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #1234
🔴 Ensure JavaScript in a link href (e.g., javascript:...) is triggered when calling click()
in Selenium 2.48 (regression vs 2.47.1) on Firefox.
Reproduce and validate behavior on Firefox 42 (32-bit on 64-bit machine) as described in
provided test case/videos.
🟡
🎫 #5678
🔴 Investigate and fix repeated ChromeDriver instantiation causing "Error: ConnectFailure
(Connection refused)" after the first instance on Ubuntu 16.04 with Selenium 3.9.0 /
Chrome 65 / ChromeDriver 2.35.
Provide a resolution or mitigation for the connection refused errors seen on subsequent
ChromeDriver instances.
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:
Silent query failure: The script suppresses Bazel query errors and also appears to query the literal string file
instead of the changed file path, causing silent missing/incorrect targets rather than a
clear, actionable failure.

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
done

if (( ${#bazel_targets[@]} == 0 )); then
  echo "No bazel targets found after checking the diff."
  exit 0
fi

# Only consider test targets under the binding roots
BINDINGS_UNIVERSE="set(//java/... //py/... //rb/... //dotnet/... //javascript/selenium-webdriver/...)"

# Get test targets based on the changes
echo "Getting test targets..."
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


 ... (clipped 2 lines)

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 targets input: The workflow consumes inputs.targets and interpolates it into a shell loop/outputs without
explicit validation or strict quoting guarantees, which may be acceptable for trusted
workflow_call usage but needs confirmation of the caller trust boundary.

Referred Code
on:
  workflow_call:
    inputs:
      targets:
        required: false
        type: string
        default: ''
  workflow_dispatch:

permissions:
  contents: read

jobs:
  filter-targets:
    name: Filter Targets
    runs-on: ubuntu-latest
    outputs:
      targets: ${{ steps.filter.outputs.targets }}
    steps:
    - name: Filter JavaScript targets
      id: filter


 ... (clipped 14 lines)

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

@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
Query actual changed filename

In the bazel query command, replace the literal string "file" with the variable
"$file" to correctly query each changed file.

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

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

__

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

High
General
Safely split input targets

To safely handle multi-line or space-containing targets from inputs.targets,
read them into an array with a newline IFS before iterating.

.github/workflows/ci-javascript.yml [26-31]

-targets="${{ inputs.targets }}"
+IFS=$'\n' read -r -a target_array <<< "${{ inputs.targets }}"
 filtered=()
 
-for t in $targets; do
+for t in "${target_array[@]}"; do
   [[ "$t" == //javascript/selenium-webdriver* ]] && filtered+=("$t")
 done
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that word-splitting on $targets is not robust. Using read -r -a with a specific IFS is a good practice for handling multi-line or space-containing inputs correctly.

Low
  • Update

@titusfortner titusfortner force-pushed the js_gha branch 3 times, most recently from af41175 to ecdfdd4 Compare January 5, 2026 17:14
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor comment.

strategy:
fail-fast: false
matrix:
nodejs-version: ['20.19.5', '24.11.1']
Copy link
Member

Choose a reason for hiding this comment

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

Why not 22 instead of 20?

Copy link
Member Author

@titusfortner titusfortner Jan 8, 2026

Choose a reason for hiding this comment

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

Because 20 doesn't EOL until April, and we say we still support it.
We want to test the newest and oldest to make sure nothing is broken in either.
I'd use 25 instead of 24 if the current version of bazel rules supported it.
In May we switch to 22 and 26

@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 C-nodejs JavaScript Bindings Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants