test(drive): improve identity module test coverage#3330
Conversation
Add 48 new tests covering previously untested areas of the drive identity module including partial identity fetches, balance operations, nonce and revision fetching, identity insertion error paths, and IdentityRootStructure enum conversions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds comprehensive server-only unit tests for identity operations in rs-drive: balance update, balance fetches, nonce, partial identity, revision, insertions, and identity root utilities. No production logic or public API signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3330 +/- ##
============================================
+ Coverage 73.22% 73.48% +0.25%
============================================
Files 3291 3295 +4
Lines 271882 273133 +1251
============================================
+ Hits 199084 200703 +1619
+ Misses 72798 72430 -368
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/rs-drive/src/drive/identity/fetch/mod.rs (1)
431-463: Validate ordering in the ascending-range test.The test currently checks only
len(). Since this is specifically the ascending path, assert returned keys are sorted ascending to actually validate behavior.💡 Suggested assertion addition
assert_eq!(balances.len(), 5); + let keys: Vec<[u8; 32]> = balances.keys().copied().collect(); + assert!(keys.windows(2).all(|w| w[0] <= w[1]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/identity/fetch/mod.rs` around lines 431 - 463, The test should_fetch_balances_by_range_ascending only asserts balances.len(), but needs to validate the ascending order of returned keys; update the test to extract the returned keys from the BTreeMap returned by fetch_many_identity_balances_by_range (variable balances) and assert they are in ascending order relative to the original identities (or that each key is >= previous key). Locate the test function should_fetch_balances_by_range_ascending and after the existing len() assertion add an assertion that iterates the keys (or collects them into a Vec) and checks monotonic non-decreasing order to confirm the ascending-range behavior of fetch_many_identity_balances_by_range.packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs (1)
300-311: Strengthen masternode reinsertion test with a state assertion.
assert!(result.is_ok())confirms no error, but it doesn’t validate the intended side effect (“re-enable keys”). Add a postcondition check on key state after reinsertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs` around lines 300 - 311, After calling drive.add_new_identity(...) and asserting result.is_ok(), fetch the stored identity from the drive (e.g., via a getter used elsewhere in tests such as a get_identity or fetch_identity method) using the same identity id and transaction/context, then assert the masternode's keys are enabled (check the specific field that indicates key/state re-enabled on the returned identity object). This adds a postcondition verifying the intended side effect of re-enabling keys after reinsertion.packages/rs-drive/src/drive/identity/fetch/balance/mod.rs (1)
91-96: Make estimated-mode balance assertion explicit.Line 95 (
assert!(balance.is_some())) is too loose for a behavior test. Assert the exact expected value for estimated mode to avoid false positives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/identity/fetch/balance/mod.rs` around lines 91 - 96, The test currently uses a loose assertion assert!(balance.is_some()) for the estimated mode; update the assertion to explicitly expect the estimated-mode value (e.g., assert_eq!(balance, Some(0))) so the behavior is precisely validated. Locate the test in mod.rs that examines estimated vs apply modes (the block that already checks fee_result.processing_fee > 0 and uses the local variable balance) and replace the generic is_some() check with an exact equality check for the estimated-mode case, or guard it with the mode flag used in the test so the apply-mode still allows actual balances.packages/rs-drive/src/drive/identity/balance/update.rs (1)
976-981: Assert insufficient-balance error payload, not just variant.This currently validates only the enum branch. Consider asserting
identity_id,balance, andrequired_balancetoo so regressions in error construction are caught.💡 Suggested assertion hardening
- assert!(matches!( - result, - Err(Error::Identity(IdentityError::IdentityInsufficientBalance( - _ - ))) - )); + match result { + Err(Error::Identity(IdentityError::IdentityInsufficientBalance(err))) => { + assert_eq!(err.balance(), added_balance); + assert_eq!(err.required_balance(), added_balance + 1); + assert_eq!(err.identity_id().to_buffer(), identity.id().to_buffer()); + } + other => panic!("unexpected result: {:?}", other), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/identity/balance/update.rs` around lines 976 - 981, The test currently only checks the enum branch using assert!(matches!(result, Err(Error::Identity(IdentityError::IdentityInsufficientBalance(_))))); — instead, destructure the error payload from result to capture the IdentityInsufficientBalance fields and assert their values (e.g., capture identity_id, balance, required_balance) against the expected values; locate the pattern matching on Error::Identity / IdentityError::IdentityInsufficientBalance and replace the wildcard (_) with a pattern like IdentityInsufficientBalance { identity_id: actual_id, balance: actual_bal, required_balance: actual_req } then assert_eq! on actual_id, actual_bal, and actual_req to the expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-drive/src/drive/identity/mod.rs`:
- Around line 421-422: The test module is only gated with #[cfg(test)] but calls
feature-gated functions (e.g., identity_contract_info_root_path and other
functions requiring #[cfg(any(feature = "server", feature = "verify"))]); update
the module gating so tests compile under minimal feature sets: change the outer
test module to #[cfg(all(test, any(feature = "server", feature = "verify")))]
(or #[cfg(test)] plus a nested #[cfg(any(feature = "server", feature =
"verify"))] if you prefer), and move the server-only test
should_create_correct_contract_info_root_path into its own nested #[cfg(feature
= "server")] module so verify-only builds still run the other tests.
---
Nitpick comments:
In `@packages/rs-drive/src/drive/identity/balance/update.rs`:
- Around line 976-981: The test currently only checks the enum branch using
assert!(matches!(result,
Err(Error::Identity(IdentityError::IdentityInsufficientBalance(_))))); —
instead, destructure the error payload from result to capture the
IdentityInsufficientBalance fields and assert their values (e.g., capture
identity_id, balance, required_balance) against the expected values; locate the
pattern matching on Error::Identity / IdentityError::IdentityInsufficientBalance
and replace the wildcard (_) with a pattern like IdentityInsufficientBalance {
identity_id: actual_id, balance: actual_bal, required_balance: actual_req } then
assert_eq! on actual_id, actual_bal, and actual_req to the expected values.
In `@packages/rs-drive/src/drive/identity/fetch/balance/mod.rs`:
- Around line 91-96: The test currently uses a loose assertion
assert!(balance.is_some()) for the estimated mode; update the assertion to
explicitly expect the estimated-mode value (e.g., assert_eq!(balance, Some(0)))
so the behavior is precisely validated. Locate the test in mod.rs that examines
estimated vs apply modes (the block that already checks
fee_result.processing_fee > 0 and uses the local variable balance) and replace
the generic is_some() check with an exact equality check for the estimated-mode
case, or guard it with the mode flag used in the test so the apply-mode still
allows actual balances.
In `@packages/rs-drive/src/drive/identity/fetch/mod.rs`:
- Around line 431-463: The test should_fetch_balances_by_range_ascending only
asserts balances.len(), but needs to validate the ascending order of returned
keys; update the test to extract the returned keys from the BTreeMap returned by
fetch_many_identity_balances_by_range (variable balances) and assert they are in
ascending order relative to the original identities (or that each key is >=
previous key). Locate the test function should_fetch_balances_by_range_ascending
and after the existing len() assertion add an assertion that iterates the keys
(or collects them into a Vec) and checks monotonic non-decreasing order to
confirm the ascending-range behavior of fetch_many_identity_balances_by_range.
In `@packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs`:
- Around line 300-311: After calling drive.add_new_identity(...) and asserting
result.is_ok(), fetch the stored identity from the drive (e.g., via a getter
used elsewhere in tests such as a get_identity or fetch_identity method) using
the same identity id and transaction/context, then assert the masternode's keys
are enabled (check the specific field that indicates key/state re-enabled on the
returned identity object). This adds a postcondition verifying the intended side
effect of re-enabling keys after reinsertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff9e5ea-427e-4ca9-bef8-06d15f95483f
📒 Files selected for processing (8)
packages/rs-drive/src/drive/identity/balance/update.rspackages/rs-drive/src/drive/identity/fetch/balance/mod.rspackages/rs-drive/src/drive/identity/fetch/mod.rspackages/rs-drive/src/drive/identity/fetch/nonce/mod.rspackages/rs-drive/src/drive/identity/fetch/partial_identity/mod.rspackages/rs-drive/src/drive/identity/fetch/revision/mod.rspackages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rspackages/rs-drive/src/drive/identity/mod.rs
- Gate test module with feature flags to prevent compilation errors in minimal feature builds - Add ascending order assertion to range balance fetch test - Fix estimated mode balance assertion to expect exact stateless value - Add key verification to masternode reinsertion test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Audit Review
Good coverage additions across identity fetch, balance, nonce, revision, partial identity, insertion, and enum operations. Tests are well-organized into submodules, consistently named with should_ prefix, and properly feature-gated. Found one tautological assertion and several verification gaps. Details in inline comments below.
Good practices observed
- Tests exercise real Drive instances, not mocks
- Consistent
should_naming convention - Proper
#[cfg(feature = "server")]gating - Good coverage of non-existent identity paths
- Error-path testing for duplicate insertion and insufficient balance
|
|
||
| // BTreeMap keys are always sorted, confirming ascending order | ||
| let keys: Vec<[u8; 32]> = balances.keys().copied().collect(); | ||
| assert!(keys.windows(2).all(|w| w[0] <= w[1])); |
There was a problem hiding this comment.
High: BTreeMap ordering assertion is tautological
BTreeMap always stores keys in sorted order regardless of query ordering. This assertion will always pass, even if the underlying query returned results in descending or random order. It tests a property of the Rust standard library, not of the production code.
To actually test query ordering, collect into a Vec:
let balances: Vec<([u8; 32], u64)> = drive
.fetch_many_identity_balances_by_range::<Vec<([u8; 32], u64)>>(
None, true, 10, None, platform_version,
)
.expect("should fetch balances by range");
assert!(balances.windows(2).all(|w| w[0].0 <= w[1].0));Also worth adding a descending=false test to verify the ascending parameter actually affects results.
|
|
||
| // After merging nonce 1, the nonce value should contain 1 | ||
| // The nonce encoding includes missing revision bits, so just check it's nonzero | ||
| assert!(nonce > 0); |
There was a problem hiding this comment.
Medium: Weak nonce assertion — exact value is knowable
The comment says "The nonce encoding includes missing revision bits, so just check it's nonzero," but for this specific case (merging nonce 1 into a fresh identity with nonce 0), the missing revision bits are all zero. The stored value should be exactly 1.
Replace with:
assert_eq!(nonce, 1, "nonce after merging 1 into fresh identity should be 1");As written, any non-zero value (including a corrupted nonce) passes the test.
| platform_version, | ||
| ); | ||
|
|
||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
Medium: Masternode re-insertion test doesn't verify key re-enablement
The critical production behavior for masternode re-insertion (in add_new_identity_operations_v0) is key re-enablement: it fetches existing keys, matches them, and calls re_enable_identity_keys_operations. This test only checks result.is_ok() and fetched_keys.len() == 5.
To properly test re-enablement:
- Insert the masternode identity
- Disable at least one key (set
disabled_at) - Re-insert the same masternode identity
- Verify the previously-disabled key now has
disabled_at == None
Without this, a regression in re_enable_identity_keys_operations that silently fails would go undetected.
| .expect("should not error") | ||
| .expect("should have balance"); | ||
|
|
||
| assert_eq!(balance, -(negative_amount as i64)); |
There was a problem hiding this comment.
Medium: Missing edge case — combined positive balance + negative credit
This test covers the credits == 0 + negative credit > 0 case. But the production code in fetch_identity_balance_include_debt_operations_v0 has a branch: if credits > 0, it returns the positive balance and NEVER checks the negative credit field. Missing edge cases:
- Both balance > 0 AND negative credit > 0 — which takes precedence? (data corruption scenario)
- Both balance == 0 AND negative credit == 0 — should return
Some(0)
For a financial system, testing what happens when both fields are set is important even if it "shouldn't happen" — it documents the behavior under corruption.
|
|
||
| assert_eq!(balances.len(), 2); | ||
| assert_eq!( | ||
| *balances.get(&identity.id().to_buffer()).unwrap(), |
There was a problem hiding this comment.
Low: .unwrap() without context
These two .unwrap() calls (lines 419 and 422) should use .expect() with descriptive messages for consistency with the rest of the PR:
.expect("balance entry should exist for inserted identity")
.expect("balance entry should exist for non-existent identity (as None)")| // from the database. It returns Some(0) as a placeholder value | ||
| // while computing estimated costs. | ||
| assert!(fee_result.processing_fee > 0); | ||
| assert_eq!(balance, Some(0)); |
There was a problem hiding this comment.
Low: Estimated-mode test asserts a hardcoded placeholder
In apply=false mode, the production code returns Some(0) without querying the database (it's a StatelessDirectQuery). This test goes through the overhead of creating and inserting an identity that is never read. The only meaningful assertion here is processing_fee > 0 (fee calculation wiring).
Consider adding a comment that this test validates fee calculation in estimation mode, not actual balance fetching. The inserted identity is irrelevant to the test outcome.
| platform_version, | ||
| ); | ||
|
|
||
| assert!(result.is_err()); |
There was a problem hiding this comment.
Low: assert!(result.is_err()) without checking error type
The comment explains the error is PathParentLayerNotFound, but the assertion accepts any error. Consider matching on the specific error variant:
assert!(matches!(
result,
Err(Error::GroveDB(_))
), "expected GroveDB error for non-existent identity path");| use super::*; | ||
|
|
||
| #[test] | ||
| fn should_fetch_balances_for_existing_identities() { |
There was a problem hiding this comment.
Low: Missing test for partial results
fetch_identities_balances only has a happy-path test where all identities exist. Unlike fetch_optional_identities_balances (which explicitly returns None for missing IDs), this function simply omits missing identities from the result map. A test showing that requesting a mix of existing and non-existing IDs returns only the existing ones would document this important behavioral difference.
| use super::*; | ||
|
|
||
| #[test] | ||
| fn should_create_correct_identity_path() { |
There was a problem hiding this comment.
Nit: Path tests skip root tree byte verification
These path function tests check path.len() and verify the identity ID is at the correct index, but don't verify path[0] equals &[RootTree::Identities as u8]. If someone changed the root tree byte, these tests would still pass. Consider:
assert_eq!(path[0], &[RootTree::Identities as u8]);Same applies to the key tree, contract info, and group path tests — the structure-specific bytes at their expected positions aren't verified.
| } | ||
|
|
||
| #[test] | ||
| fn should_convert_to_u8() { |
There was a problem hiding this comment.
Nit: Incomplete enum conversion coverage
should_convert_to_u8 only tests 3 of 6 IdentityRootStructure variants. should_convert_to_u8_array only tests 2 of 6. should_convert_to_static_u8_array_ref only tests 2 of 6. If these are meant to validate the From implementations, they should test all variants for completeness, since a missing match arm on a new variant wouldn't be caught.
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>
1 participant
Issue being fixed or feature implemented
Improve test coverage for the
packages/rs-drive/src/drive/identity/module, adding tests for previously untested code paths.What was done?
Added 48 new tests across 8 files covering previously untested areas of the drive identity module:
Fetch operations (balance, nonce, revision):
fetch_identity_balance- existing identity, non-existent identity, estimated costsfetch_identity_balance_include_debt- positive balance, negative balance (debt), non-existentfetch_identity_negative_balance_operations- non-existent identity error path, zero balance for new identityfetch_identity_nonce- non-existent identity, initial nonce, updated nonce after merge, nonce with feesfetch_identity_revision- non-existent identity, initial revision, updated revision, revision with feesPartial identity fetches:
fetch_identity_with_balance- non-existent identity, existing identity, cost estimationfetch_identity_balance_with_keys- non-existent, balance with all keys, specific keys, not-found key trackingfetch_identity_balance_with_keys_and_revision- non-existent, full partial identityfetch_identity_revision_with_keys- non-existent, revision with keysfetch_identity_keys_as_partial_identity- keys-only partial identityBatch identity operations:
verify_all_identities_exist- all exist, some missingfetch_identities_balances- multiple identity balancesfetch_optional_identities_balances- existing and non-existent identitiesfetch_many_identity_balances_by_range- ascending range, limit enforcementInsert error paths:
Balance update error paths:
remove_from_identity_balance- insufficient balance error, non-existent identity errorIdentityRootStructure enum:
How Has This Been Tested?
All 48 new tests pass:
All existing tests continue to pass with no regressions.
Breaking Changes
None. This PR only adds tests.
Checklist:
Summary by CodeRabbit