Conversation
Adds the `merge_group` trigger to the PR pipeline and to the E2E orchestrator, plus an `E2E tests passed` aggregate job to require on `main` alongside `All required checks pass`. A queue entry skips the suites when the PR head commit already carries a passing `E2E tests passed` check — from the `e2e-tests` label or a dependabot push — or when the PR touches nothing E2E covers. Every unexpected case runs the suites instead of waving the merge through. In merge queue runs `dorny/paths-filter` has to keep its `base` input empty so its own base_sha..head_sha handling applies; `base: main` collapses the diff there and every suite would skip. Enabling the queue and marking the two checks required are branch protection changes still to be made. Refs #RI-8373 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: aac9e25dc6
ℹ️ 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".
|
|
||
| # Application code, the E2E suite itself, the test environment, the | ||
| # build inputs, and the workflows that run all of it. | ||
| if grep -qE '^(redisinsight/|tests/e2e|package(-lock)?\.json|\.nvmrc|Dockerfile|\.dockerignore|configs/|scripts/|\.github/(workflows|actions|build)/)' <<< "$files"; then |
There was a problem hiding this comment.
For a merge-queue PR that changes only electron-builder.json or resources/**, this regex reports no_relevant_changes=true, so e2e-passed turns green without building or testing Electron. I checked the invoked Linux pipeline: it runs npm run package:stage, whose Electron Builder configuration extends electron-builder.json and packages resources/**; therefore changes such as an invalid packaging option or missing runtime resource can reach main without the newly required E2E gate exercising them. Include these direct packaging inputs (and other equivalent build inputs) in this path predicate.
Useful? React with 👍 / 👎.
The prior-pass lookup reads the single PR named in the merge queue ref. GitHub batches up to 5 PRs per group by default, and a batched group's ref names only its newest PR, so a batch could skip E2E on the strength of a different PR's passing run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
KrumTy
changed the title
Code Coverage - Backend unit tests
Test suite run success3779 tests passing in 325 suites. Report generated by 🧪jest coverage report action from ec82dbe |
Code Coverage - Integration Tests
|
![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 ec82dbe. Configure here.
| if grep -qE '^(redisinsight/|tests/e2e|package(-lock)?\.json|\.nvmrc|Dockerfile|\.dockerignore|configs/|scripts/|\.github/(workflows|actions|build)/)' <<< "$files"; then | ||
| echo "PR #$pr_number touches paths E2E covers." | ||
| else | ||
| echo "no_relevant_changes=true" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
E2E skip allowlist omits build inputs
Medium Severity
The merge-queue skip path treats a PR as E2E-irrelevant unless its files match a short allowlist, but that list does not include inputs the suites actually build and run with, such as patches/, .npmrc, docker-entry.sh, and electron-builder.json. A change that only touches those files can still publish a successful E2E tests passed check and merge without running E2E.
Reviewed by Cursor Bugbot for commit ec82dbe. Configure here.
Code Coverage - Frontend unit tests
Test suite run success8049 tests passing in 872 suites. Report generated by 🧪jest coverage report action from ec82dbe |
1 participant
What
Wires E2E into the merge path so nothing lands on
mainwithout it. Today E2Eruns only on demand (
e2e-testslabel, dependabot pushes, nightly), so most PRsmerge untested and nightly breaks on
main.Two workflow changes, both inert until branch protection is updated:
tests.ymlgains themerge_grouptrigger. The one catch isdorny/paths-filter: itsbaseinput must be empty onmerge_groupso itsown
base_sha..head_shahandling applies. Left atbase: mainthe diffcollapses to nothing in a queue ref, every suite skips, and
All required checks passgoes green having run nothing. Coverage uploads anddeployment cleanup are skipped in queue runs — there is no PR to report to.
tests-e2e-playwright-v2.ymlgains themerge_grouptrigger and anE2E tests passedaggregate job, the E2E counterpart ofAll required checks pass. A queue entry skips the suites when the PR headcommit already carries a passing
E2E tests passedcheck (label run ordependabot push) or when the PR touches nothing E2E covers. Every other
outcome — unreadable ref, missing PR, flaky API call, crashed decision job —
runs the suites or fails the gate rather than waving the merge through. The
skip-e2elabel deliberately has no effect here.Full E2E fan-out is ~30 min wall clock (last 15 nightlies), and pre-running via
the
e2e-testslabel turns queue entry into a ~1 min skip.Follow-up, not in this PR
Requires repo admin on the
mainprotection rule:All required checks passandE2E tests passed.maximum group size 1. Group size is load-bearing, not a tuning knob: a
batched group names only its newest PR in the ref, so the prior-pass lookup
would read the wrong PR and could skip E2E for a batch containing an untested
one. Raising it means reworking
check-triggerto cover every PR in a group.Worth watching on the first queued PRs: the
productionenvironment used by theintegration matrix has a branch-policy rule with zero patterns configured, which
currently behaves permissively. If queue refs get blocked on it, add
gh-readonly-queue/*— but check what normal branches need at the same time, asadding the first pattern may turn a permissive empty list into a restrictive one.
Note this doubles unit/integration CI per merged PR, which is inherent to running
a merge queue: the suites run once on the PR and again on the queue's branch.
Testing
The decision logic and the gate were extracted from the YAML and run under
GitHub's shell (
bash -eo pipefail) across 32 scenarios, including: a crasheddecision job must fail the gate; a non-queue run must not skip its way to green;
a
skippedcheck must not count as a prior pass; unparseable ref and missing PRmust fall through to running E2E. The two API-backed steps were run against this
repo, confirming check-run lookup on a real PR head commit and that a
.github/dependabot.yml-only PR is correctly classed as needing no E2E.actionlintis clean on both files.This PR carries the
e2e-testslabel, so its own run publishes the newE2E tests passedcheck — that is the check the queue will later look for.Refs #RI-8373
🤖 Generated with Claude Code