mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-06 12:03:39 +09:00
Aleksei Sviridkin
9a0c8c75f3
## What this PR does Four fixes in e2e test selection and the helm unit sweep. Relates to #3650 #3652 #3655 #3646. select-e2e built both dependency indexes as `$(build_owners_index | sort -u)`, so it read sort's exit status and never yq's (#3650). A broken yq left the index empty, every path fell through to escalation, and CI ran the full suite on every PR. The output was identical to "no owners matched", so it looked like the conservatism the selector is supposed to apply. The suite list one line up had the same shape and a worse result. `$(find ... | sed | sort)` reports sort, never find, and errors went to /dev/null. Every escalation prints that list, so a failed find made "escalate to the full suite" emit a blank line, which both e2e lanes read as "skip Chainsaw" before posting `E2E Tests` green. The outcome that means run everything delivered a run of nothing. find is not the only way to get an empty list, and the escalations are not its only reader, so the check sits where the list is built. The `examples/backups/<app>/` rule escalates nothing and still membership-tests against that list: with it empty, an edit to a backup harness that should run its suite reports nothing to run, on a path no escalation-side guard would ever see. An empty list is a broken enumeration, so the script refuses to answer at all. Both lanes run the step under `bash -e`, so the non-zero exit fails the job. I checked both workflows: neither overrides `shell:`, both capture the selector as a plain assignment. Fourteen tests used `[ "$(echo "$output" | wc -w)" -gt 5 ]` to mean "everything was selected" (#3652). There are 21 suites, so that passes on any six, and partial escalation stayed green. They compare against the whole list now, derived the way the script's own full-suite branch derives it. Drop one suite from that branch and thirteen of them go red. All thirteen passed under the threshold. The fourteenth is the cilium case, and equality shows it was never a full-suite path at all. It resolves through the dependency graph, which reaches 19 of the 21 suites. The two it misses are kuberture and securitygroup, whose sources are not `*-application`, and the mapping covers only those plus external-dns by name. Both controllers declare `cozystack.networking`, so a CNI change that breaks either one runs neither suite. That gap is #3665. The test is renamed for what it measures and pins the 19, so closing #3665 turns it red at the right moment. Editing `hack/e2e-chainsaw/backup/chainsaw-test.yaml.disabled` selected the full suite (#3655). The suffix was ignored when deriving the suite name, the intersection against existing suites emptied, and the empty-selection safety net escalated. A disabled suite runs nowhere, so its edits are the one file class that cannot regress a test, and they were buying the most expensive outcome the selector has. The selector now has a fourth outcome, a failed step rather than a selection, so the contract enumerations in the script header, in `docs/agents/e2e-testing.md` and in both workflow comments say so. The helm unit sweep never visited `packages/tests` (#3646). cozy-lib-tests, the whole test coverage for a helper library that 34 charts consume by symlink, ran only because `packages/library/cozy-lib/Makefile` delegates its `test` target to it. Remove that one line and the suite goes from one execution to zero with the sweep still exiting 0 and still printing "All Helm unit tests passed". Every check the sweep makes runs inside a directory the loop already reached, so a package that never enters the loop takes all of them with it, and what is left is `tests_found`, which counts suites across the whole run rather than per package. The directory is on the list now: 76 executed suites become 77, at 0.1s to 0.3s over six runs against a sweep of roughly 31 seconds. The delegation stays for `make -C packages/library/cozy-lib test` ergonomics, so the suite runs twice. Neither run is load-bearing alone. ### Testing `hack/select-e2e_test.bats` goes from 27 tests to 32, `hack/helm-unit-tests.bats` is new. Each commit is green on its own: 27, 30, 32, 32. The new tests stub the failing component and assert the exit status plus the line naming what broke: a broken yq, a broken find, an empty suite list across all three diff classes, the disabled-suite path, and a guard that the disabled rule drops only that path and not the rest of the diff. The yq test asserts exit 0 on purpose. A missing dependency graph still has a correct answer, the full suite, and a missing suite list does not. Pinning both halves keeps the header and the docs from drifting into claiming otherwise. Two suites fail on my macOS box and fail identically at `origin/main`, so neither comes from this branch: `hack/migration-seaweedfs-db-adopt.bats`, where a container running as the host UID cannot write the bind-mounted temp dir under Colima, and `hack/seaweedfs-naming-audit.bats`, which I confirmed red at the merge base but did not dig into. Every other `hack/*.bats` was run individually and is green. `make unit-tests` stops at the first failing file and both of those sort ahead of `select-e2e_test.bats`, so on macOS the suites this PR touches have to be run directly. ### Named improvements, not done here - The `.disabled` rule keys on the file rather than the parked directory, so it is both wider and narrower than the property it wants. A support file under `hack/e2e-chainsaw/backup/` still buys a full run, and a `.disabled` file dropped inside a live suite directory now selects nothing where it used to select that suite. The property covering both is one level up: a suite directory holding no live `chainsaw-test.yaml` is inert, testable with the same `grep -Fxq` membership check the `examples/backups/*/*` arm already uses. It is not a free win, since a half-written suite directory escalates today and would then select nothing. A cheaper half step is narrowing the glob to `hack/e2e-chainsaw/*/chainsaw-test.yaml*.disabled`, because case globs cross `/` and the shipped pattern also matches nested paths. - `packages/tests/**` is not classified by the selector and reaches the full suite through the unclassified fall-through instead of a rule. Tracked in #3667. - `full_suite_list()` in the bats file cannot fail. cozytest.sh's awk rewrites a lone `}` into `return 0; }`, so a broken find inside the helper would return an empty expected set instead of a red test. Nothing false can pass today, because the script hard-fails on an empty suite list before any assert runs, but that helper is what every equality assert measures against. - The yq check runs before the changed-file list is read, so with a broken graph even a docs-only diff escalates. Measured: a broken yq plus a `docs/x.md` diff returns 21 suites at head against 0 at the merge base. Only reachable in an already broken state, but the graph is consulted at the component lookup and in the closure walk, so moving the check to first use would charge nothing to docs, dashboards and inert-meta diffs. ### Screenshots No UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: four files under `hack/`, one under `docs/agents/`, and comments in the two e2e workflows. Nothing under `hack/` was moved or renamed and no make-target contract changed, so the ccp entry does not fire. It gates on `hack/package.mk` and `hack/common-envs.mk`, neither of which is in the diff. The node prerequisites in `hack/e2e-prepare-cluster.bats` that ansible-cozystack mirrors by hand are untouched, and so is `.github/workflows/tags.yaml`. `docs/agents/` is internal contributor documentation, not a website source. No package was added, renamed or removed, and no `values.schema.json` or version enum changed, so the website and provider entries do not fire either. - [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): select-e2e no longer reports an empty selection when it cannot enumerate the Chainsaw suites, which made an escalation to the full suite indistinguishable from "skip Chainsaw" and left the E2E gate green with nothing run. A failure to list the suites is now named on stderr and fails the step, a yq failure is named and still escalates to the full suite, and an edit to a disabled Chainsaw suite is inert instead of escalating to a full run. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved end-to-end test selection for disabled, empty, or partially changed test suites. * Test selection now safely escalates to the full suite when suite discovery or dependency analysis fails. * Helm unit-test sweeps now include tests under `packages/tests`. * **Tests** * Added coverage for suite discovery failures, disabled suites, command failures, and exact test selection. * Added Helm test-sweep coverage, including failure handling. * **Documentation** * Clarified test-impact analysis rules and suite-selection outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->