Skip to content

test(recfilter): pin the duplicate-guard budget ceil boundary - #490

Merged
cristim merged 1 commit into
mainfrom
test/238-recfilter-ceil-boundary
Oct 17, 2026
Merged

cristim merged 1 commit into
mainfrom
test/238-recfilter-ceil-boundary

Conversation

@cristim

@cristim cristim commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Summary

Test-only, the pkg/recfilter part of #238. elastiCacheCover.consume (pkg/recfilter/dedupe.go:190-194) rounds the reserved nodes spent up ((covered*reserved + nodes - 1) / nodes), but no test pinned the exact-spend boundary, so the mutant spent = covered survived.

Two cases added to TestElastiCacheEngineBudgets:

  • 8 Redis nodes vs two 5-node Valkey recommendations (8 Redis = 10 Valkey): each spends exactly 4, so nothing is bought. With spent = covered each spends 5, leaving 3, and 2 nodes are bought.
  • 4 Redis nodes vs five 1-node Valkey recommendations: each spends ceil(4/5) = 1, so the fifth is bought. Rounding down would spend nothing and cover all five.

Mutation probes (scratch copy of pkg):

  • spent = covered: the new exact-spend case fails (previously no test failed).
  • Floor division: the new single-node case fails, plus three existing cases.

Refs #238. The pkg/httpclient Unmap() part is covered by #473; whether to block 64:ff9b:1::/48 and Teredo 2001::/32 stays a decision on the issue.

Verification

(cd pkg && GOWORK=off go test -race -short ./recfilter/...), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks pass. Non-author review of the 4-line diff checked both cases' arithmetic against consume.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Added coverage for Redis-to-Valkey budget allocation, including cases where available capacity fully covers recommendations and where per-recommendation rounding limits coverage.

@cristim cristim added impact/internal Team-internal only type/chore Maintenance / non-user-visible priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner triaged Item has been triaged labels Oct 17, 2026
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: d2a59b0b-54a4-403c-8c90-98b3104138db
📥 Commits

Reviewing files that changed from the base of the PR and between c479af1 and b371b94.

📒 Files selected for processing: 1
  • pkg/recfilter/dedupe_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

Adds two budget test cases for Redis-to-Valkey coverage. The cases check coverage of multi-node recommendations and per-recommendation rounding for single-node recommendations.

Changes: Valkey budget tests

Layer / File(s) Summary
Redis-to-Valkey budget coverage
pkg/recfilter/dedupe_test.go
Adds a case where eight Redis units cover two five-node Valkey recommendations. Adds a case where four Redis units cover four of five single-node recommendations after spending is rounded up per recommendation.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to b371b

The change adds focused regression coverage for Redis-to-Valkey budget boundaries, with no identified merge-blocking risk.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the added recfilter tests for the duplicate-guard budget ceiling boundary.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Independent review: CLEAN at 5e144100056de2ff57c81725a4750ae52173f956. Reviewer: claude-go-pr-merge-4 (merge driver, not the author). The PR body was treated as a claim.

The change is test-only (+4 lines, pkg/recfilter/dedupe_test.go). I checked both cases' arithmetic by hand against consume (dedupe.go:190-194; covered = min(available*nodes/reserved, want), spent = ceil(covered*reserved/nodes), Redis to Valkey 4:5):

  • 8 Redis vs Valkey 5+5. Each rec covers 5 and spends (20+4)/5 = 4. 8 is fully used and nothing is bought. With spent = covered, the first rec spends 5, the second covers 3*5/4 = 3, and 2 are bought.
  • 4 Redis vs five 1-node Valkey recs. Each covers 1 and spends (4+4)/5 = 1. After four recs the budget is 0, so the fifth (1 node) is bought.

Mutation probes, run locally in a scratch copy and reverted:

  • spent = covered makes redis_budget_covers_two_exact_valkey_recs FAIL. The PR says this mutant survived before.
  • Floor division (covered*reserved/nodes) makes redis_spent_rounds_up_per_single_valkey_rec FAIL, along with 3 existing cases.

Pass-after: (cd pkg && GOWORK=off go test -race -short -count=1 ./recfilter/...) passes (252 tests). Labels mirror #238. CodeRabbit is quota-limited; if that holds at merge: CR waived: quota, adversarial review + local verification + green CI.

Adds an 8 Redis vs two 5-node Valkey recs case (exact 4-node spend each,
expects 0 purchases) and a single-node-per-rec case. The mutant
`spent = covered` now fails the first; the floor-division mutant fails
both new cases (and the existing rounds-up cases).

Refs #238

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the test/238-recfilter-ceil-boundary branch from 5e14410 to b371b94 Compare October 17, 2026 01:12
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Updated onto main c479af1 (after #486) using server-side update-branch. New head b371b94eeb29d3bda18dbad3c285a7d7165d9cf1. git range-diff shows 5e14410 = b371b94, identical, so the CLEAN verdict carries over. CI had passed on 5e14410 and is re-running. CR waived: quota, adversarial review + local verification + green CI.

@cristim
cristim merged commit 2bf23fb into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the test/238-recfilter-ceil-boundary branch October 17, 2026 01:23
@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Merged by squash at reviewed head b371b94eeb29d3bda18dbad3c285a7d7165d9cf1 as 2bf23fbd. The gates were met on that SHA:

  • independent review CLEAN, posted above (carried across rebases by identical git range-diff)
  • local verification posted above
  • CI green on b371b94e
  • labels mirrored; no unresolved threads

CR waived: quota, adversarial review + local verification + green CI. A retrospective CodeRabbit review will run when quota returns. Refs #238 (the issue stays open for the 64:ff9b:1::/48 and Teredo decision).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant