Repository navigation
test(valkey): add default render helm unit tests - #4346
Akanksha Trehun (magic-peach) wants to merge 1 commit into
Conversation
valkey was one of the remaining user-facing apps with no helm-unittest coverage. Signed-off-by: Akanksha Trehun <[email protected]>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
area/database
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Valkey app now has a ChangesValkey Helm test coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Akanksha Trehun (@magic-peach) NOT LGTM, only because of the PR description. The tests are fine and they can go red: I broke the chart once per assertion, and each of the 16 assertions failed alone, in the test I had predicted before the run. No code change is needed.
Business context: valkey ships to tenants without a helm-unittest suite, so a regression in its default render passes CI unnoticed. This PR adds the suite and the test: target the CI sweep looks for.
The blocker is the description. This repo uses the PR body verbatim as the merge commit message, so three sentences from it would land in git log wrong:
- "valkey was one of the eight user-facing apps in
packages/apps/called out in #3631". The issue lists foundationdb, http-cache, mongodb, rabbitmq, redis, tcp-balancer, vpc and vpn. valkey is not on that list. - "pinning the default Deployment render". The valkey chart renders no Deployment. The suite pins the auth Secret, the RedisFailover and the two WorkloadMonitors from
templates/valkeyfailover.yaml. - "it passes (3 assertions)". The file has 3 tests with 16 assertions.
Editing the description is enough, no push needed. Something like "valkey is a user-facing app in packages/apps/ with no helm-unittest coverage, the same gap #3631 describes for the eight apps it lists. This adds a suite pinning the default render of templates/valkeyfailover.yaml (auth Secret, RedisFailover, both WorkloadMonitors)", plus "3 tests, 16 assertions" further down. I'll approve after that.
Every mutation failed only the assertion it targeted. That covers the document count, the Secret's kind, name and password key, the RedisFailover's kind, engine, image, storage size and both replica counts, and kind, name and replicas on both WorkloadMonitors. Flipping the authEnabled default to false shifts every documentIndex by one. All three tests fail then, because each index group starts with an isKind.
CI will reach the suite. The Unit & controller tests job runs make unit-tests, which calls hack/helm-unit-tests.sh. That script runs every package where make -n test succeeds. At head the probe prints helm unittest ., and make -C packages/apps/valkey test prints the Test Suites: 1 passed line the script checks for. Against current main (41ef4a6) the branch merges cleanly, main has no test target for valkey, and the suite passes on the merged tree.
The image pin that blocked #4074 is not a problem here. valkey/valkey:8.1.8 comes from files/versions.yaml, which only this chart's own make update rewrites. The test file has no cozystack/cozystack ref and no rc version for the release rewrite to hit.
CI has not run the suite yet: Pre-Commit Checks and Pull Request wait for a maintainer to approve the fork run. I ran it locally with helm v4.3.0 and helm-unittest 1.1.1.
Not blocking: nothing pins the password carry-over at templates/valkeyfailover.yaml:2-5. That lookup is what keeps a tenant's password across upgrades, and deleting line 5 leaves the suite green. A kubernetesProvider fixture with an existing Secret covers it, the same way packages/apps/mongodb/tests/credentials_test.yaml does. I tried one locally and it fails with that line removed. Fine for a follow-up.
What this PR does
valkey was one of the eight user-facing apps in
packages/apps/called out in #3631 as shipping with zero helm-unittest coverage. This adds atests/suite pinning the default Deployment render and wires the standardtest: helm unittest .target the sibling charts already use, following the same pattern as #4081, #4074 and #4093.Ran
helm unittest .locally against the chart, it passes (3 assertions).make showneeds a live cluster connection throughcozyhr, which was not available in this environment, so I relied onhelm unittestfor local verification, same as the referenced sibling PRs.Screenshots
Not applicable, test-only change.
Downstream repositories
Walked the trigger map in docs/agents/contributing.md against the diff: this only adds a test file and a Makefile target inside an existing package, it does not add, rename or remove a package, and it does not touch
values.yaml,values.schema.json,Chart.yamlorREADME.md, so none of the listed triggers apply.Release note
Summary by CodeRabbit