test: add raft_group_tests.cpp for RaftGroup coverage#28
Conversation
Add 17 unit tests covering: - RaftGroup construction and initial state - Start/stop lifecycle - State machine interface - Log replication (replicate fails when not leader) - RPC serialization (RequestVoteArgs, AppendEntriesArgs) - Log entry structure and defaults - Persistent/volatile state structures - Leader state management - NodeState enum values - Group ID differentiation Part of ongoing coverage improvement effort.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new GoogleTest test suite for RaftGroup functionality with a fixture that sets up a coordinator-mode cluster on port 6200. The suite validates construction state, lifecycle operations, replication behavior before leadership, and properties of Raft data structures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/raft_group_tests.cpp`:
- Around line 49-50: TearDown() currently only removes "raft_group_1.state",
which misses files created by tests like DifferentGroupIds that create group id
2; update TearDown() in tests/raft_group_tests.cpp to remove
"raft_group_2.state" as well (or generically remove all "raft_group_*.state") so
state files from tests such as DifferentGroupIds are cleaned up and do not leak
between runs; locate the TearDown() method and add removal of raft_group_2.state
(or implement a loop/pattern-based removal) to ensure complete cleanup.
- Around line 30-35: The test uses a fixed port TEST_PORT = 6200 and assigns it
to config_.cluster_port then constructs RpcServer(TEST_PORT); change this so the
test picks a free/dynamic port (e.g. bind to port 0 or use a utility that finds
an available port) before creating RpcServer and before setting
config_.cluster_port so both the ClusterManager and RpcServer use the same
dynamic port; ensure rpc_->start() still asserts success and that the chosen
port is propagated to the RaftManager("node1", *cm_, *rpc_) initialization.
- Around line 122-132: The test currently uses EXPECT_TRUE(true) which is
tautological; replace it with real assertions on the AppendEntriesArgs instance
created in TEST_F(RaftGroupTests, AppendEntriesArgsStructure). Verify that
args.leader_id == "leader1", args.term == 1, args.prev_log_index == 5,
args.prev_log_term == 1, args.leader_commit == 3, and that args.entries is empty
(e.g., args.entries.empty() or args.entries.size() == 0). If a
serialization/deserialization helper exists for AppendEntriesArgs, also assert
that serializing and then deserializing the object preserves these fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd46d134-9f85-4847-b3d5-73694ef53e82
📒 Files selected for processing (2)
CMakeLists.txttests/raft_group_tests.cpp
| constexpr uint16_t TEST_PORT = 6200; | ||
| config_.cluster_port = TEST_PORT; | ||
| cm_ = std::make_unique<ClusterManager>(&config_); | ||
| rpc_ = std::make_unique<RpcServer>(TEST_PORT); | ||
| ASSERT_TRUE(rpc_->start()) << "RpcServer failed to start - port may be in use"; | ||
| manager_ = std::make_unique<RaftManager>("node1", *cm_, *rpc_); |
There was a problem hiding this comment.
Avoid fixed test port to reduce CI flakiness.
Using a hardcoded port (6200) can make tests fail nondeterministically when run in parallel or on busy runners (Line 30 / Line 34). Prefer a dynamically allocated/free port strategy in the fixture setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/raft_group_tests.cpp` around lines 30 - 35, The test uses a fixed port
TEST_PORT = 6200 and assigns it to config_.cluster_port then constructs
RpcServer(TEST_PORT); change this so the test picks a free/dynamic port (e.g.
bind to port 0 or use a utility that finds an available port) before creating
RpcServer and before setting config_.cluster_port so both the ClusterManager and
RpcServer use the same dynamic port; ensure rpc_->start() still asserts success
and that the chosen port is propagated to the RaftManager("node1", *cm_, *rpc_)
initialization.
- TearDown: Add cleanup for raft_group_2.state and raft_group_3.state - AppendEntriesArgsStructure: Replace tautological EXPECT_TRUE(true) with real assertions on term, leader_id, prev_log_index, prev_log_term, leader_commit, and entries.empty()
1 participant
Summary
Add 17 unit tests covering
RaftGroupconsensus implementation:Test Categories
replicate()fails when not leader (not yet elected)RequestVoteArgs,AppendEntriesArgsstructuresLogEntry,RaftPersistentState,RaftVolatileState,LeaderStateRequestVoteReply,AppendEntriesReplyNodeStatevaluesTest Results
Part of ongoing coverage improvement effort.
Summary by CodeRabbit