Skip to content

Commit 127e200

Browse files
fix(ci): decide the per-app bats rule per path, not after the merge
Two defects in the hack/e2e-apps/ arm added by the previous commit. The arm set trigger_any=1 for whatever basename it derived and left the verdict to the final intersection. That makes the escalation depend on the REST of the diff: hack/e2e-apps/monitoring-oidc-system.bats alone emptied the selection and the backstop escalated, but the same file beside packages/apps/redis/templates/redis.yaml selected `redis` with nothing on stderr, where the pre-change selector ran all 21 suites. That is the merge-before-escalate shape #3330 removed from the graph walk, reintroduced one rule over. The basename is now membership-tested against the suite list on the spot and an unmatched one sets trigger_full=1 and names the file. POSIX case matches `/` with `*`, so the arm also sees nested paths, and the unanchored capture turned hack/e2e-apps/fixtures/postgres.bats into the "suite" fixtures/postgres -- right outcome by accident, reason line printing a path where a suite name belongs. The capture is anchored to one segment, and what the arm does with anything that is not a top-level <name>.bats is now stated rather than left to fall out of a regex: it is shared material for those suites the way hack/e2e-chainsaw/_lib/ is for the Chainsaw ones, so it escalates for the same reason. Both claims that said otherwise are corrected -- the arm's own comment and docs/agents/e2e-testing.md both asserted the deferred escalation was equivalent to the old fall-through, which held only for a single-file diff. The backstop comment drops the e2e-apps clause it gained last commit, since that route no longer reaches it. The header's "four of the seven" becomes count-free: this commit adds two escalating branches, so a hardcoded total was already stale. Tests: the mixed-diff case is the regression pin (the isolated case passes with the escalation deferred and cannot see the bug), plus a nested-path case and a non-.bats case. Both fixes are mutation-verified -- restoring either defect turns the file red. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent cefc3a6 commit 127e200

3 files changed

Lines changed: 100 additions & 28 deletions

File tree

‎docs/agents/e2e-testing.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,11 @@ A parent HelmRelease that hit its wait timeout, uninstalled, and reinstalled is
6868

6969
`hack/select-e2e.sh` walks the `packages/core/platform/sources/*.yaml` dependency graph and runs only the Chainsaw suites affected by a diff. A suite is a directory under `hack/e2e-chainsaw/` containing a `chainsaw-test.yaml`.
7070

71-
- Conservative escalation: edits to `packages/library/`, `packages/core/`, the Go trees (`api/`, `cmd/`, `internal/`, `pkg/`), the codegen under `tools/`, `go.mod`/`go.sum`, top-level `hack/*.sh` helpers, the `hack/e2e-*.bats` the sandbox runs and the `hack/lib/` they source, `hack/*.mk`, `hack/buildkitd.toml`, `hack/e2e-*.yaml`, the shared `hack/e2e-chainsaw/_lib/`, the chainsaw config `hack/e2e-chainsaw/.chainsaw.yaml`, the `Makefile`, or any workflow that runs the suite escalate to the **full suite**. Only `pull-requests.yaml` is executed from the pull request's own head, so escalating on the other three buys generic regression coverage rather than coverage of the edit; the trade and its measured cost are written next to `full_suite_pattern` rather than repeated here. A per-suite edit (any file under `hack/e2e-chainsaw/<app>/`) selects **only** that suite, with one exception: a `*.disabled` file selects nothing, because a parked suite is executed by nothing and editing it cannot regress a test. `hack/e2e-apps/<name>.bats` — the pre-Chainsaw per-app BATS suites — maps the same way, off the basename instead of the directory. What remains in that directory is wired to nothing after the migration, and it is deliberately mapped rather than marked inert: inert would bake the orphan status into the rule and go quietly wrong the day a lane runs those files again, while a name that no suite carries drops out of the intersection and escalates through the backstop exactly as it did when nothing classified it.
71+
- Conservative escalation: edits to `packages/library/`, `packages/core/`, the Go trees (`api/`, `cmd/`, `internal/`, `pkg/`), the codegen under `tools/`, `go.mod`/`go.sum`, top-level `hack/*.sh` helpers, the `hack/e2e-*.bats` the sandbox runs and the `hack/lib/` they source, `hack/*.mk`, `hack/buildkitd.toml`, `hack/e2e-*.yaml`, the shared `hack/e2e-chainsaw/_lib/`, the chainsaw config `hack/e2e-chainsaw/.chainsaw.yaml`, the `Makefile`, or any workflow that runs the suite escalate to the **full suite**. Only `pull-requests.yaml` is executed from the pull request's own head, so escalating on the other three buys generic regression coverage rather than coverage of the edit; the trade and its measured cost are written next to `full_suite_pattern` rather than repeated here. A per-suite edit (any file under `hack/e2e-chainsaw/<app>/`) selects **only** that suite, with one exception: a `*.disabled` file selects nothing, because a parked suite is executed by nothing and editing it cannot regress a test. `hack/e2e-apps/<name>.bats` — the pre-Chainsaw per-app BATS suites — maps the same way, off the basename instead of the directory. What remains in that directory is wired to nothing after the migration, and it is deliberately mapped rather than marked inert: inert would bake the orphan status into the rule and go quietly wrong the day a lane runs those files again. The basename is membership-tested against the suite list on the spot, and a name no suite carries escalates there and then, naming the file. Deferring that to the final intersection instead would leave the verdict to the rest of the diff — alone the selection empties and the backstop escalates, but beside any path that contributed a suite the escalation disappears and the run narrows to that suite in silence, which is the merge-before-escalate shape #3330 removed from the graph walk. Anything else under `hack/e2e-apps/` — a nested path, a non-`.bats` file — is shared material for those suites the way `hack/e2e-chainsaw/_lib/` is for the Chainsaw ones, and escalates for the same reason.
7272
- **Every path is classified, and an unclassified one escalates.** The selector has three selection outcomes — full suite, a scoped set, or nothing — and each is reached by an explicit rule; the fourth outcome in the bullet below is not a selection but a refusal to produce one. Several rules produce the empty one, which is worth knowing before reading an empty selection as "the inert list matched": documentation, dashboards and `*.md` are dropped first wherever they live, a `*.disabled` Chainsaw suite and a `examples/backups/<app>/` edit whose app has no suite are dropped where chainsaw and backup-example paths are matched, and `inert_config_pattern` covers repo meta, agent config, `hack/testdata/`, the helm-unittest fixture charts under `packages/tests/`, every `.gitattributes` (all of them are `linguist-generated` markers, which reach no build, chart or test) and the top-level `hack/*.bats` that are not `hack/e2e-*.bats`. That last one is the unit lane, and the root `Makefile` is the authority on the split: `BATS_UNIT_FILES := $(filter-out hack/e2e-%.bats,$(wildcard hack/*.bats))` keeps 60 files for `make unit-tests` while `packages/core/testing`'s recipes run only the three it filters out, so the sandbox never executes one of the 60 and no Chainsaw suite can regress from an edit to one. Being inert here does not leave them untested: the `unit-tests` job is gated on the `plan` job's `code` output, which `pull-requests.yaml` computes as "any changed path outside `docs/`" and never from `select-e2e.sh`, so a bats-only pull request still runs the whole unit lane — plus install and the OpenAPI tests, since the `e2e` job also reads `code` rather than the selection. `full_suite_pattern` covers shared build inputs and the e2e harness, and the graph lookup handles `packages/(apps|system|extra)/`. A path matching none of them escalates to the full suite and logs why, because both lanes read an empty selection as "skip Chainsaw" and then post `E2E Tests` green — so a silent default is a green gate with nothing tested (#3392). When that escalation fires for something genuinely inert, add it to `inert_config_pattern`; do not relax the fall-through. The escalation list and the inert list are checked in that order, so a specific escalation beats a broad inert directory.
7373
- **Coverage is decided per changed path.** A path that resolves through the graph but reaches no runnable suite is covered by nothing, so it forces the full run on its own account, whatever the rest of the diff selected. Reading that off the merged selection instead let any other changed path contributing a suite name swallow the escalation, with nothing in the output to record that it happened: change a platform component, adjust the Chainsaw test beside it, and the component's escalation disappeared behind that one suite (#3330). The unit is the path rather than the source, because one path can belong to several PackageSources — `system/postgres-operator` belongs to two, one of which reaches no suite — and deciding per source would escalate on that half and run everything for every change to it.
7474
- **There is a fourth outcome, and it is a failed step rather than a selection.** It fires when the suite list itself is unavailable: `find` cannot list `hack/e2e-chainsaw/`, or that directory holds no `chainsaw-test.yaml`. There is no fallback left to take, because escalating would print an empty list and both lanes read an empty selection as "skip Chainsaw" before posting `E2E Tests` green — the outcome meaning "run everything" would deliver a run of nothing. So the selector names what broke on stderr and exits non-zero; both lanes run the step under `bash -e`, so the job fails and the required check goes red. A red `Select E2E tests` step is a real signal, not a flake: read its stderr line before rerunning. A `yq` failure is deliberately not in this class — what is missing there is the dependency graph, not the suite list, so the selector still has a safe answer and escalates to the full suite. That is conservative but quiet: the only sign is one stderr line inside a step that passes, so a selector running blind on every PR looks exactly like a project that escalates a lot.
75-
- **Every escalation names its cause on stderr.** "Run everything" is the same list of suite names whichever rule produced it, so a reason line is the only thing that distinguishes the seven branches that can reach it, and without one the answer to "why did this pull request run the whole suite" has to be re-derived by hand from the diff. Four of the seven used to be silent — the shared `hack/e2e-chainsaw/_lib/` and `.chainsaw.yaml` case, the `full_suite_pattern` match, a `packages/` path no PackageSource claims, and the empty-selection backstop — and `full_suite_pattern` is the commonest cause of a full run by a wide margin, so the usual answer was the one the log never carried. Each line names the path or the sources that caused it, and a unit test asserts each message, because a reason that regresses to silence changes no selection and is invisible to every other test. The lines go to **stderr** and must stay there: stdout is the suite list and both lanes parse it, so a reason line on stdout would be read as a suite name.
75+
- **Every escalation names its cause on stderr.** "Run everything" is the same list of suite names whichever rule produced it, so a reason line is the only thing that distinguishes the branches that can reach it, and without one the answer to "why did this pull request run the whole suite" has to be re-derived by hand from the diff. Four of them used to be silent — the shared `hack/e2e-chainsaw/_lib/` and `.chainsaw.yaml` case, the `full_suite_pattern` match, a `packages/` path no PackageSource claims, and the empty-selection backstop — and `full_suite_pattern` is the commonest cause of a full run by a wide margin, so the usual answer was the one the log never carried. Each line names the path or the sources that caused it, and a unit test asserts each message, because a reason that regresses to silence changes no selection and is invisible to every other test. The lines go to **stderr** and must stay there: stdout is the suite list and both lanes parse it, so a reason line on stdout would be read as a suite name.
7676
- **The one path the fall-through cannot catch is a new e2e workflow.** `inert_config_pattern` makes `.github/` inert as a directory, and the workflows that run the suite are exempted from it by name in `full_suite_pattern`. A workflow added under `.github/workflows/` therefore reads as inert until someone adds it to that enumeration, and the fall-through never sees it. That is why the list carries a `rg -l test-chainsaw .github/workflows/` reminder: the enumeration is the guard, not a convenience.
7777
- The `full-e2e` PR label forces the whole suite.
7878
- The selection is passed to `make test-chainsaw CHAINSAW_SUITES="<names>"`, which runs `chainsaw test <names>` from `hack/e2e-chainsaw/` (empty = the whole suite).

‎hack/select-e2e.sh‎

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@
4545
# rather than a courtesy: the answer "run everything" is the same 21 suite names
4646
# whichever rule produced it, so without a reason line the only way to learn why
4747
# a pull request ran the whole suite is to re-derive the selection by hand. Four
48-
# of the seven were silent, full_suite_pattern — the commonest cause by a wide
49-
# margin — among them, which made the usual answer the one the log never had. A
50-
# test asserts each line, because a reason that regresses to silence changes no
51-
# selection and so is invisible to every other test here.
48+
# of them were silent before this rule, full_suite_pattern — the commonest cause
49+
# by a wide margin — among them, which made the usual answer the one the log
50+
# never had. A test asserts each line, because a reason that regresses to silence
51+
# changes no selection and so is invisible to every other test here. Deliberately
52+
# stated without a count: the number of escalating branches moves whenever a rule
53+
# is added, and a comment carrying a stale one reads as authoritative.
5254
#
5355
# The lines go to stderr, never stdout. stdout is the suite list and both lanes
5456
# parse it, so a reason line there would be read as a suite name.
@@ -312,7 +314,7 @@ while IFS= read -r file || [ -n "$file" ]; do
312314
selected_apps="$selected_apps $app"
313315
trigger_any=1
314316
continue ;;
315-
hack/e2e-apps/*.bats)
317+
hack/e2e-apps/*)
316318
# The pre-Chainsaw per-app BATS suites. One file per app, named after it,
317319
# so the suite name comes off the basename exactly as the rule above takes
318320
# it off the directory.
@@ -323,14 +325,34 @@ while IFS= read -r file || [ -n "$file" ]; do
323325
# mapping is correct either way — a file named after an app selects that
324326
# app's suite whether or not anything currently executes it.
325327
#
326-
# A basename that names no suite (the monitoring-oidc-* pair left here has
327-
# no Chainsaw suite of its own) is dropped by intersect_suites downstream,
328-
# and if it was the only selection the backstop escalates and says so. That
329-
# is the same outcome the unclassified fall-through gave these paths before,
330-
# so nothing is lost by mapping optimistically.
331-
app=$(echo "$file" | sed -nE 's,^hack/e2e-apps/(.+)\.bats$,\1,p')
332-
selected_apps="$selected_apps $app"
333-
trigger_any=1
328+
# The membership test is what makes this per-path, and it is not optional.
329+
# Adding an unmatched name to selected_apps and letting the final
330+
# intersection drop it leaves the verdict to the REST of the diff: alone it
331+
# empties the selection and the backstop escalates, but beside any path that
332+
# contributed a suite the escalation vanishes and the run narrows to that
333+
# suite with nothing on stderr. That is the merge-before-escalate shape
334+
# #3330 removed from the graph walk, and it applies here for the same
335+
# reason — coverage is a property of the changed path.
336+
#
337+
# `[^/]+` rather than `.+`: POSIX case matches `/` with `*`, so this arm
338+
# also sees nested paths, and an unanchored capture would turn
339+
# hack/e2e-apps/fixtures/postgres.bats into the "suite" fixtures/postgres.
340+
# Nothing nested exists here today; whatever appears would be shared
341+
# material for these suites the way hack/e2e-chainsaw/_lib/ is for the
342+
# Chainsaw ones, so it escalates for the same reason rather than being
343+
# guessed at. A non-.bats file directly in the directory lands here too and
344+
# is treated the same way.
345+
app=$(echo "$file" | sed -nE 's,^hack/e2e-apps/([^/]+)\.bats$,\1,p')
346+
if [ -z "$app" ]; then
347+
echo "select-e2e: '$file' is not a per-app suite file (hack/e2e-apps/<name>.bats), so it is shared material for all of them — escalating to the full suite" >&2
348+
trigger_full=1
349+
elif echo "$all_apps" | grep -Fxq "$app"; then
350+
selected_apps="$selected_apps $app"
351+
trigger_any=1
352+
else
353+
echo "select-e2e: '$file' names no runnable suite ('$app') — escalating to the full suite" >&2
354+
trigger_full=1
355+
fi
334356
continue ;;
335357
examples/backups/*/*)
336358
# The etcd, postgres, mariadb and clickhouse backup round-trip tests
@@ -472,12 +494,12 @@ done
472494
final_apps=$(intersect_suites "$group_suites $selected_apps")
473495

474496
# Backstop. Every graph path above either escalates or contributes a suite that
475-
# exists, so what still reaches this is a directly-selected name that is not a
476-
# suite: a per-suite edit naming a directory that holds no chainsaw-test.yaml —
477-
# shared material beside _lib/, or a suite nested deeper than the depth-2 scan
478-
# looks — or a hack/e2e-apps/<name>.bats whose <name> no suite carries.
479-
# Selecting nothing for those would skip E2E outright, so failing towards the
480-
# full suite is the only safe way to be wrong here.
497+
# exists, and both rules that select a suite by name — the per-app BATS one and
498+
# the examples/backups/<app>/ one — membership-test it first, so what still
499+
# reaches this is a per-suite Chainsaw edit naming a directory that holds no
500+
# chainsaw-test.yaml: shared material beside _lib/, or a suite nested deeper than
501+
# the depth-2 scan looks. Selecting nothing for those would skip E2E outright, so
502+
# failing towards the full suite is the only safe way to be wrong here.
481503
#
482504
# group_suites is empty whenever this fires — every group either escalated above
483505
# or contributed a suite that exists — so the names worth naming are the

0 commit comments

Comments
 (0)