test(drive): improve coverage for vote paths, document info, token ops, and asset lock proofs#3452
Merged
Merged
Conversation
…ops, and instant asset lock proof Add 101 unit tests across four previously untested files: - drive::votes::paths (37 tests): path construction, vec/slice consistency, constant values - util::object_size_info::document_info (32 tests): variant methods, accessors, size info - util::batch::drive_op_batch::token (11 tests): operation construction, clone, debug - dpp::identity::state_transition::asset_lock_proof::instant (21 tests): construction, accessors, round-trip serialization, equality Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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 6 minutes and 44 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)
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage to four files across rs-dpp and rs-drive packages with no production code changes, introducing approximately 1,320 lines of unit tests for InstantAssetLockProof, vote-tree paths, TokenOperationType, and DocumentInfo variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/rs-dpp/src/identity/state_transition/asset_lock_proof/instant/instant_asset_lock_proof.rs (1)
277-284:test_new_stores_fields_correctlycurrently does not validatenew()field assignment.At Line 279 the test uses a fixture and only checks
output_indexplus accessor calls; it never proves constructor inputs are preserved. Please construct viaInstantAssetLockProof::new(...)and assert equality on all three fields.Proposed test tightening
#[test] fn test_new_stores_fields_correctly() { - let proof = raw_instant_asset_lock_proof_fixture(None, None); - assert_eq!(proof.output_index(), 0); - // Verify the instant lock and transaction are accessible - let _il = proof.instant_lock(); - let _tx = proof.transaction(); + let base = raw_instant_asset_lock_proof_fixture(None, None); + let instant_lock = base.instant_lock.clone(); + let transaction = base.transaction.clone(); + let output_index = 5; + + let proof = InstantAssetLockProof::new( + instant_lock.clone(), + transaction.clone(), + output_index, + ); + + assert_eq!(proof.instant_lock(), &instant_lock); + assert_eq!(proof.transaction(), &transaction); + assert_eq!(proof.output_index(), output_index); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/identity/state_transition/asset_lock_proof/instant/instant_asset_lock_proof.rs` around lines 277 - 284, The test test_new_stores_fields_correctly should construct an InstantAssetLockProof using InstantAssetLockProof::new(...) instead of using the raw fixture return value directly; obtain the expected output_index, instant_lock, and transaction from raw_instant_asset_lock_proof_fixture(None, None) (or from dedicated fixtures), call InstantAssetLockProof::new(expected_output_index, expected_instant_lock.clone(), expected_transaction.clone()), then assert that proof.output_index() == expected_output_index, proof.instant_lock() == &expected_instant_lock, and proof.transaction() == &expected_transaction to verify the constructor stored all fields correctly.packages/rs-drive/src/drive/votes/paths.rs (1)
572-579: Consider extracting a shared helper for vec/slice parity assertions.The repeated
for (vec_elem, slice_elem)comparison blocks are consistent but duplicated; a small local helper would reduce maintenance overhead.Also applies to: 595-601, 617-623, 640-646, 669-672, 721-724, 764-767, 794-797, 872-875, 899-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/votes/paths.rs` around lines 572 - 579, Extract a small test helper (e.g., assert_path_vec_slice_parity or assert_vec_matches_slice) that accepts a Vec of path elements and a slice (&[&[u8]] or whatever types used) and asserts equal length and that each vec_elem.as_slice() == *slice_elem; then replace the repeated for-loop blocks in functions like test_vote_decisions_tree_path_vec_matches_slice_form, the tests around lines 595-601, 617-623, 640-646, 669-672, 721-724, 764-767, 794-797, 872-875, and 899-902 to call this helper (pass vote_decisions_tree_path_vec()/vote_decisions_tree_path() and the analogous path_vec/path pairs in other tests), removing duplicated loop logic. Ensure the helper name is descriptive and used consistently across those test functions.packages/rs-drive/src/util/object_size_info/document_info.rs (2)
503-527: Consider renaming this test to reflect what it actually verifies.This test doesn't call
get_estimated_size_for_document_type; it only verifies the constant values. The current nametest_estimated_size_for_owner_idis misleading. Consider renaming to something liketest_system_field_size_constants.Also, the
drop(info)at line 526 is unnecessary—Rust automatically drops values at the end of their scope.♻️ Suggested rename and cleanup
#[test] - fn test_estimated_size_for_owner_id() { - let info = DocumentInfo::DocumentEstimatedAverageSize(100); - // We cannot build a real DocumentTypeRef without a full contract, - // but for system fields the document type is not consulted. + fn test_system_field_size_constants() { + // DocumentTypeRef requires a full DataContract, so we verify the + // constant values that get_estimated_size_for_document_type returns + // for system fields like $ownerId, $createdAt, etc. ... - drop(info); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/object_size_info/document_info.rs` around lines 503 - 527, Rename the test function test_estimated_size_for_owner_id to a name that reflects it only verifies constant sizes (e.g., test_system_field_size_constants) and remove the unnecessary drop(info) call; update the test declaration (the function name) that references DocumentInfo and assert constants DEFAULT_HASH_SIZE_U16, U64_SIZE_U16, and U32_SIZE_U16 so the test name matches its actual purpose.
584-600: Consider adding a test forget_storage_flags_refwithDocumentEstimatedAverageSize.The implementation handles
DocumentEstimatedAverageSizedifferently—it returnsStorageFlags::optional_default_as_ref()(line 278-279)—but this behavior is not covered by any test. Adding a test would ensure this edge case remains consistent.🧪 Suggested additional test
#[test] fn test_get_storage_flags_ref_none_for_owned_without_flags() { let doc = make_document([41; 32]); let info = DocumentInfo::DocumentOwnedInfo((doc, None)); assert!(info.get_storage_flags_ref().is_none()); } + + #[test] + fn test_get_storage_flags_ref_for_estimated_returns_optional_default() { + let info = DocumentInfo::DocumentEstimatedAverageSize(100); + // Implementation returns StorageFlags::optional_default_as_ref() + let flags = info.get_storage_flags_ref(); + assert_eq!(flags, StorageFlags::optional_default_as_ref()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/object_size_info/document_info.rs` around lines 584 - 600, Add a unit test that covers the DocumentEstimatedAverageSize variant for get_storage_flags_ref: construct a DocumentInfo::DocumentEstimatedAverageSize instance (e.g. with an estimated size value), call info.get_storage_flags_ref(), and assert it returns StorageFlags::optional_default_as_ref(); reference get_storage_flags_ref, DocumentEstimatedAverageSize, and StorageFlags::optional_default_as_ref() so the test verifies this specific branch.packages/rs-drive/src/util/batch/drive_op_batch/token.rs (1)
575-605: Consider testing Clone and Debug for additional variants.The
CloneandDebugtests currently only validate theTokenBurnvariant. While the derived implementations should work consistently, testing a couple more variants (especially those with different field types likeTokenMintManywith aVecorTokenSetPriceForDirectPurchasewith anOption) would provide additional confidence.♻️ Optional: expand Clone and Debug test coverage
#[test] fn test_token_operation_clone() { let op = TokenOperationType::TokenBurn { token_id: test_token_id(), identity_balance_holder_id: test_identity_id(), burn_amount: 100, }; let cloned = op.clone(); match cloned { TokenOperationType::TokenBurn { burn_amount, .. } => { assert_eq!(burn_amount, 100); } _ => panic!("clone should preserve variant"), } + + // Test cloning a variant with Vec + let op2 = TokenOperationType::TokenMintMany { + token_id: test_token_id(), + recipients: vec![(test_recipient_id(), 100)], + mint_amount: 500, + allow_first_mint: true, + }; + let cloned2 = op2.clone(); + match cloned2 { + TokenOperationType::TokenMintMany { recipients, .. } => { + assert_eq!(recipients.len(), 1); + } + _ => panic!("clone should preserve variant"), + } } #[test] fn test_token_operation_debug() { let op = TokenOperationType::TokenBurn { token_id: test_token_id(), identity_balance_holder_id: test_identity_id(), burn_amount: 42, }; let debug_str = format!("{:?}", op); assert!(debug_str.contains("TokenBurn")); assert!(debug_str.contains("42")); + + // Test Debug for variant with Option + let op2 = TokenOperationType::TokenSetPriceForDirectPurchase { + token_id: test_token_id(), + price: None, + }; + let debug_str2 = format!("{:?}", op2); + assert!(debug_str2.contains("TokenSetPriceForDirectPurchase")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/batch/drive_op_batch/token.rs` around lines 575 - 605, Add additional unit tests exercising Clone and Debug for other TokenOperationType variants (e.g., TokenMintMany and TokenSetPriceForDirectPurchase). For Clone: create instances of TokenMintMany (with a non-empty Vec) and TokenSetPriceForDirectPurchase (with Some(price)), call .clone(), and assert cloned fields (Vec length/contents and Option value) match originals; name tests like test_token_operation_clone_mint_many and test_token_operation_clone_set_price. For Debug: format!("{:?}", instance) for the same variants and assert the debug string contains the variant name ("TokenMintMany", "TokenSetPriceForDirectPurchase") and representative data (e.g., an element from the Vec or the price value); mimic the existing test_token_operation_debug structure.
🤖 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/util/batch/drive_op_batch/token.rs`:
- Around line 314-606: Add unit tests covering the three missing
TokenOperationType variants and the Some case for price: implement tests named
like test_token_set_status_construction (construct
TokenOperationType::TokenSetStatus with test_token_id() and a TokenStatus
variant and assert fields), test_token_history_construction (construct
TokenOperationType::TokenHistory with test_token_id(), test_identity_id(), nonce
and a TokenEvent and assert token_id/owner_id/nonce),
test_token_mark_perpetual_release_construction (construct
TokenOperationType::TokenMarkPerpetualReleaseAsDistributed with test_token_id(),
test_recipient_id(), a RewardDistributionMoment value and assert fields), and
test_token_set_price_some (construct
TokenOperationType::TokenSetPriceForDirectPurchase with price:
Some(TokenPricingSchedule) and assert token_id and price.is_some()); use the
helper functions test_token_id()/test_identity_id()/test_recipient_id() and
existing enum types TokenStatus, TokenEvent, RewardDistributionMoment,
TokenPricingSchedule to build valid values.
---
Nitpick comments:
In
`@packages/rs-dpp/src/identity/state_transition/asset_lock_proof/instant/instant_asset_lock_proof.rs`:
- Around line 277-284: The test test_new_stores_fields_correctly should
construct an InstantAssetLockProof using InstantAssetLockProof::new(...) instead
of using the raw fixture return value directly; obtain the expected
output_index, instant_lock, and transaction from
raw_instant_asset_lock_proof_fixture(None, None) (or from dedicated fixtures),
call InstantAssetLockProof::new(expected_output_index,
expected_instant_lock.clone(), expected_transaction.clone()), then assert that
proof.output_index() == expected_output_index, proof.instant_lock() ==
&expected_instant_lock, and proof.transaction() == &expected_transaction to
verify the constructor stored all fields correctly.
In `@packages/rs-drive/src/drive/votes/paths.rs`:
- Around line 572-579: Extract a small test helper (e.g.,
assert_path_vec_slice_parity or assert_vec_matches_slice) that accepts a Vec of
path elements and a slice (&[&[u8]] or whatever types used) and asserts equal
length and that each vec_elem.as_slice() == *slice_elem; then replace the
repeated for-loop blocks in functions like
test_vote_decisions_tree_path_vec_matches_slice_form, the tests around lines
595-601, 617-623, 640-646, 669-672, 721-724, 764-767, 794-797, 872-875, and
899-902 to call this helper (pass
vote_decisions_tree_path_vec()/vote_decisions_tree_path() and the analogous
path_vec/path pairs in other tests), removing duplicated loop logic. Ensure the
helper name is descriptive and used consistently across those test functions.
In `@packages/rs-drive/src/util/batch/drive_op_batch/token.rs`:
- Around line 575-605: Add additional unit tests exercising Clone and Debug for
other TokenOperationType variants (e.g., TokenMintMany and
TokenSetPriceForDirectPurchase). For Clone: create instances of TokenMintMany
(with a non-empty Vec) and TokenSetPriceForDirectPurchase (with Some(price)),
call .clone(), and assert cloned fields (Vec length/contents and Option value)
match originals; name tests like test_token_operation_clone_mint_many and
test_token_operation_clone_set_price. For Debug: format!("{:?}", instance) for
the same variants and assert the debug string contains the variant name
("TokenMintMany", "TokenSetPriceForDirectPurchase") and representative data
(e.g., an element from the Vec or the price value); mimic the existing
test_token_operation_debug structure.
In `@packages/rs-drive/src/util/object_size_info/document_info.rs`:
- Around line 503-527: Rename the test function test_estimated_size_for_owner_id
to a name that reflects it only verifies constant sizes (e.g.,
test_system_field_size_constants) and remove the unnecessary drop(info) call;
update the test declaration (the function name) that references DocumentInfo and
assert constants DEFAULT_HASH_SIZE_U16, U64_SIZE_U16, and U32_SIZE_U16 so the
test name matches its actual purpose.
- Around line 584-600: Add a unit test that covers the
DocumentEstimatedAverageSize variant for get_storage_flags_ref: construct a
DocumentInfo::DocumentEstimatedAverageSize instance (e.g. with an estimated size
value), call info.get_storage_flags_ref(), and assert it returns
StorageFlags::optional_default_as_ref(); reference get_storage_flags_ref,
DocumentEstimatedAverageSize, and StorageFlags::optional_default_as_ref() so the
test verifies this specific branch.
🪄 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: 31efb203-0b8d-4905-b3a9-af0e58c4eab9
📒 Files selected for processing (4)
packages/rs-dpp/src/identity/state_transition/asset_lock_proof/instant/instant_asset_lock_proof.rspackages/rs-drive/src/drive/votes/paths.rspackages/rs-drive/src/util/batch/drive_op_batch/token.rspackages/rs-drive/src/util/object_size_info/document_info.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3452 +/- ##
============================================
+ Coverage 83.13% 83.30% +0.16%
============================================
Files 2621 2621
Lines 270434 272169 +1735
============================================
+ Hits 224833 226726 +1893
+ Misses 45601 45443 -158
🚀 New features to boost your workflow:
|
Add tests for TokenSetStatus, TokenHistory, TokenMarkPerpetualReleaseAsDistributed, and TokenSetPriceForDirectPurchase with Some(price). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 101 tests across 4 files in rs-drive and rs-dpp.
drive/votes/paths.rsutil/object_size_info/document_info.rsutil/batch/drive_op_batch/token.rsasset_lock_proof/instant/instant_asset_lock_proof.rsTest plan
cargo test -p drive --libandcargo test -p dpp --libpasscargo fmt --allclean🤖 Generated with Claude Code
Summary by CodeRabbit