Skip to content

Commit 4492d01

Browse files
vaadin-botZheSun88claude
authored
ci: ensure test-results runs on cancellation so auto-merge blocks (#24290) (CP: 25.0) (#24301)
This PR cherry-picks changes from the original PR #24290 to branch 25.0. --- #### Original PR description > ## Summary > > Cherry-pick PRs targeting release branches were being merged even when > a matrix `it-tests (N, ...)` shard had been cancelled (e.g. by the > 30-min job timeout when CI hung). Example: [#24286](#24286) > merged at 11:49 UTC two seconds after `it-tests (2)` was cancelled at > 11:48:58, with `test-results` showing as `SKIPPED`. > > ## Why this happened > > `test-results` is registered as a required status check on the 24.10 > branch protection. The intent is that it acts as the single chokepoint > representing "all matrix it-tests passed" (since the matrix job names > include the dynamic module list and can't be required individually). > > The job's gate today is: > > ```yaml > test-results: > ... > if: ${{ failure() || success() }} > needs: [unit-tests, it-tests] > ``` > > GitHub Actions `if:` semantics on a `needs:` chain treat > `failure() || success()` as covering only success and failure — **not > cancellation**. So when any matrix shard is cancelled, the > `test-results` job's gate evaluates to false → SKIPPED. > > A skipped required status check > [satisfies the requirement in GitHub's evaluation](https://github.com/orgs/community/discussions/13690), > so the auto-merge bot proceeds and merges the PR. > > The job's existing `Set Failure Status` step (line 268) is already > written correctly with `if: always() && (... .result != 'success')` — > it just never gets a chance to run because the whole job is skipped > first. > > ## Fix > > Change the job-level gate to `if: ${{ always() }}` so `test-results` > always runs (success / failure / cancelled / timed out). The existing > `Set Failure Status` step then explicitly fails the job when any > upstream needs entry is in a non-success state, including > `cancelled`. The required check turns **red** instead of grey, > auto-merge correctly blocks on it. > > Tiny one-liner; comment added so the next person to touch this gate > understands why `always()` is required. > > ## Test plan > > - [ ] Push a draft PR with an artificially cancelled it-tests shard > (or wait for the next natural occurrence) and confirm > `test-results` runs and reports failure rather than being > skipped. > - [ ] Confirm the auto-merge bot does *not* merge a cherry-pick PR > while `test-results` is failing. > > ## Follow-up > > This change should be cherry-picked to `25.0`, `25.1`, and `main` > once verified here. The same one-liner applies on every branch that > carries `validation.yml`. > > 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Zhe Sun <zhe@vaadin.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Zhe Sun <31067185+ZheSun88@users.noreply.github.com>
1 parent b9b5632 commit 4492d01

1 file changed

Lines changed: 5 additions & 1 deletion

File tree

.github/workflows/validation.yml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,11 @@ jobs:
229229
issues: read
230230
checks: write
231231
pull-requests: write
232-
if: ${{ failure() || success() }}
232+
# Use always() so this job runs on cancellation too. With
233+
# failure() || success() it would be skipped when any matrix entry
234+
# was cancelled, and a skipped required check satisfies branch
235+
# protection — letting auto-merge proceed past a cancelled IT.
236+
if: ${{ always() }}
233237
needs: [unit-tests, it-tests]
234238
runs-on: ubuntu-24.04
235239
steps:

0 commit comments

Comments
 (0)