From 46cf7404a626d9301bb5f4dbd33fe42dde975c4d Mon Sep 17 00:00:00 2001 From: "Yoshiaki Ueda (bootjp)" Date: Tue, 2 Jun 2026 00:44:23 +0900 Subject: [PATCH] chore(test): drop redundant *Eventually wrappers obsoleted by PR #898 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- adapter/redis_lua_compat_test.go | 15 ++++++---- adapter/test_util.go | 49 +++++++++----------------------- 2 files changed, 22 insertions(+), 42 deletions(-) diff --git a/adapter/redis_lua_compat_test.go b/adapter/redis_lua_compat_test.go index 525997f6f..a77bc631f 100644 --- a/adapter/redis_lua_compat_test.go +++ b/adapter/redis_lua_compat_test.go @@ -121,12 +121,15 @@ func TestRedis_LuaRPopLPushBullMQLikeLists(t *testing.T) { rdb := redis.NewClient(&redis.Options{Addr: nodes[0].redisAddress}) defer func() { _ = rdb.Close() }() - // Raft leadership can briefly churn right after createNode returns — the - // freshly-elected leader can step down if the initial heartbeats miss - // quorum on a slow CI runner (see waitForRaftReadiness). Retry the first - // writes so they ride out that re-election instead of failing the test. - rpushEventually(t, ctx, rdb, "bull:test:wait", "job-1", "job-2", "job-3") - lpushEventually(t, ctx, rdb, "bull:test:active", "job-0") + // Direct first writes: waitForRaftReadiness (called by createNode) + // now drives one no-op Raft entry through full quorum before + // returning, so any subsequent write is guaranteed to land on a + // leader that has already held quorum through one apply. The + // post-readiness leader-churn window the prior rpushEventually / + // lpushEventually wrappers absorbed is closed at the readiness + // layer instead — see PR #898. + 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()) result, err := rdb.Eval(ctx, ` local moved = redis.call("RPOPLPUSH", KEYS[1], KEYS[2]) diff --git a/adapter/test_util.go b/adapter/test_util.go index 744249a66..8fe5646fa 100644 --- a/adapter/test_util.go +++ b/adapter/test_util.go @@ -19,7 +19,6 @@ import ( pb "github.com/bootjp/elastickv/proto" "github.com/bootjp/elastickv/store" "github.com/cockroachdb/errors" - "github.com/redis/go-redis/v9" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/sys/unix" @@ -46,10 +45,12 @@ const ( testEngineMaxSizePerMsg = 1 << 20 testEngineMaxInflight = 256 - // leaderChurnRetryTimeout bounds how long doEventually keeps retrying a - // write that fails with a transient leader-unavailable error. It covers - // both startup churn right after createNode returns and mid-test churn - // when the leader briefly steps down under CI load. + // leaderChurnRetryTimeout bounds how long retryNotLeader keeps + // retrying a write that fails with a transient leader-unavailable + // error. Used by tests that issue writes inside long mid-test + // loops where leadership can briefly step down under CI load. + // Startup churn (createNode → first write) is closed at the + // readiness layer by waitForWriteableLeader (PR #898), not here. leaderChurnRetryTimeout = 5 * time.Second // leaderChurnRetryInterval is the poll interval between retries. leaderChurnRetryInterval = 50 * time.Millisecond @@ -750,23 +751,17 @@ func eventuallyExpired(t *testing.T, ttl time.Duration, condition func() bool, m require.Eventually(t, condition, ttlExpiryHeadroom, ttlExpiryPoll, msg) } -// doEventually retries do() while it returns a transient "not leader" error, -// giving the cluster a few seconds to re-settle leadership after startup. -// Non-"not leader" errors fail the test immediately. -// -// MUST be called from the main test goroutine only. The final require.NoError -// calls t.FailNow() on failure; invoking FailNow from a worker goroutine is -// a testing.T contract violation. For parallel-worker use, call -// retryNotLeader directly and report errors back via a channel. -func doEventually(t *testing.T, do func() error) { - t.Helper() - require.NoError(t, retryNotLeader(context.Background(), do)) -} - // retryNotLeader calls do() repeatedly while it returns a transient // leader-unavailable error, capped at leaderChurnRetryTimeout. It returns // the final error (or nil on success) without touching testing.T, making // it safe to call from worker goroutines in parallel tests. +// +// Use cases: mid-test leader churn under CI load (e.g. grpc_test.go's +// 9999-iteration consistency loops). The startup window — leader-churn +// between createNode returning and the first write — is closed at the +// readiness layer instead (waitForWriteableLeader, PR #898), so first- +// write callers should NOT wrap in retryNotLeader; direct calls suffice +// and are clearer. func retryNotLeader(ctx context.Context, do func() error) error { deadline := time.Now().Add(leaderChurnRetryTimeout) var lastErr error @@ -785,21 +780,3 @@ func retryNotLeader(ctx context.Context, do func() error) error { } } } - -// rpushEventually wraps RPUSH in doEventually so transient leader churn -// immediately after createNode doesn't fail the test. -func rpushEventually(t *testing.T, ctx context.Context, rdb *redis.Client, key string, vals ...any) { - t.Helper() - doEventually(t, func() error { - return rdb.RPush(ctx, key, vals...).Err() - }) -} - -// lpushEventually wraps LPUSH in doEventually so transient leader churn -// immediately after createNode doesn't fail the test. -func lpushEventually(t *testing.T, ctx context.Context, rdb *redis.Client, key string, vals ...any) { - t.Helper() - doEventually(t, func() error { - return rdb.LPush(ctx, key, vals...).Err() - }) -}