test(drive): address review comments on identity test coverage#3336
Conversation
Fixes from code review on PR #3330: - Replace tautological BTreeMap ordering assertion with actual data verification - Assert exact nonce value (1) instead of just nonzero after merge - Verify keys are re-enabled after masternode re-insertion (disable first, then re-insert) - Add edge case test for identity with both positive balance and negative credit - Replace .unwrap() with .expect() with descriptive messages - Document that Some(0) is expected placeholder in estimated (apply=false) mode - Match specific GroveDB error variant instead of generic is_err() - Add partial results test for fetch_identities_balances (some exist, some don't) - Assert path[0] == RootTree::Identities in all path function tests - Test all 6 IdentityRootStructure variants in conversion tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ 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 (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3336 +/- ##
============================================
+ Coverage 73.48% 73.60% +0.12%
============================================
Files 3295 3295
Lines 273133 274156 +1023
============================================
+ Hits 200703 201793 +1090
+ Misses 72430 72363 -67
🚀 New features to boost your workflow:
|
Issue being fixed or feature implemented
Addresses code review comments from PR #3330 (identity test coverage).
What was done?
HIGH: BTreeMap ordering assertion is tautological (
fetch/mod.rs) -- Replaced with assertions that verify each identity's actual balance is correct in the returned map.MEDIUM: Weak nonce assertion (
fetch/nonce/mod.rs) -- Changedassert!(nonce > 0)toassert_eq!(nonce, 1)with documentation explaining why the value is exactly 1 after merging nonce 1 from initial state.MEDIUM: Masternode re-insertion test doesn't verify key re-enablement (
insert/add_new_identity/mod.rs) -- Now disables all keys before re-insertion, verifies they are disabled, then verifies they are re-enabled after masternode re-insertion.MEDIUM: Missing edge case -- combined positive balance + negative credit (
fetch/balance/mod.rs) -- Added test that sets both positive balance and negative credit, documenting thatfetch_identity_balance_include_debtreturns just the positive balance when it is nonzero.LOW:
.unwrap()without context (fetch/mod.rs) -- Replaced with.expect("descriptive message").LOW: Estimated-mode test asserts hardcoded placeholder (
fetch/balance/mod.rs) -- Already documented; the comment explainsSome(0)is the expected placeholder inapply=falsemode.LOW:
assert!(result.is_err())without checking error type (fetch/balance/mod.rs) -- Now usesmatches!(result, Err(Error::GroveDB(_)))with a descriptive failure message.LOW: Missing test for partial results (
fetch/mod.rs) -- Added test where one identity exists and another does not, verifyingfetch_identities_balancesreturns only the existing one.NIT: Path tests skip root tree byte verification (
mod.rs) -- All path function tests now assertpath[0] == RootTree::Identities.NIT: Incomplete enum conversion coverage (
mod.rs) -- All three conversion tests (to_u8,to_u8_array,to_static_u8_array_ref) now cover all 6IdentityRootStructurevariants.How Has This Been Tested?
Breaking Changes
None.
Checklist: