ci: add single step to validate if all tests passed #11763
Conversation
WalkthroughThe GitHub Actions workflow was renamed from "Commit Validation" to "Tests", the file-pattern filter was narrowed to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer Push
participant GH as GitHub Actions
participant Build as build
participant Coverage as coverage
participant Docs as docs
participant Format as formatting
participant TL as tests-linux
participant TW as tests-windows
participant All as all-passed
Dev->>GH: push triggers `Tests` workflow (test*.yml)
GH->>Build: start build
GH->>Coverage: start coverage
GH->>Docs: start docs
GH->>Format: start formatting
GH->>TL: start tests-linux
GH->>TW: start tests-windows
note right of All `#D6EAF8`: Aggregator job\n(always runs)
All->>Build: wait for completion
All->>Coverage: wait for completion
All->>Docs: wait for completion
All->>Format: wait for completion
All->>TL: wait for completion
All->>TW: wait for completion
alt any dependent job failed
All-->>GH: exit 1 (failure aggregate)
else all dependencies succeeded
All-->>GH: exit 0 (success)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
michaelbromley
left a comment
There was a problem hiding this comment.
Note on why this PR exists:
We noticed that it was possible to merge a PR even when checks were failing or not yet completed. The only restriction to prevent merge was the 2 review minimum.
So I then altered the settings to require all the tests to pass before one can merge.
However, this then means that targeted workflows that do not run tests (e.g. PRs just on docs that don't need to run tests) would not be mergable, since the tests didn't run.
Apparently GH Actions does not support this level of control natively.
So this solution allows us to require just this one single job "all-passed" to succeed, and then merge is allowed. In the case of e.g. docs PRs where tests are skipped, the "all-passed" will still succeed because it only fails on "failure" in any of the dependent jobs, and will ignore "skipped" jobs.
# Conflicts: # .github/workflows/tests.yml
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: tests-linux (20) / mssql
- GitHub Check: tests-linux (20) / mysql_mariadb_latest
- GitHub Check: tests-linux (20) / mysql_mariadb
- GitHub Check: tests-linux (20) / sap
- GitHub Check: tests-linux (20) / sqlite
- GitHub Check: tests-linux (20) / postgres (17)
- GitHub Check: tests-linux (20) / postgres (14)
- GitHub Check: tests-linux (20) / cockroachdb
- GitHub Check: tests-linux (20) / oracle
- GitHub Check: tests-linux (18) / postgres (17)
- GitHub Check: tests-linux (18) / postgres (14)
- GitHub Check: tests-linux (18) / sap
- GitHub Check: tests-linux (18) / oracle
- GitHub Check: tests-linux (18) / mssql
- GitHub Check: tests-linux (18) / mysql_mariadb_latest
- GitHub Check: tests-linux (18) / mysql_mariadb
- GitHub Check: tests-windows / better-sqlite3
- GitHub Check: tests-windows / sqljs
- GitHub Check: tests-windows / sqlite
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
34-34: Confirm this behavior change is intentional—prior logic explicitly avoided triggering tests on workflow changes.The pattern
test*.ymlis safe and specific (matches onlytests.yml,tests-linux.yml,tests-windows.yml), with no cascading or circular dependency risks detected. However, git history shows commit4955db92explicitly decided to "dont run tests on workflow changes," and this PR reverses that decision. Verify this policy shift is intentional before merging.
| all-passed: | ||
| runs-on: ubuntu-latest | ||
| if: always() | ||
| needs: | ||
| - build | ||
| - coverage | ||
| - docs | ||
| - formatting | ||
| - tests-linux | ||
| - tests-windows | ||
| steps: | ||
| - run: exit ${{ contains(needs.*.result, 'failure') && 1 || 0 }} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the needs.*.result syntax for aggregating job statuses.
The all-passed job uses contains(needs.*.result, 'failure') to aggregate the results of 6 dependent jobs. While the intent is clear—exit with code 1 if any job failed, 0 otherwise—confirm that the needs.*.result expansion correctly evaluates to an array containing all job results in your GitHub Actions version.
A few observations:
- The job correctly uses
if: always()to ensure it runs regardless of upstream failures. - Skipped jobs (due to conditional
ifstatements on upstream jobs) will not appear as 'failure', so they won't block the check. This is correct for selective test execution. - The job depends on all major test/validation jobs, ensuring comprehensive coverage for branch protection rules.
Alternatively, for more explicit error reporting, you could check each job individually: exit ${{ (needs.build.result == 'failure' || needs.coverage.result == 'failure' || ...) && 1 || 0 }}. However, the current approach is concise and appropriate if the needs.*.result array expansion works as expected.
🏁 Script executed:
#!/bin/bash
# Search for existing examples of needs.*.result usage in the repository
rg -nP 'needs\.\*\.result' .github/workflows/Length of output: 102
🌐 Web query:
GitHub Actions needs.*.result syntax job results array
💡 Result:
Short answer: needs.<job_id>.result is a string (one of "success", "failure", "cancelled", "skipped") for a specific dependent job. The needs context is an object keyed by job id (needs.jobA, needs.jobB, ...), not an ordered array — there is no built-in needs.*.result array to iterate. To check multiple job results you must reference each job (e.g. needs.build.result, needs.test.result) or explicitly list/collect outputs into a single job output that you parse. [1][2]
Example:
- Access single job result: ${{ needs.build.result }}.
- No array form like needs.*.result exists; to aggregate, write step outputs to a job output and read/parse that output in the downstream job. [1][2]
Sources:
- GitHub Actions contexts reference (needs and jobs contexts). [docs].
- Examples/explanations of job outputs and needs usage.
Fix: needs.*.result is not valid GitHub Actions syntax and must be replaced with explicit job result checks.
The needs context in GitHub Actions is an object keyed by job id (not an array), so needs.*.result does not exist. The contains() function will not receive an actual array of results, and the aggregation will fail.
Line 127: Replace with explicit job-by-job checks:
- run: exit ${{ (needs.build.result == 'failure' || needs.coverage.result == 'failure' || needs.docs.result == 'failure' || needs.formatting.result == 'failure' || needs.tests-linux.result == 'failure' || needs.tests-windows.result == 'failure') && 1 || 0 }}Alternatively, use a reusable step or structured output approach to avoid the verbose conditional.
🤖 Prompt for AI Agents
.github/workflows/tests.yml around lines 116 to 127: the step using
contains(needs.*.result, 'failure') is invalid because needs is an object keyed
by job id, not an array; replace that expression with explicit per-job result
checks (e.g., check needs.build.result, needs.coverage.result,
needs.docs.result, needs.formatting.result, needs.tests-linux.result,
needs.tests-windows.result combined with || to produce exit code 1 if any are
'failure'), or alternatively implement a reusable step/aggregate output that
computes a single boolean result and reference that instead.
Description of change
Add single step to validate if all tests passed, which will be used for branch protection rules
Pull-Request Checklist
masterbranchFixes #00000Summary by CodeRabbit