Skip to content

Commit f21a00f

Browse files
committed
raftengine: add missing tests, document Close order, and deduplicate remaining engine wrappers
Address @claude review feedback: - Add durationToTicks unit tests (zero tick, exact, remainder, below min) - Add dest_already_exists validation test for MigrateFSMStore - Document Engine.Close must be called before FactoryResult.Close - Deduplicate remaining 8 instances of hashicorpraftengine.New() in tests
1 parent c275bb1 commit f21a00f

7 files changed

Lines changed: 47 additions & 9 deletions

internal/raftengine/factory.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ type FactoryResult struct {
1818
RegisterTransport func(grpc.ServiceRegistrar)
1919
// Close releases engine-specific resources that are not owned by
2020
// Engine.Close (e.g. raft log stores, transport managers). Callers
21-
// must still call Engine.Close separately.
21+
// must call Engine.Close first to ensure the raft instance is fully
22+
// shut down before the underlying stores and transports are released.
2223
Close func() error
2324
}
2425

internal/raftengine/hashicorp/migrate_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,14 @@ func TestMigrateFSMStoreValidation(t *testing.T) {
6363
require.Error(t, err)
6464
require.Contains(t, err.Error(), "at least one peer is required")
6565
})
66+
67+
t.Run("dest_already_exists", func(t *testing.T) {
68+
dest := t.TempDir() // already exists
69+
peers := []hashicorpraftengine.MigrationPeer{{ID: "n1", Address: "127.0.0.1:7001"}}
70+
_, err := hashicorpraftengine.MigrateFSMStore("/some/path", dest, peers)
71+
require.Error(t, err)
72+
require.Contains(t, err.Error(), "already exists")
73+
})
6674
}
6775

6876
func TestMigrateFSMStoreSeedsHashicorpDataDir(t *testing.T) {

kv/lock_resolver_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,9 @@ func TestLockResolver_LeaderOnlyExecution(t *testing.T) {
209209
r, stop := newSingleRaft(t, "lr-leader", NewKvFSM(st))
210210
defer stop()
211211

212+
e := hashicorpraftengine.New(r)
212213
groups := map[uint64]*ShardGroup{
213-
1: {Engine: hashicorpraftengine.New(r), Store: st, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r))},
214+
1: {Engine: e, Store: st, Txn: NewLeaderProxyWithEngine(e)},
214215
}
215216
ss := NewShardStore(engine, groups)
216217
lr := NewLockResolver(ss, groups, nil)
@@ -239,8 +240,9 @@ func TestLockResolver_CloseStopsBackground(t *testing.T) {
239240
r, stop := newSingleRaft(t, "lr-close", NewKvFSM(st))
240241
defer stop()
241242

243+
e := hashicorpraftengine.New(r)
242244
groups := map[uint64]*ShardGroup{
243-
1: {Engine: hashicorpraftengine.New(r), Store: st, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r))},
245+
1: {Engine: e, Store: st, Txn: NewLeaderProxyWithEngine(e)},
244246
}
245247
ss := NewShardStore(engine, groups)
246248
lr := NewLockResolver(ss, groups, nil)

kv/shard_store_txn_lock_test.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,9 @@ func TestShardStoreGetAt_ReturnsTxnLockedForPendingLock(t *testing.T) {
9797
r1, stop1 := newSingleRaft(t, "g1", NewKvFSM(st1))
9898
defer stop1()
9999

100+
e1 := hashicorpraftengine.New(r1)
100101
groups := map[uint64]*ShardGroup{
101-
1: {Engine: hashicorpraftengine.New(r1), Store: st1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
102+
1: {Engine: e1, Store: st1, Txn: NewLeaderProxyWithEngine(e1)},
102103
}
103104
shardStore := NewShardStore(engine, groups)
104105

@@ -233,8 +234,9 @@ func TestShardStoreScanAt_ReturnsTxnLockedForPendingLock(t *testing.T) {
233234
r1, stop1 := newSingleRaft(t, "g1", NewKvFSM(st1))
234235
defer stop1()
235236

237+
e1 := hashicorpraftengine.New(r1)
236238
groups := map[uint64]*ShardGroup{
237-
1: {Engine: hashicorpraftengine.New(r1), Store: st1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
239+
1: {Engine: e1, Store: st1, Txn: NewLeaderProxyWithEngine(e1)},
238240
}
239241
shardStore := NewShardStore(engine, groups)
240242

@@ -262,8 +264,9 @@ func TestShardStoreScanAt_ReturnsTxnLockedForPendingLockWithoutCommittedValue(t
262264
r1, stop1 := newSingleRaft(t, "g1", NewKvFSM(st1))
263265
defer stop1()
264266

267+
e1 := hashicorpraftengine.New(r1)
265268
groups := map[uint64]*ShardGroup{
266-
1: {Engine: hashicorpraftengine.New(r1), Store: st1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
269+
1: {Engine: e1, Store: st1, Txn: NewLeaderProxyWithEngine(e1)},
267270
}
268271
shardStore := NewShardStore(engine, groups)
269272

@@ -291,8 +294,9 @@ func TestShardStoreScanAt_ReturnsTxnLockedWhenPendingLockExceedsUserLimit(t *tes
291294
r1, stop1 := newSingleRaft(t, "g1", NewKvFSM(st1))
292295
defer stop1()
293296

297+
e1 := hashicorpraftengine.New(r1)
294298
groups := map[uint64]*ShardGroup{
295-
1: {Engine: hashicorpraftengine.New(r1), Store: st1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
299+
1: {Engine: e1, Store: st1, Txn: NewLeaderProxyWithEngine(e1)},
296300
}
297301
shardStore := NewShardStore(engine, groups)
298302

kv/sharded_coordinator_abort_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ func TestShardedAbortRollback_PrepareFailOnShard2_CleansShard1Locks(t *testing.T
4848
s2 := store.NewMVCCStore()
4949
failTxn := &failingTransactional{err: errors.New("simulated shard2 prepare failure")}
5050

51+
e1 := hashicorpraftengine.New(r1)
5152
groups := map[uint64]*ShardGroup{
52-
1: {Engine: hashicorpraftengine.New(r1), Store: s1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
53+
1: {Engine: e1, Store: s1, Txn: NewLeaderProxyWithEngine(e1)},
5354
2: {Store: s2, Txn: failTxn},
5455
}
5556

kv/sharded_coordinator_del_prefix_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,9 @@ func TestShardedCoordinator_DelPrefixPreservesTxnKeys(t *testing.T) {
341341
r1, stop1 := newSingleRaft(t, "dp-txn-g1", NewKvFSM(s1))
342342
t.Cleanup(stop1)
343343

344+
e1 := hashicorpraftengine.New(r1)
344345
groups := map[uint64]*ShardGroup{
345-
1: {Engine: hashicorpraftengine.New(r1), Store: s1, Txn: NewLeaderProxyWithEngine(hashicorpraftengine.New(r1))},
346+
1: {Engine: e1, Store: s1, Txn: NewLeaderProxyWithEngine(e1)},
346347
}
347348
shardStore := NewShardStore(engine, groups)
348349
coord := NewShardedCoordinator(engine, groups, 1, NewHLC(), shardStore)

main_bootstrap_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"testing"
5+
"time"
56

67
"github.com/hashicorp/raft"
78
"github.com/stretchr/testify/require"
@@ -67,3 +68,23 @@ func TestResolveBootstrapServers(t *testing.T) {
6768
require.ErrorIs(t, err, ErrNoBootstrapMembersConfigured)
6869
})
6970
}
71+
72+
func TestDurationToTicks(t *testing.T) {
73+
t.Parallel()
74+
75+
t.Run("zero tick returns min", func(t *testing.T) {
76+
require.Equal(t, 5, durationToTicks(200*time.Millisecond, 0, 5))
77+
})
78+
79+
t.Run("exact division", func(t *testing.T) {
80+
require.Equal(t, 20, durationToTicks(200*time.Millisecond, 10*time.Millisecond, 1))
81+
})
82+
83+
t.Run("remainder rounds up", func(t *testing.T) {
84+
require.Equal(t, 21, durationToTicks(205*time.Millisecond, 10*time.Millisecond, 1))
85+
})
86+
87+
t.Run("below min returns min", func(t *testing.T) {
88+
require.Equal(t, 10, durationToTicks(50*time.Millisecond, 10*time.Millisecond, 10))
89+
})
90+
}

0 commit comments

Comments
 (0)