test(drive): address review comments on identity test coverage#3334
test(drive): address review comments on identity test coverage#3334QuantumExplorer wants to merge 1 commit into
Conversation
- Fix tautological BTreeMap ordering assertion by verifying actual
returned identity data instead of relying on BTreeMap's inherent
sort order (fetch/mod.rs)
- Assert exact nonce value (1) after merge instead of weak nonzero
check, since the value is deterministic for revision=0, nonce=0
(fetch/nonce/mod.rs)
- Verify key re-enablement in masternode re-insertion test by first
disabling keys, re-inserting, then asserting all keys are no longer
disabled (insert/add_new_identity/mod.rs)
- Add edge-case test for identity with both positive balance and
negative credit to verify the combined return value
(fetch/balance/mod.rs)
- Replace .unwrap() with .expect("descriptive message") for optional
balance lookups (fetch/mod.rs)
- Document that estimated-mode balance returns Some(0) as expected
placeholder behavior, not real balance (fetch/balance/mod.rs)
- Match on specific PathParentLayerNotFound error variant instead of
generic is_err() (fetch/balance/mod.rs)
- Add partial-results test for fetch_identities_balances where some
identities exist and others don't (fetch/mod.rs)
- Add root tree byte verification (assert path[0] == Identities) to
all path function tests (mod.rs)
- Cover all 6 IdentityRootStructure variants in TryFrom, u8 array,
and static u8 array conversion tests (mod.rs)
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 (8)
📝 WalkthroughWalkthroughThis PR adds comprehensive unit test coverage across the identity module, including tests for balance operations, identity fetching, nonce management, revision tracking, and insertion logic. All changes are test-only additions with no modifications to production code or public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ 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.
|
|
Closing due to merge conflicts. Will recreate on latest v3.1-dev. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean test-improvement PR that addresses all 10 review comments from #3330. Two confirmed gaps: contract-info path tests omit the distinguishing path[2] assertion, and the ascending-range test doesn't actually verify ordering. Both are minor test-completeness issues, not correctness bugs.
Reviewed commit: 19e8df4
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/identity/mod.rs`:
- [SUGGESTION] lines 586-603: Contract-info path tests never assert path[2] (the IdentityContractInfo segment)
Both `should_create_correct_contract_info_root_path` (line 586) and `should_create_correct_contract_info_group_path` (line 594) skip asserting `path[2]`. The function shows `path[2]` is `IdentityRootStructure::IdentityContractInfo` — the most semantically important segment that distinguishes these paths from other identity paths. Without this assertion, a bug that used the wrong `IdentityRootStructure` variant in position [2] would pass both tests.
In `packages/rs-drive/src/drive/identity/fetch/mod.rs`:
- [SUGGESTION] lines 480-519: should_fetch_balances_by_range_ascending doesn't verify ascending order
The test fetches into a `BTreeMap<[u8; 32], u64>` which inherently sorts by key regardless of the DB ordering. The test asserts all 5 identities are present with correct balances, but never verifies that the `ascending: true` parameter actually produced ascending-ordered results. If the underlying query ignored the ascending flag, this test would still pass. Consider collecting into a `Vec` first and asserting the keys are sorted.
| fn should_create_correct_contract_info_root_path() { | ||
| let id = [5u8; 32]; | ||
| let path = identity_contract_info_root_path(&id); | ||
| assert_eq!(path.len(), 3); | ||
| assert_eq!(path[0], &[RootTree::Identities as u8]); | ||
| assert_eq!(path[1], &id); | ||
| } | ||
|
|
||
| #[test] | ||
| fn should_create_correct_contract_info_group_path() { | ||
| let id = [6u8; 32]; | ||
| let group_id = [7u8; 32]; | ||
| let path = identity_contract_info_group_path(&id, &group_id); | ||
| assert_eq!(path.len(), 4); | ||
| assert_eq!(path[0], &[RootTree::Identities as u8]); | ||
| assert_eq!(path[1], &id); | ||
| assert_eq!(path[3], &group_id); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Contract-info path tests never assert path[2] (the IdentityContractInfo segment)
Both should_create_correct_contract_info_root_path (line 586) and should_create_correct_contract_info_group_path (line 594) skip asserting path[2]. The function shows path[2] is IdentityRootStructure::IdentityContractInfo — the most semantically important segment that distinguishes these paths from other identity paths. Without this assertion, a bug that used the wrong IdentityRootStructure variant in position [2] would pass both tests.
| fn should_create_correct_contract_info_root_path() { | |
| let id = [5u8; 32]; | |
| let path = identity_contract_info_root_path(&id); | |
| assert_eq!(path.len(), 3); | |
| assert_eq!(path[0], &[RootTree::Identities as u8]); | |
| assert_eq!(path[1], &id); | |
| } | |
| #[test] | |
| fn should_create_correct_contract_info_group_path() { | |
| let id = [6u8; 32]; | |
| let group_id = [7u8; 32]; | |
| let path = identity_contract_info_group_path(&id, &group_id); | |
| assert_eq!(path.len(), 4); | |
| assert_eq!(path[0], &[RootTree::Identities as u8]); | |
| assert_eq!(path[1], &id); | |
| assert_eq!(path[3], &group_id); | |
| } | |
| // In should_create_correct_contract_info_root_path (after line 591): | |
| assert_eq!(path[2], Into::<&[u8; 1]>::into(IdentityRootStructure::IdentityContractInfo)); | |
| // In should_create_correct_contract_info_group_path (after line 601): | |
| assert_eq!(path[2], Into::<&[u8; 1]>::into(IdentityRootStructure::IdentityContractInfo)); |
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/identity/mod.rs`:
- [SUGGESTION] lines 586-603: Contract-info path tests never assert path[2] (the IdentityContractInfo segment)
Both `should_create_correct_contract_info_root_path` (line 586) and `should_create_correct_contract_info_group_path` (line 594) skip asserting `path[2]`. The function shows `path[2]` is `IdentityRootStructure::IdentityContractInfo` — the most semantically important segment that distinguishes these paths from other identity paths. Without this assertion, a bug that used the wrong `IdentityRootStructure` variant in position [2] would pass both tests.
| fn should_fetch_balances_by_range_ascending() { | ||
| let drive = setup_drive_with_initial_state_structure(None); | ||
| let platform_version = PlatformVersion::latest(); | ||
|
|
||
| let identities: Vec<Identity> = | ||
| Identity::random_identities(5, 3, Some(42), platform_version) | ||
| .expect("expected random identities"); | ||
|
|
||
| for identity in &identities { | ||
| drive | ||
| .add_new_identity( | ||
| identity.clone(), | ||
| false, | ||
| &BlockInfo::default(), | ||
| true, | ||
| None, | ||
| platform_version, | ||
| ) | ||
| .expect("expected to add identity"); | ||
| } | ||
|
|
||
| let balances: BTreeMap<[u8; 32], u64> = drive | ||
| .fetch_many_identity_balances_by_range::<BTreeMap<[u8; 32], u64>>( | ||
| None, | ||
| true, | ||
| 10, | ||
| None, | ||
| platform_version, | ||
| ) | ||
| .expect("should fetch balances by range"); | ||
|
|
||
| assert_eq!(balances.len(), 5); | ||
|
|
||
| // Verify every inserted identity appears in the result with the correct balance | ||
| for identity in &identities { | ||
| let balance = balances | ||
| .get(&identity.id().to_buffer()) | ||
| .expect("should have balance for inserted identity"); | ||
| assert_eq!(*balance, identity.balance()); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: should_fetch_balances_by_range_ascending doesn't verify ascending order
The test fetches into a BTreeMap<[u8; 32], u64> which inherently sorts by key regardless of the DB ordering. The test asserts all 5 identities are present with correct balances, but never verifies that the ascending: true parameter actually produced ascending-ordered results. If the underlying query ignored the ascending flag, this test would still pass. Consider collecting into a Vec first and asserting the keys are sorted.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/identity/fetch/mod.rs`:
- [SUGGESTION] lines 480-519: should_fetch_balances_by_range_ascending doesn't verify ascending order
The test fetches into a `BTreeMap<[u8; 32], u64>` which inherently sorts by key regardless of the DB ordering. The test asserts all 5 identities are present with correct balances, but never verifies that the `ascending: true` parameter actually produced ascending-ordered results. If the underlying query ignored the ascending flag, this test would still pass. Consider collecting into a `Vec` first and asserting the keys are sorted.
2 participants
Issue being fixed or feature implemented
Addresses review comments from the drive identity test coverage PR (#3330) on
test/drive-identity-coverage.What was done?
Fixed all 10 review comments:
HIGH: BTreeMap ordering assertion is tautological — Replaced the tautological
windows(2).all(|w| w[0] <= w[1])assertion on BTreeMap keys (which is always sorted) with verification that every inserted identity appears in the result with the correct balance.MEDIUM: Weak nonce assertion — Replaced
assert!(nonce > 0)withassert_eq!(nonce, 1)since the exact value is deterministic when merging nonce=1 onto an identity with initial nonce=0.MEDIUM: Masternode re-insertion test doesn't verify key re-enablement — Rewrote the test to first disable all keys, then re-insert the masternode identity, then verify all keys have
is_disabled() == false.MEDIUM: Missing edge case — combined positive balance + negative credit — Added test that sets both a positive balance and negative credit on the same identity, verifying that
fetch_identity_balance_include_debtreturns the positive balance (negative credit is not consulted when balance > 0).LOW:
.unwrap()without context — Replaced bare.unwrap()calls with.expect("descriptive message").LOW: Estimated-mode test asserts hardcoded placeholder — Enhanced the comment to clearly document that
Some(0)is expected placeholder behavior in stateless/estimated mode, not a real balance. Added descriptive assertion message.LOW:
assert!(result.is_err())without checking error type — Replaced genericis_err()with amatches!assertion onError::GroveDB(e) if matches!(e.as_ref(), grovedb::Error::PathParentLayerNotFound(_)).LOW: Missing test for partial results — Added
fetch_identities_balances_partialtest module where one identity exists and one doesn't, verifying only the existing one appears in results.NIT: Path tests skip root tree byte verification — Added
assert_eq!(path[0], &[RootTree::Identities as u8])(or the vec equivalent) to all path function tests.NIT: Incomplete enum conversion coverage — Extended
should_convert_to_u8,should_convert_to_u8_array, andshould_convert_to_static_u8_array_reftests to cover all 6IdentityRootStructurevariants.How Has This Been Tested?
Breaking Changes
None.
Checklist:
Summary by CodeRabbit