Skip to content

[build] dependency changes should run CI tests for given binding#17534

Merged
titusfortner merged 2 commits into
trunkfrom
deps_ci
May 20, 2026
Merged

[build] dependency changes should run CI tests for given binding#17534
titusfortner merged 2 commits into
trunkfrom
deps_ci

Conversation

@titusfortner
Copy link
Copy Markdown
Member

@titusfortner titusfortner commented May 20, 2026

💥 What does this PR do?

Currently the way ci.yml is detecting bazel targets based on file changes, it is only looking at the queryable target graph and the lockfiles aren't on that, so this adds an explicit override. So ci-<language> workflow will run.

🔧 Implementation Notes

I'm also deduping the other targets so we don't mix and match wildcards and explicit targets. Not that it matters to bazel, but logs will be more readable.

🤖 AI assistance

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

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add dependency lock file overrides for CI target detection

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add explicit target overrides for dependency lock files
• Lock files now trigger appropriate language-specific CI tests
• Deduplicate targets to avoid mixing wildcards with explicit targets
• Rename HIGH_IMPACT_DIRS to HIGH_IMPACT_PATHS for clarity
Diagram
flowchart LR
  A["Changed Files"] --> B{"Check HIGH_IMPACT_PATTERN"}
  B -->|Match| C["Run All Tests"]
  B -->|No Match| D{"Index Exists?"}
  D -->|Yes| E["Get Override Targets"]
  E --> F["Filter Index Results"]
  F --> G["Deduplicate & Combine"]
  G --> H["Return Targets"]
  D -->|No| I["Directory Fallback"]

Loading

File Changes

1. rake_tasks/bazel.rake ✨ Enhancement +18/-6

Add dependency lock file target overrides

• Added TARGET_OVERRIDES hash mapping dependency lock files to their bindings
• Implemented logic to extract override targets and filter index results to avoid duplication
• Renamed HIGH_IMPACT_DIRS to HIGH_IMPACT_PATHS and added MODULE.bazel to the list
• Updated comments to reflect terminology change from "dirs" to "paths"

rake_tasks/bazel.rake


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label May 20, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 20, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Implicit empty-union regex 🐞 Bug ⚙ Maintainability
Description
covered_pattern is always built via Regexp.union(override_targets.map { ... }), which becomes
non-obvious when override_targets is empty and makes the subsequent grep_v behavior easy to
misread and accidentally break during future edits. Make the empty-override case explicit so the
dedupe logic remains clear and robust.
Code

rake_tasks/bazel.rake[R46-49]

Evidence
The PR introduces override+dedupe logic that always constructs covered_pattern from
override_targets. When no override files are changed, override_targets is an empty array, so the
meaning of Regexp.union([]) becomes an implicit dependency that is not clear from the code path.

rake_tasks/bazel.rake[43-53]

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

### Issue description
`covered_pattern` is constructed even when `override_targets` is empty. While this can work due to Ruby `Regexp.union` edge-case behavior, the intent is not obvious and the logic is fragile to refactors.

### Issue Context
This code decides which Bazel targets to write into `bazel-targets.txt` for downstream CI workflow selection.

### Fix Focus Areas
- rake_tasks/bazel.rake[46-49]

### Suggested fix
Add an explicit branch:
- If `override_targets.empty?`, skip building `covered_pattern` and set `index_targets = affected_targets_with_index(...)` directly.
- Else, build `covered_pattern` and filter `index_targets` with `grep_v` as today.

This keeps behavior identical while making the logic self-explanatory.

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


Grey Divider

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

Updates the Bazel “affected targets” Rake task so CI reliably runs the correct binding test workflows when dependency/lock files change (even when those files are not covered by the Bazel-derived test-file index).

Changes:

  • Treat MODULE.bazel as a high-impact path to trigger full test runs.
  • Add explicit binding target overrides for key dependency/lock files and dedupe/avoid redundant indexed targets for the same binding.
  • Rename “dirs” terminology to “paths” for clarity and update related filtering logic.

@titusfortner titusfortner merged commit d25b215 into trunk May 20, 2026
40 of 41 checks passed
@titusfortner titusfortner deleted the deps_ci branch May 20, 2026 21:35
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants