Skip to content

test(hack): restore set -u and refresh the cozytest-era headers - #3498

Closed
myasnikovdaniil wants to merge 1 commit into
test/bats-vanillafrom
test/bats-strict-mode
Closed

myasnikovdaniil wants to merge 1 commit into
test/bats-vanillafrom
test/bats-strict-mode

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Part 2 of a stack implementing #3453. Stacked on top of the runner switch — review that one first; this diff only makes sense against it.

What

Restores the set -u that hack/cozytest.sh applied to every test body, and rewrites the 27 file headers that documented the cozytest contract in prose.

Why set -u needs a per-file load

bats enforces set -e but not set -u, so without this a test that reads an unset variable silently sees an empty string and passes — the one strictness property the move to bats would otherwise drop. There is no runner-level way to inject it. Two mechanisms were measured and rejected:

Mechanism Result
setup() in --setup-suite-file Does not reach test files at all — the unset-variable canary still passes
SHELLOPTS=nounset in the environment Does reach them, but is exported, so it also applies to the hack/*.sh scripts under test — changing the behaviour of the code being tested rather than the tests, and dropping 2 tests outright
set -u inside setup() (chosen) Shell option, not exported, so it stays scoped to the test body

hack/test_helper.bash defines the shared setup(); all 32 unit files load it.

Verification

The restoration is mutation-checked, not assumed. A canary test reading an unset variable aborts with unbound variable with the load in place, and passes vacuously without it:

not ok 329 CANARY unset variable must abort the test
# hack/zz-canary-tmp.bats: line 8: DELIBERATELY_UNSET_CANARY: unbound variable

The suite is 328/328 green either way, so nothing in the tree was relying on the laxer behaviour. make bats-unit-tests and the pre-commit hook both pass.

The headers

27 of the 32 files stated things that are now false: "there is no bats run or $status", "setup()/teardown() are not honored", "Run with: hack/cozytest.sh ...", title-sanitization rules inherited from the awk parser, and instructions to keep } off column 0 so the parser would not truncate a test. A false comment about the harness is worse than no comment — the next author reads "setup() is not honored" and hand-rolls per-test cleanup the runner would have done for them.

Where a constraint merely stopped binding rather than reversing, the comment says so instead of vanishing: the column-0 heredoc indentation and the manual status capture in migration-seaweedfs-db-adopt.bats are both kept and marked belt-and-braces. Claims that survived the move are left alone — ! cmd really is still vacuous under set -e, because bash suppresses errexit for a negated command exactly as dash did.

hack/seaweedfs-naming-audit.bats gains a note that its subject is POSIX sh executed by /bin/sh but sourced into bats' bash, so shellcheck's shell=sh directive is now the only thing guarding that gap.

Refs: #3453

@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review size/L This PR changes 100-499 lines, ignoring generated files labels Jul 31, 2026
@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.

The layer below moved the unit suite to bats(1), which enforces `set -e`
but not `set -u`. cozytest.sh ran every test body under `set -eu -x`, so
without this a test that reads an unset variable silently sees an empty
string and passes.

There is no runner-level way to inject it, which is why this needs a
per-file load rather than a Makefile flag. Two mechanisms were measured and
rejected: a setup() defined in --setup-suite-file does not reach test files
at all, and SHELLOPTS=nounset does reach them but is exported, so it also
applies to the hack/*.sh scripts the tests exercise as subprocesses --
changing the behaviour of the code under test rather than the tests, and
dropping two tests outright. `set -u` inside setup() is a shell option
rather than an exported one, so it stays scoped to the test body.

hack/test_helper.bash therefore defines the shared setup(), and all 32 unit
files load it. The restoration is mutation-checked, not assumed: a canary
test reading an unset variable aborts with "unbound variable" with the load
in place and passes vacuously without it. Suite is 328/328 green either
way, so nothing in the tree was relying on the laxer behaviour.

The headers are the other half. 27 of the 32 files documented the cozytest
contract in prose -- "there is no bats `run` or `$status`",
"setup()/teardown() are not honored", "Run with: hack/cozytest.sh ...",
title-sanitization rules from the awk parser, and instructions to keep `}`
off column 0 so the parser would not truncate a test. Every one of those
statements is now false or vacuous, and a false comment about the harness
is worse than none: the next author reads "setup() is not honored" and
hand-rolls per-test cleanup that the runner would have done.

Where a constraint merely stopped binding rather than reversing, the
comment says so instead of deleting the line -- the column-0 heredoc
indentation and the manual status capture in migration-seaweedfs-db-adopt
are both kept, now marked belt-and-braces. Claims that survived the move
are left alone: `! cmd` really is still vacuous under `set -e`, since bash
suppresses errexit for a negated command exactly as dash did.

hack/seaweedfs-naming-audit.bats gains a note that its subject is POSIX sh
executed by /bin/sh but sourced into bats' bash, so shellcheck's shell=sh
directive is now the only thing guarding that gap.

Refs: #3453
Signed-off-by: Myasnikov Daniil <[email protected]>
@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 20fb175d-78aa-4ff2-8d6f-4e167136e333

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@myasnikovdaniil

Copy link
Copy Markdown
Contributor Author

Replaced by #3849, which carries the shim as its first commit.

Worth recording why it needed reworking rather than rebasing: the shim was complete when written, 32 files of 32, and is now 32 of 63. The gap is drift rather than a defect, since 27 of the 31 uncovered files postdate this pull request. The cause is that coverage needed a hand-added load test_helper per file, so every new file was uncovered by construction.

#3849 makes it structural instead: an audit enumerates the unit files from the filesystem and fails on any that does not load the helper, and cross-checks its own idea of the file set against make print-bats-unit-files. There is no list to maintain, so it cannot drift the same way.

Closing in favour of it.

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 20, 2026
## Why

I replayed the 150 most recently merged pull requests through
`hack/select-e2e.sh` with every escalation branch instrumented. 118 of
them (78.7%) ran the full 21-suite Chainsaw run, and the causes, counted
once per escalating PR with the count where that cause was the only one
in brackets, were `FULL_PATTERN` 88 [68], `NO_SUITE_FOR_GROUP` 24 [24],
`CHAINSAW_SHARED` 18 [1] and `UNCLASSIFIED` 7 [4]. `YQ_BROKEN`,
`NO_GRAPH_OWNER` and `BACKSTOP_EMPTY` never fired.

Two problems come out of that. Most of the escalation is real but
unnecessary. Whole classes of path that provably cannot affect a
Chainsaw suite were escalating because no rule claimed them. And the
commonest cause of a full run was absent from the log, so "why did this
pull request run everything" could only be answered by re-deriving the
selection by hand.

## What changes

**Every escalation now names its cause on stderr.** Seven branches could
reach the full suite and four did it in silence, `full_suite_pattern`
among them, the commonest cause by a wide margin, so the usual answer
was the one the log never carried. The lines go to stderr and must stay
there: stdout is the suite list and both e2e lanes parse it, so a reason
line on stdout would be read as a suite name. A unit test asserts every
message, verified by muting each of the eleven in turn and confirming
the file goes red for all of them. A reason that regresses to silence
changes no selection and is otherwise invisible.

**`cozystack.etcd-application` gains its missing `dependsOn:
cozystack.etcd-operator`.** `packages/extra/etcd` renders `kind:
EtcdCluster` from `etcd-operator.cozystack.io/v1alpha2`, and this was
the only operator-backed application source in the tree with no edge to
its operator. It is a latent install bug as much as a selection gap:
without the edge `etcd-rd` registers the ApplicationDefinition as soon
as the engine is up, so a tenant can create an `Etcd` before the
operator exists and its HelmRelease fails on `no matches for kind
"EtcdCluster"`, and `hack/select-install.sh`'s forward closure for the
`etcd` suite omitted the operator for the same reason. On the selection
side `cozystack.etcd-operator` reached no runnable suite, so every
change to the operator ran all 21.

**`vm-disk-application` maps to the `vminstance` suite.** It resolved to
a `vm-disk` suite that does not exist, `intersect_suites()` dropped the
name, and the source reached nothing runnable. Neither did
`cozystack.kubevirt-cdi` above it, whose only other dependents are
`vm-default-images` and `vm-disk-application` itself, so every CDI
change ran all 21. The coverage was there and the table did not know it:
the `vminstance` suite creates a `VMDisk` and asserts the `DataVolume`
behind it (`hack/e2e-chainsaw/vminstance/vmdisk.yaml`,
`vmdisk-vmi.yaml`). Same defect class as the etcd edge above, on the
mapping side rather than the graph side, and with no production blast
radius of its own, since `src_to_suites` is read by the selector and by
nothing else.

**`cozystack.cozystack-basics` joins `cozystack.cozystack-engine` as a
propagation hub excluded from the reverse-dependency walk.** Every edge
into it exists so a namespace or a platform-wide policy is in place
before the dependent installs, which `kubevirt-cdi` states in its own
source ("Depend on cozystack-basics so the target namespace exists
first"), and that is install ordering rather than behaviour. The damage
runs opposite to the engine's: the engine fans one change out to every
app, basics narrows instead. It reaches no suite today so it escalates,
but it sits upstream of `kubevirt-cdi`, so the first suite to land under
CDI silently converts the platform's namespace-and-policy package from
the full run to that one suite. Not hypothetical — on #3426 enabling the
site-router suite takes `cozystack-basics` from 22 suites to
`site-router` alone, and nothing in the output says coverage was lost.
Dropping the reverse edges keeps the package reachable, stops it
propagating, and leaves a change to it running everything through the
per-path escalation.

**`hack/e2e-apps/<name>.bats` maps to the `<name>` suite**, off the
basename, exactly as the per-suite rule takes the name off a
`hack/e2e-chainsaw/<app>/` directory. Deliberately mapped rather than
marked inert: what remains there is wired to nothing after the Chainsaw
migration, and inert would bake that 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, so an unmatched
one escalates there and then rather than having the verdict deferred to
the final intersection, where the rest of the diff would decide it.

**`packages/tests/` and three `.gitattributes` join
`inert_config_pattern`,** which is what the script's own header says to
do with a genuinely inert path instead of widening the fall-through.
`packages/tests/` is a helm-unittest fixture chart, and changing a test
*of* `cozy-lib` does not change `cozy-lib`, no PackageSource lists those
paths as a component, and nothing installs them, while a change to the
library itself still escalates through `packages/library/`. The
`.gitattributes` entries are enumerated rather than matched by filename,
because the justification is what those files contain (only
`linguist-generated` markers) and the name does not carry it:
`.gitattributes` can also set `filter`, `eol`, `working-tree-encoding`
and `export-subst`, each of which changes what lands in the working tree
and therefore what gets built.

**`full_suite_pattern` escalates only `hack/e2e-*.bats`, not every
`hack/*.bats`.** The root `Makefile` is the authority on the split and
draws it at exactly that prefix, `BATS_UNIT_FILES := $(filter-out
hack/e2e-%.bats,$(wildcard hack/*.bats))`, so the 60 files it keeps are
the unit lane and the e2e sandbox runs none of them, while the three it
filters out are what `packages/core/testing`'s recipes execute and those
still escalate. Being inert here does not leave them untested: `make
unit-tests` 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
reads the same output rather than the selection.

## Measured effect

The before-and-after comparison below is a second measurement over a
slightly different population, stated separately because the two are not
interchangeable: it replays the last 150 first-parent merge commits on
`main` (`git diff --name-only $sha^1 $sha`) through the base selector
with base sources versus this branch with its own, where the histogram
above walks the 150 merged pull requests as GitHub lists them. The
populations overlap heavily and the direction is the same, and the
totals differ by two commits.

| | full suite | scoped | nothing |
|---|---|---|---|
| before | 120 | 20 | 10 |
| after | 106 | 25 | 19 |

Full-suite rate 80.0% → 70.7%, verdict changed on 15 commits. Fourteen
de-escalated and I read every one of their file lists: they are
unit-lane bats files, non-e2e workflows, `docs/`, `CODEOWNERS`,
helm-unittest fixtures and linguist markers. The fifteenth gets one
suite *wider*: a `cert-manager` change now also selects `etcd`, which is
correct, since `cert-manager` is a dependency of `etcd-operator` and the
etcd app now genuinely reaches it through the new edge.

That replay predates the two reachability rules, and replaying the same
150 first-parent merges across those two on their own — the selector at
`73bc4c0ae` against the selector at `adf231513`, same sources on both
sides — changes no verdict at all. Two of the 150 touch
`cozystack-basics` and both ran the full suite before and after, which
is the hub exclusion keeping something true rather than failing to do
anything, and none of the 150 touch `kubevirt-cdi` or `vm-disk`, so the
mapping shows up only in a direct selection.

A few individual selections, before → after:

| changed files | before | after |
|---|---|---|
| `hack/select-e2e_test.bats` | 21 suites | *nothing* |
| `hack/select-e2e_test.bats` +
`packages/apps/redis/templates/redis.yaml` | 21 suites | `redis` |
| `hack/e2e-install-cozystack.bats` | 21 suites | 21 suites |
| `packages/tests/cozy-lib-tests/**` | 21 suites | *nothing* |
| `packages/system/.gitattributes` | 21 suites | *nothing* |
| `hack/e2e-apps/postgres.bats` | 21 suites | `postgres` |
| `packages/system/etcd-operator/values.yaml` | 21 suites | `etcd` |
| `packages/system/kubevirt-cdi/values.yaml` | 21 suites | `vminstance`
|
| `packages/apps/vm-disk/values.yaml` | 21 suites | `vminstance` |
| `packages/system/cozystack-basics/values.yaml` | 21 suites | 21 suites
|

## Blast radius of the `dependsOn` edge

A `dependsOn` on a PackageSource is an install-ordering edge in
production, not only a test-selection hint, so this is the part worth
reviewing hardest. Both Packages are emitted by
`packages/core/platform/templates/bundles/system.yaml` under the single
`bundles.system.enabled` guard with no intervening conditional between
them, in every variant, so no supported configuration has the app
without the operator. `hack/select-install.sh --validate` reports the
graph still has no cycle and nothing dangling. The one new exposure is
an admin who lists `cozystack.etcd-operator` in
`bundles.disabledPackages` while keeping the app, which now leaves it
`DependenciesNotReady`. That is the same exposure postgres, mariadb,
kafka and redis already carry, with `Package.spec.ignoreDependencies` as
the escape hatch. On upgrade `etcd-rd` waits for the operator's two
HelmReleases, and an unhealthy `etcd-operator` already means broken etcd
apps.

## Testing

Every rule added or changed has a test in `hack/select-e2e_test.bats`,
the install-side change has one in `hack/select-install_test.bats`, and
the new graph edge has a helm-unittest guard at
`packages/core/platform/tests/sources_etcd_application_dependson_test.yaml`.
The round-trip test that walks every suite through both mapping tables
stays green, `vminstance` included now that two sources map onto it.

The two reachability rules were each checked by removing the rule and
confirming its own test reddens: without the mapping the CDI test falls
back to the full-suite escalation, and without the hub exclusion
`cozystack-basics` selects `redis vminstance` instead of everything. The
hub test has to seed its own hazard, since no suite sits downstream of
basics in this tree yet, so it gives `redis-application` an edge onto
basics in a copied sources directory and pins that a redis change still
selects redis, or the assertion would pass against a graph that resolved
nothing.

`make bats-unit-tests` aborts on the first failing file (`for f in …; do
… || exit 1; done`), and `hack/ghcr-mirror_test.bats` fails on `main`
today and sorts before `select-e2e_test.bats`, so the aggregate target
never reaches these tests. I ran the 60 unit files individually instead:
59 pass, and the one failure is that pre-existing one, byte-identical to
the base commit and referencing nothing this branch touches. Also green:
`make helm-unit-tests`, `make rd-presets-check migrations-target-check
test-check-readiness`, `hack/select-install.sh --validate`, and
`pre-commit run --all-files` leaving a clean tree.

## Two things a reviewer should know, neither addressed here

`assert_selection` in `hack/select-e2e_test.bats` aborts the whole file
with `exit 1` rather than failing a single test. That is the only thing
that works under `hack/cozytest.sh`, which provides no real `run`,
`$status` or `skip`, and it is the idiom the existing tests in that file
already use. It becomes wrong when #3497 and #3498 flatten these onto
real `bats(1)`, where a failing assert should end one test and let the
rest report. Worth converting with that work rather than ahead of it.

`cozystack.mongodb-application` carries the identical missing operator
edge: `packages/apps/mongodb` renders `psmdb.percona.com/v1` while the
source depends only on `cozystack.networking` and
`cozystack.cozystack-engine`, so `cozystack.mongodb-operator` still
reaches no runnable suite and every change to it escalates to all 21.
Same defect class and same fix shape as the etcd edge, but it is a
second production install-ordering change and belongs in its own pull
request.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- The default etcd application now includes its operator and required
platform dependencies, ensuring the EtcdCluster provider is available.

- **Bug Fixes**
- Improved end-to-end test selection for application-specific changes,
shared components, inert paths, and unresolved or unclassified files.
- Full-suite escalations now provide clear reasons through standard
error.

- **Tests**
- Expanded coverage for test selection, dependency reachability, and
etcd application dependency declarations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Andrei Kvapil (kvaps) pushed a commit that referenced this pull request Sep 7, 2026
## Why

I replayed the 150 most recently merged pull requests through
`hack/select-e2e.sh` with every escalation branch instrumented. 118 of
them (78.7%) ran the full 21-suite Chainsaw run, and the causes, counted
once per escalating PR with the count where that cause was the only one
in brackets, were `FULL_PATTERN` 88 [68], `NO_SUITE_FOR_GROUP` 24 [24],
`CHAINSAW_SHARED` 18 [1] and `UNCLASSIFIED` 7 [4]. `YQ_BROKEN`,
`NO_GRAPH_OWNER` and `BACKSTOP_EMPTY` never fired.

Two problems come out of that. Most of the escalation is real but
unnecessary. Whole classes of path that provably cannot affect a
Chainsaw suite were escalating because no rule claimed them. And the
commonest cause of a full run was absent from the log, so "why did this
pull request run everything" could only be answered by re-deriving the
selection by hand.

## What changes

**Every escalation now names its cause on stderr.** Seven branches could
reach the full suite and four did it in silence, `full_suite_pattern`
among them, the commonest cause by a wide margin, so the usual answer
was the one the log never carried. The lines go to stderr and must stay
there: stdout is the suite list and both e2e lanes parse it, so a reason
line on stdout would be read as a suite name. A unit test asserts every
message, verified by muting each of the eleven in turn and confirming
the file goes red for all of them. A reason that regresses to silence
changes no selection and is otherwise invisible.

**`cozystack.etcd-application` gains its missing `dependsOn:
cozystack.etcd-operator`.** `packages/extra/etcd` renders `kind:
EtcdCluster` from `etcd-operator.cozystack.io/v1alpha2`, and this was
the only operator-backed application source in the tree with no edge to
its operator. It is a latent install bug as much as a selection gap:
without the edge `etcd-rd` registers the ApplicationDefinition as soon
as the engine is up, so a tenant can create an `Etcd` before the
operator exists and its HelmRelease fails on `no matches for kind
"EtcdCluster"`, and `hack/select-install.sh`'s forward closure for the
`etcd` suite omitted the operator for the same reason. On the selection
side `cozystack.etcd-operator` reached no runnable suite, so every
change to the operator ran all 21.

**`vm-disk-application` maps to the `vminstance` suite.** It resolved to
a `vm-disk` suite that does not exist, `intersect_suites()` dropped the
name, and the source reached nothing runnable. Neither did
`cozystack.kubevirt-cdi` above it, whose only other dependents are
`vm-default-images` and `vm-disk-application` itself, so every CDI
change ran all 21. The coverage was there and the table did not know it:
the `vminstance` suite creates a `VMDisk` and asserts the `DataVolume`
behind it (`hack/e2e-chainsaw/vminstance/vmdisk.yaml`,
`vmdisk-vmi.yaml`). Same defect class as the etcd edge above, on the
mapping side rather than the graph side, and with no production blast
radius of its own, since `src_to_suites` is read by the selector and by
nothing else.

**`cozystack.cozystack-basics` joins `cozystack.cozystack-engine` as a
propagation hub excluded from the reverse-dependency walk.** Every edge
into it exists so a namespace or a platform-wide policy is in place
before the dependent installs, which `kubevirt-cdi` states in its own
source ("Depend on cozystack-basics so the target namespace exists
first"), and that is install ordering rather than behaviour. The damage
runs opposite to the engine's: the engine fans one change out to every
app, basics narrows instead. It reaches no suite today so it escalates,
but it sits upstream of `kubevirt-cdi`, so the first suite to land under
CDI silently converts the platform's namespace-and-policy package from
the full run to that one suite. Not hypothetical — on #3426 enabling the
site-router suite takes `cozystack-basics` from 22 suites to
`site-router` alone, and nothing in the output says coverage was lost.
Dropping the reverse edges keeps the package reachable, stops it
propagating, and leaves a change to it running everything through the
per-path escalation.

**`hack/e2e-apps/<name>.bats` maps to the `<name>` suite**, off the
basename, exactly as the per-suite rule takes the name off a
`hack/e2e-chainsaw/<app>/` directory. Deliberately mapped rather than
marked inert: what remains there is wired to nothing after the Chainsaw
migration, and inert would bake that 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, so an unmatched
one escalates there and then rather than having the verdict deferred to
the final intersection, where the rest of the diff would decide it.

**`packages/tests/` and three `.gitattributes` join
`inert_config_pattern`,** which is what the script's own header says to
do with a genuinely inert path instead of widening the fall-through.
`packages/tests/` is a helm-unittest fixture chart, and changing a test
*of* `cozy-lib` does not change `cozy-lib`, no PackageSource lists those
paths as a component, and nothing installs them, while a change to the
library itself still escalates through `packages/library/`. The
`.gitattributes` entries are enumerated rather than matched by filename,
because the justification is what those files contain (only
`linguist-generated` markers) and the name does not carry it:
`.gitattributes` can also set `filter`, `eol`, `working-tree-encoding`
and `export-subst`, each of which changes what lands in the working tree
and therefore what gets built.

**`full_suite_pattern` escalates only `hack/e2e-*.bats`, not every
`hack/*.bats`.** The root `Makefile` is the authority on the split and
draws it at exactly that prefix, `BATS_UNIT_FILES := $(filter-out
hack/e2e-%.bats,$(wildcard hack/*.bats))`, so the 60 files it keeps are
the unit lane and the e2e sandbox runs none of them, while the three it
filters out are what `packages/core/testing`'s recipes execute and those
still escalate. Being inert here does not leave them untested: `make
unit-tests` 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
reads the same output rather than the selection.

## Measured effect

The before-and-after comparison below is a second measurement over a
slightly different population, stated separately because the two are not
interchangeable: it replays the last 150 first-parent merge commits on
`main` (`git diff --name-only $sha^1 $sha`) through the base selector
with base sources versus this branch with its own, where the histogram
above walks the 150 merged pull requests as GitHub lists them. The
populations overlap heavily and the direction is the same, and the
totals differ by two commits.

| | full suite | scoped | nothing |
|---|---|---|---|
| before | 120 | 20 | 10 |
| after | 106 | 25 | 19 |

Full-suite rate 80.0% → 70.7%, verdict changed on 15 commits. Fourteen
de-escalated and I read every one of their file lists: they are
unit-lane bats files, non-e2e workflows, `docs/`, `CODEOWNERS`,
helm-unittest fixtures and linguist markers. The fifteenth gets one
suite *wider*: a `cert-manager` change now also selects `etcd`, which is
correct, since `cert-manager` is a dependency of `etcd-operator` and the
etcd app now genuinely reaches it through the new edge.

That replay predates the two reachability rules, and replaying the same
150 first-parent merges across those two on their own — the selector at
`73bc4c0ae` against the selector at `adf231513`, same sources on both
sides — changes no verdict at all. Two of the 150 touch
`cozystack-basics` and both ran the full suite before and after, which
is the hub exclusion keeping something true rather than failing to do
anything, and none of the 150 touch `kubevirt-cdi` or `vm-disk`, so the
mapping shows up only in a direct selection.

A few individual selections, before → after:

| changed files | before | after |
|---|---|---|
| `hack/select-e2e_test.bats` | 21 suites | *nothing* |
| `hack/select-e2e_test.bats` +
`packages/apps/redis/templates/redis.yaml` | 21 suites | `redis` |
| `hack/e2e-install-cozystack.bats` | 21 suites | 21 suites |
| `packages/tests/cozy-lib-tests/**` | 21 suites | *nothing* |
| `packages/system/.gitattributes` | 21 suites | *nothing* |
| `hack/e2e-apps/postgres.bats` | 21 suites | `postgres` |
| `packages/system/etcd-operator/values.yaml` | 21 suites | `etcd` |
| `packages/system/kubevirt-cdi/values.yaml` | 21 suites | `vminstance`
|
| `packages/apps/vm-disk/values.yaml` | 21 suites | `vminstance` |
| `packages/system/cozystack-basics/values.yaml` | 21 suites | 21 suites
|

## Blast radius of the `dependsOn` edge

A `dependsOn` on a PackageSource is an install-ordering edge in
production, not only a test-selection hint, so this is the part worth
reviewing hardest. Both Packages are emitted by
`packages/core/platform/templates/bundles/system.yaml` under the single
`bundles.system.enabled` guard with no intervening conditional between
them, in every variant, so no supported configuration has the app
without the operator. `hack/select-install.sh --validate` reports the
graph still has no cycle and nothing dangling. The one new exposure is
an admin who lists `cozystack.etcd-operator` in
`bundles.disabledPackages` while keeping the app, which now leaves it
`DependenciesNotReady`. That is the same exposure postgres, mariadb,
kafka and redis already carry, with `Package.spec.ignoreDependencies` as
the escape hatch. On upgrade `etcd-rd` waits for the operator's two
HelmReleases, and an unhealthy `etcd-operator` already means broken etcd
apps.

## Testing

Every rule added or changed has a test in `hack/select-e2e_test.bats`,
the install-side change has one in `hack/select-install_test.bats`, and
the new graph edge has a helm-unittest guard at
`packages/core/platform/tests/sources_etcd_application_dependson_test.yaml`.
The round-trip test that walks every suite through both mapping tables
stays green, `vminstance` included now that two sources map onto it.

The two reachability rules were each checked by removing the rule and
confirming its own test reddens: without the mapping the CDI test falls
back to the full-suite escalation, and without the hub exclusion
`cozystack-basics` selects `redis vminstance` instead of everything. The
hub test has to seed its own hazard, since no suite sits downstream of
basics in this tree yet, so it gives `redis-application` an edge onto
basics in a copied sources directory and pins that a redis change still
selects redis, or the assertion would pass against a graph that resolved
nothing.

`make bats-unit-tests` aborts on the first failing file (`for f in …; do
… || exit 1; done`), and `hack/ghcr-mirror_test.bats` fails on `main`
today and sorts before `select-e2e_test.bats`, so the aggregate target
never reaches these tests. I ran the 60 unit files individually instead:
59 pass, and the one failure is that pre-existing one, byte-identical to
the base commit and referencing nothing this branch touches. Also green:
`make helm-unit-tests`, `make rd-presets-check migrations-target-check
test-check-readiness`, `hack/select-install.sh --validate`, and
`pre-commit run --all-files` leaving a clean tree.

## Two things a reviewer should know, neither addressed here

`assert_selection` in `hack/select-e2e_test.bats` aborts the whole file
with `exit 1` rather than failing a single test. That is the only thing
that works under `hack/cozytest.sh`, which provides no real `run`,
`$status` or `skip`, and it is the idiom the existing tests in that file
already use. It becomes wrong when #3497 and #3498 flatten these onto
real `bats(1)`, where a failing assert should end one test and let the
rest report. Worth converting with that work rather than ahead of it.

`cozystack.mongodb-application` carries the identical missing operator
edge: `packages/apps/mongodb` renders `psmdb.percona.com/v1` while the
source depends only on `cozystack.networking` and
`cozystack.cozystack-engine`, so `cozystack.mongodb-operator` still
reaches no runnable suite and every change to it escalates to all 21.
Same defect class and same fix shape as the etcd edge, but it is a
second production install-ordering change and belongs in its own pull
request.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- The default etcd application now includes its operator and required
platform dependencies, ensuring the EtcdCluster provider is available.

- **Bug Fixes**
- Improved end-to-end test selection for application-specific changes,
shared components, inert paths, and unresolved or unclassified files.
- Full-suite escalations now provide clear reasons through standard
error.

- **Tests**
- Expanded coverage for test selection, dependency reachability, and
etcd application dependency declarations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant