Skip to content

fix(ci): keep full-suite E2E escalation immune to unrelated suite edits - #3434

Closed
IvanHunters wants to merge 2 commits into
mainfrom
fix/select-e2e-full-suite-escalation
Closed

IvanHunters wants to merge 2 commits into
mainfrom
fix/select-e2e-full-suite-escalation

Conversation

@IvanHunters

@IvanHunters IvanHunters commented Jul 22, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Fixes a correctness bug in the diff-driven E2E test selector hack/select-e2e.sh. When a changed packages/* component resolves into the PackageSource graph but has no *-application descendant, the selector is supposed to escalate to the full Chainsaw suite via its safety net rather than silently skip E2E. That safety net keyed off the combined selection being empty, so if the same PR also edited one unrelated suite's tests, that suite was contributed through selected_apps, final_apps became non-empty, and the escalation was silently swallowed: the same system-package change selected the full suite alone but exactly one suite alongside a suite edit.

The fix decides the escalation from the source-derived selection alone (sources_apps), independent of directly-selected per-suite edits, and then unions the per-suite edits afterward. The original empty-final_apps catch-all is kept for the separate case of a per-suite edit on a directory that has no chainsaw-test.yaml yet.

Adds a regression test covering a no-app-descendant package change combined with an unrelated per-suite edit (RED before the fix, GREEN after). The full select-e2e_test.bats suite passes and shellcheck is clean on the changed code.

Downstream repositories

Walked the trigger map against the diff. Changed paths are hack/select-e2e.sh and hack/select-e2e_test.bats; the only hack/-related downstream trigger (cozystack/ccp) is about moving or renaming files under hack/ or changing make-target behaviour, and this PR does neither.

  • No downstream repository is affected by this change

Release note

fix(ci): a PR that edits one E2E suite's tests no longer suppresses full-suite escalation for an unrelated system-package change

Summary by CodeRabbit

  • Bug Fixes

    • Improved end-to-end test selection when system package changes have no directly runnable application tests.
    • Ensured these changes trigger the full test suite, even when unrelated suite-specific edits are included.
  • Tests

    • Added coverage verifying correct escalation to the full end-to-end suite in mixed-change scenarios.

hack/select-e2e.sh escalated to the full Chainsaw suite when a changed
packages/* component resolved into the PackageSource graph but had no
*-application descendant, via the empty-final_apps safety net. That net
keyed off the combined selection, so a PR that also touched one unrelated
suite's tests contributed that suite through selected_apps and made
final_apps non-empty, silently swallowing the escalation: the same
system-package change selected the full suite alone but exactly one suite
alongside a suite edit.

Decide the escalation from the source-derived selection alone
(sources_apps) instead of the combined result, then union the directly
selected per-suite edits afterwards. Keep the original empty-final_apps
catch-all for a per-suite edit on a directory with no chainsaw-test.yaml.

Add a regression test covering a no-app-descendant package change combined
with an unrelated per-suite edit.

Signed-off-by: IvanHunters <[email protected]>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The E2E selector now separates graph-derived suites from directly edited suites, applies graph-specific full-suite escalation when no runnable descendants exist, and validates the behavior with a new Bats regression test.

Changes

E2E selection behavior

Layer / File(s) Summary
Separate graph selection and escalation
hack/select-e2e.sh
Graph-derived sources are mapped to runnable Chainsaw suites, checked independently for empty application descendants, then combined with directly selected suites.
Validate escalation with unrelated edits
hack/select-e2e_test.bats
Adds coverage for package-only and package-plus-unrelated-suite changes, asserting both escalate to the full suite.

Estimated code review effort: 3 (Moderate) | ~15–30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PackageGraph
  participant SelectE2E
  participant ChainsawSuites
  PackageGraph->>SelectE2E: provide selected_sources and all_sources
  SelectE2E->>ChainsawSuites: intersect graph-derived candidates
  ChainsawSuites-->>SelectE2E: return sources_apps
  SelectE2E->>SelectE2E: escalate when no runnable descendants exist
  SelectE2E->>ChainsawSuites: combine graph and direct suite selections
  ChainsawSuites-->>SelectE2E: return final_apps
Loading

Possibly related issues

Possibly related PRs

Suggested labels: area/testing

Suggested reviewers: androndo, lexfrei

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main fix: preserving full-suite E2E escalation despite unrelated per-suite edits.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/select-e2e-full-suite-escalation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size/M This PR changes 30-99 lines, ignoring generated files area/ci Issues or PRs related to CI workflows, GitHub Actions, automation kind/bug Categorizes issue or PR as related to a bug labels Jul 22, 2026
Use ASCII punctuation in the comments added by the previous commit.

Signed-off-by: IvanHunters <[email protected]>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
hack/select-e2e_test.bats (1)

182-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid an EXIT trap in this new E2E test.

Line 183 adds an EXIT trap; use explicit cleanup after the assertions instead. As per coding guidelines, “avoid EXIT and RETURN traps.”

Proposed fix
     tmp=$(mktemp -d)
-    trap 'rm -rf "$tmp"' EXIT
     cp -r packages/core/platform/sources "$tmp/sources"
@@
     both=$(hack/select-e2e.sh "$tmp/both" "$tmp/sources")
     [ "$(echo "$both" | wc -w)" -gt 5 ]
+    rm -rf "$tmp"
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@hack/select-e2e_test.bats` around lines 182 - 183, Remove the EXIT trap
associated with the temporary directory in the new E2E test. Add explicit
cleanup with rm -rf "$tmp" after all assertions complete, while preserving the
existing temporary-directory setup and test behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@hack/select-e2e_test.bats`:
- Around line 182-183: Remove the EXIT trap associated with the temporary
directory in the new E2E test. Add explicit cleanup with rm -rf "$tmp" after all
assertions complete, while preserving the existing temporary-directory setup and
test behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ad345169-5c5b-4f32-b366-0c9e5f41f19e

📥 Commits

Reviewing files that changed from the base of the PR and between f077420 and d6458d5.

📒 Files selected for processing (2)
  • hack/select-e2e.sh
  • hack/select-e2e_test.bats

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 10, 2026
## What this PR does

`hack/select-e2e.sh` escalates to the full Chainsaw suite when a changed
package reaches no suite through the dependency graph, because otherwise
that change gets no E2E coverage at all. The escalation was read off the
merged selection, so it depended on the whole diff rather than on the
path that needed it. Any other changed path contributing a suite name
made the selection non-empty and the escalation vanished with nothing in
the output to say it had. Editing a platform component together with one
unrelated suite's Chainsaw test ran that single suite instead of
everything, which is an ordinary PR shape: change a component, adjust
the e2e near it.

Coverage is now decided per changed path. Each path resolves through its
owning sources to the suites that exist, and a path reaching none of
them forces the full run whatever the rest of the diff selected. The
unit is the path rather than the source, because
`system/postgres-operator` belongs to two PackageSources where one
reaches no suite, and deciding per source would run everything on every
change to it.

The other half is the mapping that left packages unreachable from their
own suites. `src_to_suites()` covered `*-application` names plus
`external-dns`, and the final filter dropped the rest, so
`cozystack.kuberture` and `cozystack.securitygroup-controller` were
walked to and then discarded. A CNI change ran neither of the two
controller suites most likely to regress from it, and a change to either
package escalated to the full run rather than selecting the suite that
covers it. Every reached source now maps through the table, which is the
inverse of `select-install.sh`'s `suite_to_source()`; a test walks every
suite through both, so the next source name fitting neither convention
fails there instead of quietly escalating forever.

## What was measured

Behaviour was diffed against the current selector over one `values.yaml`
per package under `packages/{apps,system,extra}`: 159 packages, 8
selections change, all of them the mapping repair. Five networking
packages and `prometheus-operator-crds` widen — 19 to 21 suites and 10
to 11, both gaining `kuberture` and `securitygroup`. `kuberture` and
`securitygroup-controller` narrow from the full suite to their own
single suite. Nothing narrows that should not.

That sweep is one path per diff, so it says nothing about the per-path
escalation, which only shows on a diff with more than one path. Pairing
each package with one unrelated Chainsaw edit gives that number. 82 of
the 159 packages escalate on their own and stop escalating once the pair
is added, which is the size of the hole: for those, adding an unrelated
suite edit to the same pull request replaced the full run with one
suite. 88 packages select something different after this change, the
extra six being the mapping repair widening a selection rather than
restoring an escalation.

Worth stating alongside it, because it decides whether the branch is a
corner or the common case: 83 of the 159 escalate on a single-path diff
today, and 81 do after this change — the two that leave are `kuberture`
and `securitygroup-controller`, which now select the suite that covers
them. So the escalation is the branch most packages take, and the
swallowed one was the branch most packages took. Counting that needs an
instrument that separates a graph walk which reached everything from an
escalation which printed everything, since the two agree once the
mapping is fixed: the count above comes from a scratch suite tree
carrying one extra suite that no source names, which only an escalation
can emit.

The suite selection is what decides which tests run, so a rule that
stops firing does not go red, it disappears. The tests pin the property
rather than a worked example: that a path reaching no suite escalates
whatever else the diff holds, that a covered path never drags the full
suite in with it, and that every suite round-trips between the two
mapping tables. Each was checked by mutation — removing the rule it
guards turns it red.

## Left for a follow-up

`resolve_suites` writes into the caller's scope because POSIX sh has no
`local`, and what keeps that safe is the single call site sitting inside
`$( )`, pinned by a test that counts occurrences of the name. Declaring
the function with `( … )` instead of `{ … }` would contain those
variables by construction and let both the test and the two paragraphs
explaining the constraint go away. A count-the-name grep is the weakest
kind of pin and it exists only because the function leaks.

The output contract at the top of the script lists the causes of a
full-suite selection but not the backstop, which after this change is a
mechanism of its own: an edit under `hack/e2e-chainsaw/<dir>/` where
that directory holds no `chainsaw-test.yaml`. The behaviour is pinned by
a test and explained where it lives; only the header block is short. In
the same area, the escalation line names the owning sources rather than
the changed path, and reports the first uncovered group rather than all
of them.

## Overlap

#3434 fixes the same escalation from a different angle: it decides the
escalation from the whole source-derived selection rather than per path.
That closes the "package with no coverage plus an unrelated suite edit"
shape, and leaves "package with no coverage plus another package that
does select suites", where the source-derived selection is non-empty and
the escalation is swallowed again. There is a test for that second shape
here.

### Screenshots

Not applicable, no UI changes.

### Downstream repositories

Walked the trigger map in `docs/agents/contributing.md` against the
diff. The changed files are `hack/select-e2e.sh`,
`hack/select-e2e_test.bats` and `docs/agents/e2e-testing.md`. The only
`hack/`-related trigger is cozystack/ccp, which gates on
`hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` and on
what a make target does; nothing here is moved, renamed, or changed in
make-target behaviour. The ansible-cozystack and talm triggers are about
`hack/e2e-prepare-cluster.bats`, which is untouched.

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

```release-note
fix(ci): a changed package that no E2E suite covers now runs the full suite even when the same pull request also edits an unrelated suite, and changes to the kuberture and securitygroup packages select their own suite instead of escalating
```

Closes #3330

Closes #3665
@myasnikovdaniil

Copy link
Copy Markdown
Contributor

IvanHunters rebase please

@myasnikovdaniil

Copy link
Copy Markdown
Contributor

closing in favor of merged #3441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci Issues or PRs related to CI workflows, GitHub Actions, automation kind/bug Categorizes issue or PR as related to a bug size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants