test(e2e): assert cluster teardown against the cluster the spec deletes

[melancholictheory]

Found while looking at the e2e suite for #371. There is no separate issue for it, so I can open one if the project prefers a PR per issue.

Summary

The teardown spec in the ValkeyCluster Ordered container asserts on a different cluster than the one it deletes, so it passes while the cluster it claims to have removed is still running.

Features / Behaviour Changes

Test only. No operator behaviour changes.

Implementation

"deletes the Valkey Cluster deployment" deletes config/samples/v1alpha1_valkeycluster.yaml, which is cluster-sample, then asserts on the shared valkeyClusterName. That variable is declared once for the whole Ordered container and reassigned by the scale-out and scale-in specs that run before it (lines 669 and 792 on main), so by the time the teardown spec runs it holds valkeycluster-scalein. The scale-in spec already deleted that cluster in its own defer with --wait=false.

All four Eventually blocks (CR, Service, ConfigMap, ValkeyNodes) therefore check a cluster that is already gone, and removal of cluster-sample is never verified. Under a label-filtered run, which #373 is heading toward, the variable would be empty and the assertions would pass vacuously again.

This names the cluster in the spec so the assertions describe what the spec actually deletes.

Limitations

This fixes the one spec. The wider coupling is still there: valkeyClusterName is shared across the container and reassigned by three specs, which is what makes the container impossible to split by label. I wrote that up in #371 and can follow up with the decoupling if the approach looks right.

Testing

Verified on a local k3d cluster, running the assertions exactly as the spec does.

With cluster-sample present and its Service, ConfigMap and six ValkeyNodes all live, the old assertions (against valkeycluster-scalein) pass 4 out of 4, which is the bug: the spec is green while the cluster it should be checking is still up.

After kubectl delete -f config/samples/v1alpha1_valkeycluster.yaml, the new assertions (against cluster-sample) pass 4 out of 4.

The cascade does not depend on the operator: the ValkeyCluster carries no finalizers, and the Service, ConfigMap and ValkeyNodes all have ownerReferences pointing at it, so the API server garbage collects them.

Checklist

• This Pull Request is related to one issue.
• Commit message explains what changed and why
• Tests are added or updated.
• Documentation files are updated.
• I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 072acfc5-9d17-4664-b42a-2dfa6f5e364c

📥 Commits

Reviewing files that changed from the base of the PR and between 5676fb9 and eda95fc.

📒 Files selected for processing (1)

• test/e2e/valkeycluster_test.go

---

📝 Walkthrough

Walkthrough

The end-to-end deletion test now uses a local sampleClusterName constant for all deleted resource checks instead of the shared mutable variable.

Changes

Deletion test updates

Layer / File(s)	Summary
Use a local cluster name in deletion checks test/e2e/valkeycluster_test.go	The test defines sampleClusterName and uses it to verify deletion of the ValkeyCluster, Service, ConfigMap, and ValkeyNodes.

Suggested reviewers: jdheyburn

Merge Risk: ⚪ Minimal · up to eda95

This localized test-only change corrects teardown assertions to target the cluster being deleted; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the test change: teardown assertions now target the cluster deleted by the spec.
Description check	✅ Passed	The description includes all required sections and clearly explains the bug, implementation, limitations, testing, and checklist status.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: build linters: plugin(logcheck): plugin "logcheck" not found
The command is terminated due to an error: build linters: plugin(logcheck): plugin "logcheck" not found

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The teardown test now verifies removal of the cluster-sample ValkeyCluster defined by the sample manifest and its associated Service, ConfigMap, and ValkeyNodes. The checked identifiers match the controller’s generated names, and the E2E test package compiles successfully.

Confidence Score: 5/5

No blocking failure remains.

No accepted blocking findings remain after validating the manifest-derived resource names and compiling the changed E2E package.

T-Rex Logs

What T-Rex did

• Compared the base revision and this change with a manifest-derived identifier verifier and ran the controller naming helper; the E2E package compiled with the 'e2e' build tag passed.
• PR teardown identifier check passed, controller-generated teardown identifiers passed, and the full cluster runtime prerequisite check completed.
• All five target-consistency checks now pass for ValkeyCluster cluster-sample, Service valkey-cluster-sample, ConfigMap valkey-cluster-sample, and ValkeyNode selector valkey.io/cluster=cluster-sample; related diff whitespace and formatting checks and the E2E-package compile also exited 0.

_{Ran code and verified through T-Rex}

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/e2e-delete-..." | Re-trigger Greptile}

[jdheyburn]

Good find, thank you!

[aamirpatel]

test/e2e/valkeycluster_test.go: Consider deriving the cluster name from the sample YAML rather than hard-coding "cluster-sample" in the test. This will prevent drift if the sample name changes (e.g., parse the metadata.name field from config/samples/v1alpha1_valkeycluster.yaml or add a small helper to extract it).

[bjosv]

Good finding! We should restructure the tests in valkeycluster_test.go in the near future, to avoid these kind of problems.