RI-8373 Move the Tests label trigger into its own workflow - #6418
RI-8373 Move the Tests label trigger into its own workflow#6418pawelangelow wants to merge 5 commits into
Conversation
`pull_request` cannot filter on which label was added, so every label started a run of the whole suite just to skip it. Dependabot adds `dependencies` and `javascript` twice each when it opens a pull request, which left four runs of thirteen skipped checks on every dependency PR, several of them repeating the name of a job that had really run. `tests-on-demand.yml` now owns the `labeled` trigger and calls `tests.yml`. A label with no suite behind it skips a single `uses:` job and reports one check rather than thirteen. The four opt-in labels still take effect the moment they are added. This retires the `-noop-<run id>` concurrency suffix and the label conditions on `changes` and `lint`, which existed only to survive the trigger, and leaves the opt-in label list in one place instead of three. References: #RI-8373
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94bc5b5312
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A concurrency group is worked out before any job `if` is read, so sharing one with `tests.yml` meant a label with no suite behind it cancelled the run a push had started, before the `suites` job could find out it had nothing to do. Adding `dependencies` to a pull request killed the suite running on it. `tests-on-demand.yml` now has a group of its own and does not cancel. Dependabot adds several labels within seconds of opening a pull request, and a cancelled check does not count as passing for branch protection the way a skipped one does, so those runs queue and skip instead. An opt-in label no longer supersedes a run already going, so the two can overlap. Losing one duplicate suite is worth more than the cancellation. References: #RI-8373
pawelangelow
added
dependencies
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e35ef39822
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e35ef39. Configure here.
Code Coverage - Backend unit tests
Test suite run success3779 tests passing in 325 suites. Report generated by 🧪jest coverage report action from 9e0fae9 |
Code Coverage - Integration Tests
|
Code Coverage - Frontend unit tests
Test suite run success8032 tests passing in 871 suites. Report generated by 🧪jest coverage report action from 9e0fae9 |
pawelangelow
added
the
run-frontend-tests
Reading whether a pull request is mergeable means going through every check row and working out which skips are gating decisions and which mean something broke. On a dependency PR that is a couple of dozen rows for an answer that is one bit wide. `required-checks-passed` waits on the gate and suite jobs and fails when any of them failed or was cancelled. A skipped job passes, since a suite gated off by the changed files is a decision. On a red run the step prints the jobs at fault, and their own checks still hold the logs, so nothing is hidden and no row disappears. The housekeeping jobs stay out: coverage, deployment cleanup and artifact removal say nothing about whether the code is sound, and two of them are skipped on Dependabot runs by design. This adds the check but does not enforce it. `main` currently requires no status checks at all, so making this the required one is a branch protection change to follow. References: #RI-8373
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 008d2a302c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| group: tests-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
When a relevant run-* label is added while the direct PR workflow is running, the called tests.yml instance still resolves this to the same tests-<PR> group and, with cancel-in-progress: true, cancels the direct run. This matters because the workflow itself documents that only the direct run produces the required All required checks pass context, whereas the called run produces suites / All required checks pass; the label therefore leaves the required check cancelled and blocks merging until another push or direct trigger. Distinguish workflow_call executions in this group while preserving cancellation among direct PR runs.
Useful? React with 👍 / 👎.
The wrapper existed to cut the skipped rows a label leaves behind. `required-checks-passed` answers the question those rows were in the way of, so their count stops mattering and the extra workflow stops paying for itself. This restores the `labeled` trigger, the `-noop-<run id>` concurrency suffix and the label conditions on `changes` and `lint`, and drops `tests-on-demand.yml`. Reverts 94bc5b5 and e35ef39. References: #RI-8373
pawelangelow
removed
dependencies
The four `run-*-tests` labels exist for a suite a pull request's own changes did not call for: the frontend tests on a backend-only PR, or the other way round. They have no part in the run that opening the PR starts. `tests-on-demand.yml` now calls `tests-frontend.yml`, `tests-backend.yml` and `tests-integration.yml` directly, one job each, so a label runs the suite it names and nothing else. `tests.yml` keeps the `labeled` trigger out entirely. That separation is what keeps the merge gate honest. `tests.yml` runs `required-checks-passed` with `always()`, so a label reaching it would report a second `All required checks pass` on the same commit, green within seconds because every suite in that run skipped, while the real suite still had twenty minutes to go. Dependabot adds four labels when it opens a pull request, so this would happen on every dependency PR. `should-run` still reads the labels, so one stays in effect for later pushes. Only the trigger moves. This also retires the `-noop-<run id>` concurrency suffix and the label conditions on `changes` and `lint`, which existed to survive a trigger this workflow no longer has. References: #RI-8373
pawelangelow
added
dependencies
1 participant
What
pull_requestcannot filter on which label was added, so every label added to a PR started a run of the whole✅ Testssuite just to skip it. Dependabot addsdependenciesandjavascripttwice each when it opens a pull request, so every dependency PR carried four such runs of thirteen skipped checks, several of them repeating the name of a job that had really run under a different one. Telling at a glance whether the tests passed took effort.tests-on-demand.ymlnow owns thelabeledtrigger and callstests.yml. A label with no suite behind it skips a singleuses:job and reports one check rather than thirteen. The four opt-in labels (run-all-tests,run-frontend-tests,run-backend-tests,run-integration-tests) still take effect the moment they are added.This retires the
-noop-<run id>concurrency suffix and the label conditions onchangesandlint, which existed only to survive the trigger, and leaves the opt-in label list in one place instead of three.Testing
All of it is observable on this PR:
pull_requestruns workflow files from the PR's merge ref, so both files are live here.Note this PR touches
.github/workflows/**, which matches theinfrafilter, so every run on it is a full suite regardless of labels.labeledremoved and theifoffchangesandlint.dependencieswhile that suite is in flight. Expect one✅ Tests (on demand)run holding a single skippedsuitesjob, and the in-flight✅ Testsrun left alone.run-frontend-tests. Expectsuitesto run, with nested checks reported assuites / frontend-tests / Frontend tests.run-all-testsinside the window. Expect the push run to be cancelled and the on-demand run to carry on.Step 4 is the gate. Whether a called workflow's own top-level
concurrencyapplies is undocumented, so both files deliberately use the identical grouptests-<pr number>. If the on-demand run instead dies the moment it starts, the nestedtests.ymlis cancelling its own parent and the wrapper needs a group of its own.Refs #RI-8373
🤖 Generated with Claude Code
Note
Medium Risk
Changes CI triggers, concurrency, and the branch-protection check surface; misconfiguration could block merges or cancel the wrong runs, but application code is untouched.
Overview
Moves PR
labeledhandling out oftests.ymlinto a newtests-on-demand.ymlworkflow so unrelated labels (e.g. Dependabot’sdependencies/javascript) no longer spin up the full test workflow only to skip every job.The on-demand workflow runs on any label add, gates on the four opt-in labels (
run-all-tests,run-frontend-tests,run-backend-tests,run-integration-tests), and reusestests.ymlviaworkflow_call. Irrelevant labels produce one skipped check instead of many. It uses a separate concurrency group with cancel-in-progress disabled so noop label runs cannot cancel in-flight push-triggered test runs.tests.ymlnow triggers only on open/sync/reopen (plus existing dispatch/call paths). It drops the-noop-<run id>concurrency hack and the duplicated labelifguards onchangesandlint, and simplifies concurrency totests-<pr number>.Adds a
required-checks-passedjob (“All required checks pass”) that aggregates core test/lint jobs (treating skipped suites as OK) for branch protection; docs note the directtests.ymlrun is the check name to require, not the nested name from on-demand runs.Reviewed by Cursor Bugbot for commit 008d2a3. Bugbot is set up for automated code reviews on this repo. Configure here.