Repository navigation
fix(ci): keep full-suite E2E escalation immune to unrelated suite edits - #3434
IvanHunters wants to merge 2 commits into
Conversation
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]>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe 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. ChangesE2E selection behavior
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
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Use ASCII punctuation in the comments added by the previous commit. Signed-off-by: IvanHunters <[email protected]>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/select-e2e_test.bats (1)
182-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid an
EXITtrap in this new E2E test.Line 183 adds an
EXITtrap; use explicit cleanup after the assertions instead. As per coding guidelines, “avoidEXITandRETURNtraps.”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
📒 Files selected for processing (2)
hack/select-e2e.shhack/select-e2e_test.bats
## 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
|
IvanHunters rebase please |
|
closing in favor of merged #3441 |
What this PR does
Fixes a correctness bug in the diff-driven E2E test selector
hack/select-e2e.sh. When a changedpackages/*component resolves into the PackageSource graph but has no*-applicationdescendant, 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 throughselected_apps,final_appsbecame 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_appscatch-all is kept for the separate case of a per-suite edit on a directory that has nochainsaw-test.yamlyet.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.batssuite passes andshellcheckis clean on the changed code.Downstream repositories
Walked the trigger map against the diff. Changed paths are
hack/select-e2e.shandhack/select-e2e_test.bats; the onlyhack/-related downstream trigger (cozystack/ccp) is about moving or renaming files underhack/or changing make-target behaviour, and this PR does neither.Release note
Summary by CodeRabbit
Bug Fixes
Tests