chore(test): drop redundant *Eventually wrappers obsoleted by PR #898#902
Conversation
|
Warning Review limit reached
More reviews will be available in 8 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the obsolete helper functions doEventually, rpushEventually, and lpushEventually from adapter/test_util.go and replaces their usage in adapter/redis_lua_compat_test.go with direct Redis calls. This cleanup is possible because startup leader churn is now handled at the readiness layer (via waitForWriteableLeader introduced in PR #898). There are no review comments, so I have no feedback to provide.
|
@claude review PR-β: drop redundant *Eventually wrappers (obsoleted by PR #898's waitForWriteableLeader). gemini-code-assist already reviewed: "There are no review comments, so I have no feedback to provide." — no findings. Verify: (a) only 2 first-write-after-createNode sites (in redis_lua_compat_test.go) used the wrappers; both migrated to direct calls; (b) doEventually/rpushEventually/lpushEventually deleted, retryNotLeader kept (still used by grpc_test.go mid-loop). |
|
Claude finished @bootjp's task in 1m 14s —— View job Review: chore(test): drop redundant *Eventually wrappers
Verification resultsClaim (a): Only 2 first-write-after-createNode sites in Confirmed. require.NoError(t, rdb.RPush(ctx, "bull:test:wait", "job-1", "job-2", "job-3").Err())
require.NoError(t, rdb.LPush(ctx, "bull:test:active", "job-0").Err())These were the only two call sites. The comment on lines 124–130 accurately explains why direct calls now suffice (the Claim (b): Confirmed by repo-wide grep:
AssessmentThe change is correct and clean. The logic is sound: No issues found. LGTM. |
bootjp
force-pushed
the
chore/drop-redundant-eventually-helpers
branch
from
247996c to
b611ce9
Compare
PR #898 added waitForWriteableLeader to waitForRaftReadiness, which drives one no-op Raft entry through full quorum before createNode returns. The post-readiness leader-churn window the rpushEventually / lpushEventually wrappers absorbed is now closed at the readiness layer instead. The wrappers are dead code. Changes: - Drop rpushEventually + lpushEventually from test_util.go (the only two callers, in redis_lua_compat_test.go, were first-write -after-createNode sites — replaced with direct RPush/LPush calls + a comment pointing back to PR #898). - Drop doEventually too: its only callers were the two helpers above. retryNotLeader (the underlying classifier loop) stays — grpc_test.go still uses it for *mid-test* leader churn inside its 9999-iteration consistency loops, which PR #898 does not address. - Drop the now-unused redis import from test_util.go. - Refresh the leaderChurnRetryTimeout / retryNotLeader doc comments to call out that startup-window churn is closed at the readiness layer, and retryNotLeader is for mid-test loops. Caller audit per /loop semantic-change rule ============================================ - rpushEventually / lpushEventually: grep across the repo confirms zero remaining call sites after the migration. Safe to delete. - doEventually: only called by the two helpers above; also safe to delete. - retryNotLeader: kept. Sole external caller is adapter/grpc_test.go (9 sites inside two consistency-loop tests). All those wrap mid-test RPCs that can race a real re-election under CI load — the readiness probe does not help mid-test churn, so the helper retains its purpose. Validation ========== go test ./adapter/ -run TestRedis_LuaRPopLPushBullMQLikeLists -race -count=3 -> ok 5.1s (the direct RPush/LPush calls inherit the writeable-leader gate from createNode, no flake) gofmt + go vet + golangci-lint run -> 0 issues
bootjp
force-pushed
the
chore/drop-redundant-eventually-helpers
branch
from
b611ce9 to
46cf740
Compare
|
@claude review Rebased onto main after PR #903 (eventuallyExpired helper) merged. Conflict was in adapter/test_util.go where PR #903 added eventuallyExpired while this PR deleted doEventually/rpushEventually/lpushEventually in the same region. Resolution: kept eventuallyExpired (from main) and dropped the *Eventually trio (PR-β intent). Head: 46cf740. Rebased diff is 1 commit, only adapter/test_util.go + adapter/redis_lua_compat_test.go changes (same scope as before rebase, just on top of newer main). go test ./adapter/ -run 'TestRedis_LuaRPopLPushBullMQLikeLists|TestRedis_HGET_TTLExpired|TestLua_HGET_HashTTL' -race -count=2 passes. gofmt + golangci-lint clean. |
|
Claude finished @bootjp's task in 1m 0s —— View job Review: chore(test): drop redundant *Eventually wrappers (post-rebase onto #903)
Verification resultsRebase conflict resolution — correct.
Diff scope — unchanged from pre-rebase.
Import cleanup — correct. Minor note (description only, not code): The PR description says " AssessmentThe rebase was handled correctly. The intended change — dropping the |
…gatorRaceFree (#906) ## Summary Fixes the CI flake observed in [Actions run 26765510693](https://github.com/bootjp/elastickv/actions/runs/26765510693/job/78890657730?pr=902): ``` --- FAIL: TestHotKeysAggregatorRaceFree (0.11s) hot_keys_test.go:356: Expected value not to be nil. ``` Surfaced on PR #902's CI run but **unrelated to PR #902** — the failing test is on `main` in `keyviz/`, which PR #902 (Redis adapter `*Eventually` helper cleanup) doesn't touch. ## Root cause Same wall-clock-racing pattern as the SQS-throttle (PR #891) and Redis-TTL (PR #903) flake series. The test: 1. Launches an aggregator goroutine (Step=5ms tick). 2. Spawns 8 workers each calling `Observe` 500 times. 3. `time.Sleep(50 * time.Millisecond)`. 4. Asserts both per-route snapshots are non-nil. The aggregator publishes a snapshot only on a tick (or on ctx-done drain). Under `-race` on a slow CI runner, the `Run` goroutine itself can be slow to schedule — the 50ms wait may not contain a single actual tick worth of scheduler time, so `hotKeysSnap` stays at its initial `nil` `atomic.Pointer` load and the assertion fires. ## Fix `require.Eventually` with a 3-second budget + 5ms poll cadence, asserting both snapshots non-nil: ```go require.Eventually(t, func() bool { return s.HotKeysSnapshot(1) != nil && s.HotKeysSnapshot(2) != nil }, 3*time.Second, 5*time.Millisecond, "aggregator must publish a snapshot for each route within the budget") ``` The test still pins the load-bearing property (snapshot reads are lock-free; the `atomic.Pointer.Load` works without contention with the publisher); only the **wait mechanism** changes from "sleep blindly and hope" to "poll until ready or fail". Total time bound is at most 3s on a slow runner, ~5–50ms in the common case. ## Validation - `go test ./keyviz/ -run TestHotKeysAggregatorRaceFree -race -count=5 -timeout 120s` → ok 1.1s (5/5 iterations green) - `gofmt` + `golangci-lint run` → 0 issues ## Pattern continuity This is the same kind of structural fix as PR #891 / #903: **replace fixed-time-window sleep with `require.Eventually`** for any condition that depends on a background goroutine's progress under `-race` on CI. Future flakes in this category should follow the same pattern.
1 participant
Summary
Follow-up to PR #898. The readiness probe (
waitForWriteableLeader) closes the startup leader-churn window at the readiness layer, so the per-testrpushEventually/lpushEventuallywrappers that absorbed that window become dead code.What's dropped
rpushEventually+lpushEventually(test_util.go): only callers wereredis_lua_compat_test.golines 128–129 (first writes immediately aftercreateNode). Migrated to directrdb.RPush/rdb.LPushwith a comment pointing back to PR test(util): commit one no-op entry to prove leader writeable in waitForRaftReadiness #898.doEventually: only used by the two helpers above; no other callers.github.com/redis/go-redis/v9import intest_util.go.What's kept
retryNotLeader: still used ingrpc_test.go(9 sites) wrapping mid-test RPCs inside 9999-iteration consistency loops. The readiness probe does NOT help mid-test churn (a real re-election can fire halfway through a long-running loop), so this helper retains its purpose. The doc comment is refreshed to clarify the startup vs mid-test distinction.Caller audit (per /loop semantic-change rule)
Repo-wide grep:
rpushEventually\b/lpushEventually\b: 0 callers remain (only historical comment refs inredis_lua_compat_test.go).doEventually\b: 0 callers remain.retryNotLeader\b: 9 callers, all ingrpc_test.go. Unchanged.Validation
go test ./adapter/ -run TestRedis_LuaRPopLPushBullMQLikeLists -race -count=3→ ok 5.1s (direct RPush/LPush inherits the writeable-leader gate fromcreateNode; no flake)gofmt+go vet+golangci-lint run→ 0 issuesScope
Closes the "*Eventually helper unnecessization audit" follow-up noted in PR #898's description.