test(tenant): anchor the operator-gate absence and refresh a spent reason - #3661

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/tenant/tests/cleanup_role_watch_test.yaml`:
- Around line 20-23: The comments in both suite-level templates lists must
explain that the list limits rendering so failures in unrelated templates cannot
affect the suite. In packages/apps/tenant/tests/cleanup_role_watch_test.yaml
lines 20-23, identify the cleanup template as the target under test; in
packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml lines 14-17, identify
the listed HelmRelease templates as the render scope and retain the explanation
that offline lookup returns empty.

Overview

This is a test-only change to two helm-unittest suites in packages/apps/tenant/tests/. It does two things:

1. Anchors the "no operator gate" negative assertions for the seaweedfs and gateway HelmReleases in vmo_webhook_dependson_test.yaml by asserting metadata.name before notExists: spec.dependsOn.
2. Refreshes stale header comments in both suites that attributed the templates: allowlist to namespace.yaml's lookup erroring under helm-unittest.

Verdict: LGTM with non-blocking notes

Why the anchoring is correct, not cosmetic: templates/seaweedfs.yaml and templates/gateway.yaml are both conditionally rendered ({{- if .Values.seaweedfs }} / {{- if eq (include "tenant.gatewayEffective" .) "true" }}). A bare notExists: spec.dependsOn passes just as well when the template renders no document at all as when it renders a document without the field — so if the gating value key were ever renamed or the helper changed, the suite would stay green while silently testing nothing. Pinning metadata.name first closes that hole. This also brings the two remaining suites in the chart in line with no_upgrade_force_test.yaml, which already anchors all seven of its releases the same way — this isn't a new idiom, it's catching up to an existing one.

Why the comment refresh is correct: the old rationale (keycloakgroups.yaml indexing .Values._cluster unguarded, failing a no-values render) is verifiably stale — the current template guards with index (.Values._cluster | default dict) "oidc-enabled". The new rationale (namespace.yaml's lookup returns empty offline rather than erroring, so the template still renders) matches that template's own doc-comment and is independently corroborated by the already-merged no_upgrade_force_test.yaml, which states the same correction.

CI corroborates this statically: "Unit & controller tests" (which executes these exact suites) is green at head, so the new asserts pass against the real templates, not just against the PR's stated reasoning.

Non-blocking notes

1. CodeRabbit flagged that the refreshed wording ("...so what reddens here is the hook/gate it guards...") is unusual phrasing that doesn't explicitly spell out the templates: allowlist's exclusion mechanism. That's still present verbatim at head — worth a small wording pass for a future reader unfamiliar with the term, but it doesn't affect correctness.
2. The "E2E (in-tree)" check is red on this PR, but the failure is in "Wait for Cluster-API provider deployments" (cluster-api provider readiness/liveness probe failures during install) — an unrelated subsystem from the tenant chart test fixtures this diff touches. Flagging for visibility only; not attributable to this change.

Closing

Small, well-scoped, and the reasoning holds up against the actual templates and sibling test file. Approving with the two notes above for optional follow-up.

The absence assertions were genuinely vacuous and the fix closes it in both directions, which I checked rather than took from the description.

Threat A, gate becomes present: adding spec.dependsOn to templates/seaweedfs.yaml reddens asserts[1] notExists fail. Threat B is the vacuity itself, and it is the interesting one. templates/seaweedfs.yaml:1 gates on .Values.seaweedfs and templates/gateway.yaml:1 on tenant.gatewayEffective, so renaming both gating values makes the templates render zero documents. On the pre-PR file that leaves the suite at 5 passed, 5 total, still claiming to police two releases that had vanished from the chart entirely. On this PR it gives 2 failed with Expected to equal: seaweedfs / Actual: no manifest found. Threat C, a second document carrying the gate, also reddens with DocumentIndex: 1, so notExists scopes across all documents of the named template and the fix has no multi-document blind spot.

The spent reason really has expired. packages/apps/tenant/templates/keycloakgroups.yaml:8 now reads index (.Values._cluster | default dict) "oidc-enabled", so the old rationale is dead. I then checked the new comment is not itself an empty claim, by injecting a nil deref into a template neither suite asserts on: with the templates: list both suites pass 13 of 13, without it both fail on index of untyped nil. So the prophylaxis is real rather than decorative.

The package is genuinely exercised, packages/apps/tenant/Makefile has a test: target and hack/helm-unit-tests.sh probes for it, so this suite is not in the silently-skipped class. Full chart run is 10 suites, 43 tests, green.

One thing to know rather than change. The equal: metadata.name pin also asserts that seaweedfs.yaml renders exactly one document, so adding a legitimate second document there reddens this suite. That is arguably the right default since it forces a conscious update, and it matches no_upgrade_force_test.yaml:18-28 which already carries this pattern, so I would leave it. The assertion is just doing two jobs and the name only mentions one.

After this the tenant chart is clean on this class. The other notExists uses are each preceded by a hasDocuments count or a documentIndex equal in the same test.