mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 14:02:56 +09:00
Aleksei Sviridkin
3ad4d452fc
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `helm unittest` is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints `Test Suites: 0 passed, 0 total` and exits 0. `hack/helm-unit-tests.sh` judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the `tests/*_test.yaml` glob reports success having asserted nothing. That is measurable in both directions rather than argued. Move `packages/system/velero/tests/velero_test.yaml` aside and run the script as it exists on main today: ```text Running tests in packages/system/velero helm unittest . ### Chart [ cozy-velero ] . Charts: 1 passed, 1 total Test Suites: 0 passed, 0 total Tests: 0 passed, 0 total ``` The run ends with `All Helm unit tests passed.` and exit 0. The chart passed, and the count of things it checked was zero. With this PR the same tree exits 1 and names the package and the reason. Neither `--strict` nor an explicit non-matching `-f` glob changes this on its own; all three forms exit 0, checked against plugin 1.1.1. So the script now reads the captured output and fails the package when the zero-suite line appears. Matching that line names a cause and a remedy where it applies, but the set of ways to run nothing is not closed. A recipe that never invokes `helm unittest`, and a `test:` rule that make finds nothing to do for, print no marker of any kind and exit 0, so no additional match reaches them; enumerating absences leaves a fresh hole every time a new shape turns up. So the script also requires the line a real run always emits, a `Test Suites:` count of at least one. That turns the question round: every way of asserting nothing exits 0, and "what did this run assert" is the question that has an answer. All 76 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no `helm unittest` would need renaming or an opt-out, which is the same bargain the zero-suite check already strikes. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe ever firing. The check keys on what make reports rather than on the path existing, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so it accepts either opening quote; anchoring on the trailing quote alone would also match a sub-make reporting some other target whose name ends in `test`. That second check is guarding a live gap rather than a hypothetical one. `hack/package.mk` line 2 reads `.PHONY=help show diff apply delete update image`, which is a variable assignment and not a target declaration, so it makes nothing phony. Of the packages the script visits that define a `test` rule, 18 declare it phony in their own Makefile; the rest are unprotected if a path of that name ever appears. No such path exists today. Fixing the shared include is #3353 and is not this PR. One deliberate non-change, stated so it is not rediscovered as an oversight. Running the suite now captures output instead of streaming it, because a pipeline's exit status in POSIX `sh` reports the last command rather than `make`. Streaming reads better and loses the status, so it stays buffered. The run is pinned to `LC_ALL=C`, since both checks match English wording and a localized `make` would disarm one of them silently, which is the failure mode the whole change exists to remove. The guard is all-or-nothing, which is worth stating plainly. A chart that loses four of its five suites still reports `Test Suites: 1 passed` and passes; only total disappearance is caught. Tying the expected count to what each chart actually has would be a per-chart number to maintain, and a number maintained in one place while the suites move in another is the failure this repo has been paying for elsewhere, so the cheap check that catches the total loss is the one worth having. One behaviour worth stating because the message does not: a suite in which every test carries `skip:` reports `Test Suites: 0 passed, 1 skipped, 1 total` and exits 0, and the positive-evidence check refuses it. That refusal is intended, since a wholly skipped suite asserted nothing, but the message talks about suites expected under `tests/`, which in that case are present and skipped on purpose. No package is in that state today. Partial skips are unaffected: `2 passed, 1 skipped, 3 total` satisfies the check. Three known gaps in what the change ships, none of them a wrong statement. The positive-evidence message explains its cause but stops short of naming a remedy, where the other two messages both end in an action; the remedy for the legitimate case, renaming the target or giving it an opt-out, currently lives only in the code comment. That same message enumerates two ways a run reports nothing, and the skipped-suite case above is a third, so it reads as a diagnosis where it is really a list of the common causes. And no document in the tree states the convention this tightens: `docs/agents/overview.md` still describes the script accurately as running over every package that defines a `test` target, but the contract is now that such a package must also report at least one passing suite. All three are improvements to make on the next touch of these files rather than reasons to hold the change. One adjacent gap stays open, and it is worth naming rather than leaving someone to assume otherwise. This catches a chart whose suite *files* vanished; it does not catch a package that loses its `test:` rule along with them. Nothing ties the presence of `tests/*_test.yaml` to the presence of a `test` target, so a change that removes both is skipped in silence, and the script only complains when no package in the tree has a `test` target at all. The test named `a package with no test target is skipped, not failed` pins that permissiveness deliberately, because the many packages that legitimately define no `test` rule would otherwise turn every run red. Closing it needs an instrument keyed on the suite files rather than on the Makefile target, which is a separate change. `hack/helm-unit-tests.bats` adds eight tests, picked up automatically by the `hack/*.bats` glob in `make unit-tests`. Three pin the new refusals, and each matches its own message so none can stay green on a refusal that came from another. Two pin what must not be refused: a phony target with a colliding path, and a sub-make reporting an unrelated target up to date. One pins that a chart which does run suites still passes, which is what stops the refusals being satisfied by rejecting everything. Two pin behaviour that was already there, namely that a failing package is still reported by name and that a package with no `test` rule is skipped rather than failed. That last one builds a tree carrying a second package that does run a suite: with only the target-less package present, the script takes an early exit above the failure summary and the assertion would hold whatever the script had done. The fixtures stub `make` output instead of invoking helm, so they need no plugin installed. Relates to #3453. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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 <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tests): `make helm-unit-tests` now fails a chart that runs no Helm unit test suites, instead of reporting success for a chart whose suite files were deleted, moved, or renamed out of the `tests/*_test.yaml` glob ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Helm test validation to detect skipped test recipes, zero discovered suites, and runs without evidence of a passing suite. * Helm test failures now provide clearer diagnostics, including affected directories and failed commands. * Test output is consistently captured and replayed for easier troubleshooting. * **Tests** * Added comprehensive coverage for successful, failed, skipped, empty, and invalid Helm test scenarios. * Expanded coverage across package test targets, including phony and unrelated sub-make behavior. * **Documentation** * Clarified requirements for package test targets and suite reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai -->