Repository navigation
refactor(core) #357, #358: remove dead leader code and add missing test coverage - #359
Conversation
|
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 5 minutes and 50 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 (4)
📝 WalkthroughWalkthroughThis PR removes two unused methods ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (2)
d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs (1)
363-413: Strengthen these metrics tests with behavioral assertions.These tests currently only verify “no panic,” so they can still pass if enabled/sampling logic regresses. Please assert observable outcomes (e.g., recorded counter/gauge events and sampling cadence) so the tests actually fail on logic breaks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs` around lines 363 - 413, Tests only check for absence of panics but not behavior; update the BackpressureMetrics tests to assert actual metric changes by reading the observable state after calls: for BackpressureMetrics::record_rejection call record_rejection(true/false) and assert the corresponding rejection counter/gauge has incremented (or remains unchanged when enabled=false); for record_buffer_utilization call record_buffer_utilization(...) and assert the buffer utilization gauge was updated with the expected value and label (write vs read); for sampling-rate tests (BackpressureMetrics::new(..., sample_rate)) call record_buffer_utilization repeatedly and assert only the expected fraction of samples produced metric updates (e.g., check an internal sample counter or exported metric count increments every N calls). Use existing accessors or add small test-only getters on BackpressureMetrics (e.g., get_rejection_count(label), get_last_buffer_utilization(label), get_sample_counter()) to make these assertions deterministic.d-engine-core/src/raft_role/leader_state_test/membership_change_test.rs (1)
1906-1909: Optionally tightenget_node_statuscall expectations for stronger regression signals.For the two-candidate case, consider asserting exact invocation count to ensure per-candidate status checks remain intact.
Optional test hardening
- membership.expect_get_node_status().returning(|_| Some(NodeStatus::Promotable)); + membership + .expect_get_node_status() + .times(2) + .returning(|_| Some(NodeStatus::Promotable));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@d-engine-core/src/raft_role/leader_state_test/membership_change_test.rs` around lines 1906 - 1909, The test currently stubs membership.expect_get_node_status() with a generic returning closure; tighten it to assert it is called once per candidate by setting an exact call count (e.g., times(2)) and keep returning Some(NodeStatus::Promotable) so the two zombie candidates from membership.expect_get_zombie_candidates() are each checked; update the mock setup for get_node_status to use the expect_* call-count assertion (referencing membership.expect_get_node_status and NodeStatus::Promotable) to catch regressions where per-candidate status checks might be skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs`:
- Around line 363-413: Tests only check for absence of panics but not behavior;
update the BackpressureMetrics tests to assert actual metric changes by reading
the observable state after calls: for BackpressureMetrics::record_rejection call
record_rejection(true/false) and assert the corresponding rejection
counter/gauge has incremented (or remains unchanged when enabled=false); for
record_buffer_utilization call record_buffer_utilization(...) and assert the
buffer utilization gauge was updated with the expected value and label (write vs
read); for sampling-rate tests (BackpressureMetrics::new(..., sample_rate)) call
record_buffer_utilization repeatedly and assert only the expected fraction of
samples produced metric updates (e.g., check an internal sample counter or
exported metric count increments every N calls). Use existing accessors or add
small test-only getters on BackpressureMetrics (e.g.,
get_rejection_count(label), get_last_buffer_utilization(label),
get_sample_counter()) to make these assertions deterministic.
In `@d-engine-core/src/raft_role/leader_state_test/membership_change_test.rs`:
- Around line 1906-1909: The test currently stubs
membership.expect_get_node_status() with a generic returning closure; tighten it
to assert it is called once per candidate by setting an exact call count (e.g.,
times(2)) and keep returning Some(NodeStatus::Promotable) so the two zombie
candidates from membership.expect_get_zombie_candidates() are each checked;
update the mock setup for get_node_status to use the expect_* call-count
assertion (referencing membership.expect_get_node_status and
NodeStatus::Promotable) to catch regressions where per-candidate status checks
might be skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c35a988b-5b69-496b-8cd8-e4cd923f1d33
📒 Files selected for processing (4)
d-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/backpressure_test.rsd-engine-core/src/raft_role/leader_state_test/event_handling_test.rsd-engine-core/src/raft_role/leader_state_test/membership_change_test.rs
…st coverage #358: Delete batch_promote_learners (superseded by process_pending_promotions), if_update_commit_index (no callers), and their associated tests. Remove now-unused ensure_safe_join import. #357: Add tests for StepDownSelfRemoved event handling, conditionally_purge_zombie_nodes BatchRemove path (no_candidates / submits_batch_remove / skips_active_nodes), and BackpressureMetrics record_rejection/record_buffer_utilization branches.
2e40d92 to
0723aa7
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
What Does This PR Do?
Removes two dead functions (
batch_promote_learners,if_update_commit_index) fromLeaderStateand adds tests for four previously uncovered code paths.Type:
Why Is This Needed?
#358:
batch_promote_learnerswas superseded byprocess_pending_promotionsbut left behind with#[allow(dead_code)].if_update_commit_indexhad no callers. Both add maintenance burden and the dead_code suppression masks future regressions.#357: Four code paths in
leader_state.rshad no test coverage, causing Codecov patch failures in adjacent PRs:RaftEvent::StepDownSelfRemovedhandlerconditionally_purge_zombie_nodesBatchRemove pathBackpressureMetrics::record_rejection/record_buffer_utilization(enabled branch)Checklist
Required:
make testpassesTesting
How tested:
test_step_down_self_removed_sends_become_followerverifies BecomeFollower event on self-removal;zombie_purge_tests(3 cases: no candidates, non-Active nodes removed, Active nodes skipped); 5BackpressureMetricstests covering enabled/disabled × write/read × sampling rateleader_state_testtests passDoes This Follow d-engine's Principles?
Reviewer Notes
conditionally_purge_zombie_nodesvisibility changed from private topub(super)to enable direct unit testing — no public API change.Estimated review complexity:
Summary by CodeRabbit
Refactor
Tests