Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 30, 2025

User description

🔗 Related Issues

.NET implementation of #16809

💥 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 dotnet targets and running the whole ci-dotnet workflow, the unique set of applicable test targets are passed from the script to ci-dotnet.yml and browser tests are only run on those

To get all the tests passing I did:

  • Changed InstallCommand extension handling to support Windows better
  • Truncated the logs to make results easier to read (100 char to match Java), I didn't make it configurable, but we could
  • Submit() to use GetDomAttribute instead of GetAttribute (since we know it is an attribute)
  • A couple test fixes and guards

🔧 Implementation Notes

  • Current implementation is only 2 smoke tests on Windows, this is going to run all the tests for 3 browsers right now; I want to see what is passing right now before restricting it more.

💡 Additional Considerations

  • Total execution time for all of this is: 3h 38m 59s
  • We need define "smoke tests" for these and limit tests
  • We should also create Unit Tests
  • Hmm, and should try to run Safari just to make sure it works.

PR Type

Enhancement


Description

  • Add workflow inputs to ci-dotnet.yml for selective test execution

  • Implement target filtering logic to run only affected .NET tests

  • Expand browser test matrix to cover Chrome, Firefox, and Edge

  • Pass filtered targets from ci.yml to ci-dotnet.yml workflow


Diagram Walkthrough

flowchart LR
  A["ci.yml check job"] -->|"targets output"| B["ci-dotnet.yml"]
  B -->|"filter-targets job"| C["Filter .NET targets"]
  C -->|"filtered targets"| D["browser-tests matrix"]
  D -->|"Chrome/Firefox/Edge"| E["Run bazel tests"]
Loading

File Walkthrough

Relevant files
Enhancement
ci-dotnet.yml
Add selective test execution and browser matrix                   

.github/workflows/ci-dotnet.yml

  • Added workflow inputs targets and run-full-suite to support selective
    test execution
  • Introduced filter-targets job to filter applicable .NET targets from
    upstream workflow
  • Replaced hardcoded integration-tests job with parameterized
    browser-tests job using matrix strategy
  • Added browser matrix for Chrome, Firefox, and Edge with enhanced bazel
    test flags
+54/-6   
ci.yml
Pass filtered targets to .NET workflow                                     

.github/workflows/ci.yml

  • Pass targets output from check job to ci-dotnet.yml workflow
  • Add run-full-suite input to control full test suite execution based on
    event type
  • Removed commit message check from conditional, relying on PR title
    check instead
+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
🟡
🎫 #1234
🟡
🎫 #5678
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: 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: 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 passes needs.filter-targets.outputs.targets directly into a shell-executed
bazel test command, so a human should verify the upstream targets output cannot be
influenced to inject unexpected shell tokens/flags.

Referred Code
run: >
  bazel test
  --keep_going
  --build_tests_only
  --flaky_test_attempts 3
  --local_test_jobs 1
  --test_tag_filters=${{ matrix.browser }}
  --pin_browsers=false
  --test_env=SE_FORCE_BROWSER_DOWNLOAD=true
  --test_env=SE_SKIP_DRIVER_IN_PATH=true
  ${{ needs.filter-targets.outputs.targets }}

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
General
Simplify workflow input logic

Simplify the workflow invocation by removing the run-full-suite input and
instead passing an empty targets string to trigger a full suite run, delegating
the logic to the called workflow.

.github/workflows/ci.yml [57-63]

-run-full-suite: >-
+targets: >-
   ${{
-    github.event_name == 'schedule' ||
-    github.event_name == 'workflow_dispatch' ||
-    github.event_name == 'workflow_call' ||
-    contains(github.event.pull_request.title, '[dotnet]')
+    ( github.event_name == 'schedule' ||
+      github.event_name == 'workflow_dispatch' ||
+      github.event_name == 'workflow_call' ||
+      contains(github.event.pull_request.title, '[dotnet]')
+    ) && '' || needs.check.outputs.targets
   }}
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good refactoring suggestion that simplifies the interface between workflows by using a single targets input, making the logic cleaner and more maintainable.

Medium
Always run build job for caching

Modify the build job to always run, building the specific targets from
filter-targets to improve caching and speed up the workflow.

.github/workflows/ci-dotnet.yml [20-27]

 build:
   name: Build
-  if: ${{ inputs.run-full-suite }}
+  needs: filter-targets
   uses: ./.github/workflows/bazel.yml
   with:
     name: Build
     os: windows
-    run: bazel build //dotnet:all
+    run: bazel build ${{ needs.filter-targets.outputs.targets }}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a performance inefficiency and proposes a valid optimization to always build the required targets, which improves caching and reduces the runtime of the browser-tests job.

Low
Learned
best practice
Validate and sanitize workflow inputs

Avoid unquoted word-splitting and accept only well-formed Bazel labels to
prevent malformed/empty inputs or unexpected characters from being passed into
the command line.

.github/workflows/ci-dotnet.yml [38-50]

 run: |
   targets="${{ inputs.targets }}"
+  targets="$(echo "$targets" | xargs)"  # trim
   filtered=()
 
-  for t in $targets; do
-    [[ "$t" == //dotnet/* ]] && filtered+=("$t")
-  done
+  if [[ -n "$targets" ]]; then
+    read -r -a arr <<<"$targets"
+    for t in "${arr[@]}"; do
+      [[ "$t" =~ ^//dotnet/[^[:space:]]+$ ]] && filtered+=("$t")
+    done
+  fi
 
   if [ ${#filtered[@]} -eq 0 ]; then
-    echo "targets=//dotnet/..." >> "$GITHUB_OUTPUT"
+    printf 'targets=%s\n' '//dotnet/...' >>"$GITHUB_OUTPUT"
   else
-    echo "targets=${filtered[*]}" >> "$GITHUB_OUTPUT"
+    printf 'targets=%s\n' "${filtered[*]}" >>"$GITHUB_OUTPUT"
   fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (workflow inputs) by trimming and sanitizing before use.

Low
Possible issue
Quote test_tag_filters argument

Add quotes around the ${{ matrix.browser }} variable in the bazel test command
to prevent potential word-splitting issues.

.github/workflows/ci-dotnet.yml [71]

---test_tag_filters=${{ matrix.browser }}
+--test_tag_filters="${{ matrix.browser }}"
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential shell expansion issue and proposes adding quotes to ${{ matrix.browser }}, which improves the robustness of the script.

Low
  • Update

@titusfortner
Copy link
Member Author

@nvborisenko you were right, I didn't need that code. I must have fixed the issue with a different setting.

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.

4 participants