[build] aggregate all lint errors instead of failing fast by titusfortner · Pull Request #17637 · SeleniumHQ/selenium

titusfortner · 2026-06-05T16:20:13Z

💥 What does this PR do?

Makes lint tasks report every failure in a single run instead of exiting at the first one.
Updates other multiple step tasks to use the new implementation instead of inline rescue/capture
Automatic console logging based on keyword

🔧 Implementation Notes

Hash of keyword to lambda instead of a new class

🤖 AI assistance

No substantial AI assistance used
AI assisted (complete below)
- Tool(s): Claude Code
- What was generated: the aggregate_errors helper and the lint/verify task refactors
- I reviewed all AI output and can explain the change

🔄 Types of changes

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

qodo-code-review · 2026-06-05T16:20:33Z

Review Summary by Qodo

Aggregate all lint errors instead of failing fast

✨ Enhancement 🐞 Bug fix

Walkthroughs

Description

• Introduces aggregate_errors helper to collect and report all failures
• Refactors lint tasks to report every error instead of failing fast
• Applies new pattern to dotnet, python, ruby, and root lint tasks
• Consolidates error handling across multiple language bindings

Diagram

flowchart LR
  A["Lint Tasks"] -->|Previously| B["Exit on First Error"]
  A -->|Now| C["aggregate_errors Helper"]
  C --> D["Collect All Failures"]
  D --> E["Report All Errors Together"]
  F["dotnet:lint"] --> C
  G["py:lint"] --> C
  H["rb:lint"] --> C
  I["all:lint"] --> C

File Changes

1. rake_tasks/common.rb ✨ Enhancement +14/-0

Add aggregate_errors helper method

• Adds new aggregate_errors method that accepts named steps as lambdas
• Executes all steps and collects failures instead of stopping at first error
• Automatically converts step names to readable labels with underscores to spaces
• Raises aggregated error message with count and details of all failures

rake_tasks/common.rb

2. rake_tasks/dotnet.rake ✨ Enhancement +9/-7

Refactor dotnet lint to aggregate errors

• Refactors dotnet:lint task to use aggregate_errors helper
• Converts inline Bazel executions to lambda-based steps dictionary
• Conditionally adds enforced diagnostics step only if diagnostics exist
• Removes manual puts statements in favor of automatic logging

rake_tasks/dotnet.rake

3. rake_tasks/python.rake ✨ Enhancement +5/-5

Refactor python lint to aggregate errors

• Refactors py:lint task to use aggregate_errors helper
• Converts three separate linting steps to lambda-based dictionary
• Removes manual puts statements for ruff check and mypy
• Consolidates error reporting for all python linters

rake_tasks/python.rake

View more (2)

4. rake_tasks/ruby.rake ✨ Enhancement +5/-5

Refactor ruby lint to aggregate errors

• Refactors rb:lint task to use aggregate_errors helper
• Converts rubocop, steep, and docs steps to lambda-based dictionary
• Removes manual puts statements for individual linters
• Preserves flag argument handling for rubocop options

rake_tasks/ruby.rake

5. Rakefile ✨ Enhancement +17/-24

Refactor all lint and verify tasks to aggregate errors

• Extracts shellcheck and actionlint logic into lint_actions helper method
• Refactors root lint task to use aggregate_errors for shell and all language linting
• Refactors all:check_credentials task to use aggregate_errors pattern
• Refactors all:verify task to use aggregate_errors pattern
• Refactors all:lint task to use aggregate_errors with dynamic language steps
• Removes manual failure collection and error raising logic throughout

Rakefile

qodo-code-review · 2026-06-05T16:20:34Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Prefix lost on multiline 🐞 Bug ◔ Observability

Description

SeleniumRake.aggregate_errors raises a multi-line string (failures.join("\n\n")), so when an
inner aggregate_errors failure is caught and re-wrapped by an outer aggregate_errors, only the
first line is prefixed with the outer step label and the remaining failure lines become
unlabeled/ambiguous in output.

Code

rake_tasks/common.rb[121]
+    raise failures.join("\n\n")

Evidence

The helper raises a multi-line message string; nested usage exists in the Rakefile (:lint runs
all:lint, and all:lint itself aggregates). When an exception message contains newlines,
string-prefixing only affects the first line visually, so later lines lose the outer label context.

rake_tasks/common.rb[110-122]
Rakefile[151-156]
Rakefile[256-262]

Agent prompt

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

### Issue description
`aggregate_errors` currently raises a multi-line string. When this exception is caught by another `aggregate_errors` wrapper (nested aggregation), the outer wrapper prefixes only the first line, leaving subsequent lines without the outer context label.

### Issue Context
This happens in the repo because top-level `:lint` wraps `all:lint`, and `all:lint` itself uses `aggregate_errors`, so nested aggregation is a real execution path.

### Fix Focus Areas
- rake_tasks/common.rb[110-122]

### Suggested implementation direction
- Introduce a dedicated exception type (e.g., `SeleniumRake::AggregateErrors < StandardError`) that carries `failures` as an array.
- In `aggregate_errors`, raise that exception instead of raising a joined string.
- When rescuing in `aggregate_errors`, detect `AggregateErrors` and either:
 - merge/flatten the nested `failures` into the parent failure list (preferred), or
 - re-raise without string-wrapping.
This preserves step labels across nested calls and keeps output unambiguous.

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

2. aggregate_errors drops backtraces 📘 Rule violation ◔ Observability

Description

SeleniumRake.aggregate_errors rescues StandardError and records only e.message, which discards
exception type and backtrace and can significantly reduce CI/debug diagnostics when a step fails.
This conflicts with the requirement for automation scripts to emit explicit diagnostics and fail
safely.

Code

rake_tasks/common.rb[R110-121]

+  def self.aggregate_errors(**steps)
+    failures = steps.filter_map do |name, closure|
+      label = name.to_s.tr('_', ' ')
+      puts "Running #{label}..." unless name == :all
+      closure.call
+      nil
+    rescue StandardError => e
+      "#{label}: #{e.message}"
+    end
+    return if failures.empty?
+
+    raise "#{failures.size} of #{steps.size} steps failed:\n\n#{failures.join("\n\n")}"

Evidence

Rule 11 requires automation scripts to emit explicit diagnostics on failures. The new helper rescues
exceptions and converts them to strings containing only e.message, which removes exception class
and backtrace that are often necessary to understand and fix CI failures.

rake_tasks/common.rb[110-121]
Best Practice: Learned patterns

Agent prompt

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

## Issue description
`SeleniumRake.aggregate_errors` aggregates failures but currently truncates exceptions to only `e.message`, losing exception class and backtrace. This makes CI failures harder to diagnose.

## Issue Context
This helper is now used by top-level `:lint` and `all:*` tasks, so reduced diagnostics affects common automation paths.

## Fix Focus Areas
- rake_tasks/common.rb[110-121]

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

3. Nested lint aggregation 🐞 Bug ◔ Observability

Description

The :lint task wraps all:lint (which already aggregates per-language failures) inside another
aggregate_errors call, so the top-level exception reports only one failed step (all) even when
multiple language linters failed. This double-wrapping makes CI/log output misleading and harder to
interpret/debug.

Code

Rakefile[R152-156]

+task :lint do
+  SeleniumRake.aggregate_errors(
+    shellcheck_and_actionlint: -> { lint_actions },
+    all: -> { Rake::Task['all:lint'].invoke }
+  )

Evidence

The outer :lint task aggregates two steps, one of which is `all: -> {
Rake::Task['all:lint'].invoke }, while all:lint` itself aggregates 5 language steps. Since
aggregate_errors formats its header using the immediate steps.size, multiple language failures
get collapsed under the single outer all step, producing a misleading top-level summary count.

Rakefile[146-157]
Rakefile[256-262]
rake_tasks/common.rb[110-122]

Agent prompt

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

### Issue description
`rake lint` currently aggregates errors twice: an outer `aggregate_errors` wraps an `all:` step that invokes `all:lint`, and `all:lint` itself uses `aggregate_errors` across languages. This leads to confusing nested summaries like `1 of 2 steps failed` (outer) while the real failures are inside the nested `all` message.

### Issue Context
The goal of this PR is to report *all* lint failures in one run. Nesting aggregation preserves details but makes the top-level summary count/labels misleading.

### Fix Focus Areas
- Rakefile[146-157]
- Rakefile[256-262]

### Suggested fix
Refactor to a single aggregation level for `rake lint`:
- Option A (recommended): In `task :lint`, build one `steps` hash that includes `shellcheck_and_actionlint` plus each language lint (`java`, `py`, `rb`, `dotnet`, `node`), and call `SeleniumRake.aggregate_errors(**steps)` once.
- Option B: Move `lint_actions` into `all:lint` (as an additional step) and make top-level `:lint` just invoke `all:lint` directly (no outer aggregation).

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

qodo-code-review · 2026-06-05T19:10:32Z

Code review by qodo was updated up to the latest commit ee5dcf0

qodo-code-review · 2026-06-05T19:22:26Z

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

[build] aggregate all lint errors instead of failing fast

c58a7d4

selenium-ci added the B-build Includes scripting, bazel and CI integrations label Jun 5, 2026

[build] remove counts from error message

ee5dcf0

titusfortner merged commit c779dcd into trunk Jun 5, 2026
25 of 26 checks passed

titusfortner deleted the aggregate_errors branch June 5, 2026 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[build] aggregate all lint errors instead of failing fast#17637

[build] aggregate all lint errors instead of failing fast#17637
titusfortner merged 2 commits into
trunkfrom
aggregate_errors

titusfortner commented Jun 5, 2026

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

Uh oh!

qodo-code-review Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

titusfortner commented Jun 5, 2026

💥 What does this PR do?

🔧 Implementation Notes

🤖 AI assistance

🔄 Types of changes

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-code-review Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

qodo-code-review Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Jun 5, 2026

CI Feedback 🧐

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review Bot commented Jun 5, 2026 •

edited

Loading

qodo-code-review Bot commented Jun 5, 2026 •

edited

Loading