test(platform): add 466 unit tests across 15 files for coverage gains#3427
Conversation
Add comprehensive unit tests to previously untested utility modules, enum conversions, serialization helpers, and validation logic across rs-dpp, rs-drive, and rs-platform-value. Tested modules: - validation_result: all builder/query/transform methods - identity key enums: security_level, purpose, key_type conversions - token types: gas_fees_paid_by, token_pricing_schedule, token_event - token_configuration_item: 32-variant index uniqueness - authorized_action_takers: bytes round-trips and permission logic - fee_result: BalanceChangeForIdentity and FeeResult balance methods - encode.rs: sort-preserving u64/i64/float/u16/u32 encoding - platform-value: Display, PartialEq cross-type, string_encoding - util/vec.rs: hex encode/decode round-trips Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request adds comprehensive unit test coverage across multiple modules in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
Review GateCommit:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/rs-dpp/src/tokens/gas_fees_paid_by.rs (1)
113-118: Assert specific error variants for invalid conversions.These tests currently accept any error. It’s better to lock the contract to the expected protocol/basic error path to catch regressions in error mapping.
Suggested tightening of assertions
#[test] fn try_from_u8_invalid_value_returns_error() { - let result = GasFeesPaidBy::try_from(3u8); - assert!(result.is_err()); - let result = GasFeesPaidBy::try_from(255u8); - assert!(result.is_err()); + let result = GasFeesPaidBy::try_from(3u8); + assert!(matches!( + result, + Err(ProtocolError::ConsensusError(_)) + )); + let result = GasFeesPaidBy::try_from(255u8); + assert!(matches!( + result, + Err(ProtocolError::ConsensusError(_)) + )); } @@ fn try_from_u64_invalid_small_value_returns_error() { let result = GasFeesPaidBy::try_from(3u64); - assert!(result.is_err()); + assert!(matches!( + result, + Err(ProtocolError::ConsensusError(_)) + )); } @@ fn try_from_u64_large_value_returns_error() { let result = GasFeesPaidBy::try_from(256u64); - assert!(result.is_err()); + assert!(matches!( + result, + Err(ProtocolError::ConsensusError(_)) + )); let result = GasFeesPaidBy::try_from(u64::MAX); - assert!(result.is_err()); + assert!(matches!( + result, + Err(ProtocolError::ConsensusError(_)) + )); }As per coding guidelines, “Follow rustfmt defaults and keep code clippy-clean for Rust modules” and “Ensure Rust code passes clippy linter checks via
cargo clippy --workspace.”Also applies to: 137-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/tokens/gas_fees_paid_by.rs` around lines 113 - 118, The tests try_from_u8_invalid_value_returns_error (and the similar test at 137-148) currently only assert that an error occurred; change them to assert the exact expected error variant from GasFeesPaidBy::try_from (e.g., match on the returned Result and assert_eq! or pattern-match that the error equals the protocol/basic invalid-conversion variant your crate uses) so the test fails if error mapping changes; update both occurrences to unwrap_err() and compare to the concrete error enum/variant used by your conversion logic rather than using assert!(result.is_err()).packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs (2)
339-344: Add overlong-input coverage forIdentitydeserialization errors.Line 339 currently checks only undersized payloads. Add an oversized case too, since
from_bytesrequires exactly 33 bytes.Proposed additional assertion
#[test] fn from_bytes_identity_wrong_length_returns_error() { // tag 2 needs exactly 33 bytes total let short = vec![2; 10]; // only 10 bytes let result = AuthorizedActionTakers::from_bytes(&short); assert!(result.is_err()); + + let long = vec![2; 34]; // 34 bytes + let result = AuthorizedActionTakers::from_bytes(&long); + assert!(result.is_err()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs` around lines 339 - 344, The test function from_bytes_identity_wrong_length_returns_error currently asserts only for an undersized Identity payload; add a complementary oversized case to ensure deserialization rejects inputs longer than the required 33 bytes: after the existing short vector test, create an overlong vector starting with tag 2 and total length >33 (e.g., vec![2; 50]) and call AuthorizedActionTakers::from_bytes(&overlong) and assert it returns an Err; update the same test function so both undersized and oversized Identity payloads are covered.
258-262: Strengthendisplay_identityassertion to verify full formatting contract.Current check only validates the prefix, so output regressions after
Identity(would still pass.Proposed test hardening
#[test] fn display_identity() { let id = make_id(0xAB); - let display = format!("{}", AuthorizedActionTakers::Identity(id)); - assert!(display.starts_with("Identity(")); + let expected = format!("Identity({})", id); + assert_eq!(format!("{}", AuthorizedActionTakers::Identity(id)), expected); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs` around lines 258 - 262, The test display_identity currently only checks a prefix; change it to assert the full formatted output by constructing the exact expected string using make_id(0xAB) and comparing it to the display result (replace the starts_with assertion with an equality check). Locate function display_identity and the AuthorizedActionTakers::Identity usage and replace the prefix assertion with an assert that display equals format!("Identity({})", id) (or the equivalent full expected string built from id).packages/rs-dpp/src/fee/fee_result/mod.rs (1)
397-405: Reduce duplicated fixture reconstruction in moved-value tests.Line 397 and Line 467 rebuild equivalent fixtures just to call
fee_result_outcome. SinceBalanceChangeForIdentityisClone, cloning can keep these tests shorter and easier to maintain.♻️ Suggested simplification
- // Cannot access change after move, re-create - let refunds2 = fee_refunds_for_identity(id, 500); - let fee_result2 = FeeResult { - storage_fee: 100, - processing_fee: 50, - fee_refunds: refunds2, - removed_bytes_from_system: 0, - }; - let bci2 = fee_result2.into_balance_change(id); - let result: Result<FeeResult, FeeError> = bci2.fee_result_outcome(0); + let result: Result<FeeResult, FeeError> = bci.clone().fee_result_outcome(0); assert!(result.is_ok());- // Re-create for outcome check - let refunds2 = fee_refunds_for_identity(id, 150); - let fee_result2 = FeeResult { - storage_fee: 100, - processing_fee: 50, - fee_refunds: refunds2, - removed_bytes_from_system: 0, - }; - let bci2 = fee_result2.into_balance_change(id); - let result: Result<FeeResult, FeeError> = bci2.fee_result_outcome(0); + let result: Result<FeeResult, FeeError> = bci.clone().fee_result_outcome(0); assert!(result.is_ok());Also applies to: 467-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/fee/fee_result/mod.rs` around lines 397 - 405, Tests rebuild equivalent FeeResult fixtures (using fee_refunds_for_identity, FeeResult, into_balance_change) after moving values; instead, reuse the previously created BalanceChangeForIdentity by deriving it once and cloning it where needed (BalanceChangeForIdentity implements Clone) and then pass the cloned instance to fee_result_outcome rather than reconstructing refunds/fee_result a second time; update the two test sites (around the uses of fee_result2/into_balance_change and the later equivalent block) to call .clone() on the original balance change variable and remove the duplicated fixture construction.packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (1)
301-341: Make enum-coverage tests resilient to future variant additions.At Line 301,
all_variants()is manually curated. If a newTokenConfigurationChangeItemvariant is added but this helper is not updated, the Line 345–406 index tests can still pass and give false confidence. Consider adding an exhaustivematch-based expected-index helper (without_arm) so enum growth forces test updates at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs` around lines 301 - 341, Replace the fragile manually-curated all_variants() with an exhaustive, match-based helper that maps every TokenConfigurationChangeItem variant to its expected index (or produce the Vec by matching each variant explicitly) so adding a new TokenConfigurationChangeItem causes a compile-time error; specifically, create a function (or modify existing expected-index helper) that matches on TokenConfigurationChangeItem without a catch-all (_) arm and returns the index/value for each variant, referencing the TokenConfigurationChangeItem enum and the all_variants()/expected-index helper to locate where to implement this change.packages/rs-dpp/src/tokens/token_event.rs (1)
483-938: Optional: Consider parameterized tests for repetitive patterns.The test suite is comprehensive and well-organized. For future maintenance, you could reduce repetition using a parameterized testing crate like
rstest:#[rstest] #[case(TokenEvent::Mint(0, test_id(), None), "mint")] #[case(TokenEvent::Burn(0, test_id(), None), "burn")] // ... other variants fn associated_name_parameterized(#[case] event: TokenEvent, #[case] expected: &str) { assert_eq!(event.associated_document_type_name(), expected); }However, the current explicit approach is perfectly maintainable and offers clear test names for debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/tokens/token_event.rs` around lines 483 - 938, Suggest refactoring repetitive tests into parameterized cases to reduce duplication: identify groups testing TokenEvent::associated_document_type_name (tests calling associated_document_type_name on various TokenEvent variants) and the format_note helper (tests format_note(&None) and format_note(&Some(...))); convert them into parameterized tests (e.g., using rstest) that feed (TokenEvent, expected_name) and (Option<String>, expected_str) cases respectively, keeping the remaining descriptive tests (Display and public_note) as-is; update Cargo.toml to add rstest as a [dev-dependencies] if not present and replace the repetitive assert_eq! loops with single parameterized test functions referencing associated_document_type_name and format_note.
🤖 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-dpp/src/tokens/token_event.rs`:
- Around line 554-558: Test display_unfreeze_with_note only checks the note
suffix; update it so the formatted string also includes the main prefix by
asserting the output starts with or contains "Unfreeze". Specifically, in the
display_unfreeze_with_note test (which constructs
TokenEvent::Unfreeze(test_id(), Some("thawed".to_string())) and formats it), add
an assertion that s.starts_with("Unfreeze") or assert!(s.contains("Unfreeze"))
before/alongside the existing note assertion to match other "with note" Display
tests.
In `@packages/rs-dpp/src/util/vec.rs`:
- Around line 107-141: The decode_hex function currently slices two chars at a
time and can panic on odd-length input; update decode_hex to first check if
input.len() % 2 != 0 and return an Err variant (consistent with its Result type)
for odd-length strings, then proceed with the existing two-character parsing
logic; add a regression test (e.g., test_decode_hex_odd_length) that calls
decode_hex("abc") and asserts it returns Err to prevent future regressions.
---
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs`:
- Around line 301-341: Replace the fragile manually-curated all_variants() with
an exhaustive, match-based helper that maps every TokenConfigurationChangeItem
variant to its expected index (or produce the Vec by matching each variant
explicitly) so adding a new TokenConfigurationChangeItem causes a compile-time
error; specifically, create a function (or modify existing expected-index
helper) that matches on TokenConfigurationChangeItem without a catch-all (_) arm
and returns the index/value for each variant, referencing the
TokenConfigurationChangeItem enum and the all_variants()/expected-index helper
to locate where to implement this change.
In
`@packages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rs`:
- Around line 339-344: The test function
from_bytes_identity_wrong_length_returns_error currently asserts only for an
undersized Identity payload; add a complementary oversized case to ensure
deserialization rejects inputs longer than the required 33 bytes: after the
existing short vector test, create an overlong vector starting with tag 2 and
total length >33 (e.g., vec![2; 50]) and call
AuthorizedActionTakers::from_bytes(&overlong) and assert it returns an Err;
update the same test function so both undersized and oversized Identity payloads
are covered.
- Around line 258-262: The test display_identity currently only checks a prefix;
change it to assert the full formatted output by constructing the exact expected
string using make_id(0xAB) and comparing it to the display result (replace the
starts_with assertion with an equality check). Locate function display_identity
and the AuthorizedActionTakers::Identity usage and replace the prefix assertion
with an assert that display equals format!("Identity({})", id) (or the
equivalent full expected string built from id).
In `@packages/rs-dpp/src/fee/fee_result/mod.rs`:
- Around line 397-405: Tests rebuild equivalent FeeResult fixtures (using
fee_refunds_for_identity, FeeResult, into_balance_change) after moving values;
instead, reuse the previously created BalanceChangeForIdentity by deriving it
once and cloning it where needed (BalanceChangeForIdentity implements Clone) and
then pass the cloned instance to fee_result_outcome rather than reconstructing
refunds/fee_result a second time; update the two test sites (around the uses of
fee_result2/into_balance_change and the later equivalent block) to call .clone()
on the original balance change variable and remove the duplicated fixture
construction.
In `@packages/rs-dpp/src/tokens/gas_fees_paid_by.rs`:
- Around line 113-118: The tests try_from_u8_invalid_value_returns_error (and
the similar test at 137-148) currently only assert that an error occurred;
change them to assert the exact expected error variant from
GasFeesPaidBy::try_from (e.g., match on the returned Result and assert_eq! or
pattern-match that the error equals the protocol/basic invalid-conversion
variant your crate uses) so the test fails if error mapping changes; update both
occurrences to unwrap_err() and compare to the concrete error enum/variant used
by your conversion logic rather than using assert!(result.is_err()).
In `@packages/rs-dpp/src/tokens/token_event.rs`:
- Around line 483-938: Suggest refactoring repetitive tests into parameterized
cases to reduce duplication: identify groups testing
TokenEvent::associated_document_type_name (tests calling
associated_document_type_name on various TokenEvent variants) and the
format_note helper (tests format_note(&None) and format_note(&Some(...)));
convert them into parameterized tests (e.g., using rstest) that feed
(TokenEvent, expected_name) and (Option<String>, expected_str) cases
respectively, keeping the remaining descriptive tests (Display and public_note)
as-is; update Cargo.toml to add rstest as a [dev-dependencies] if not present
and replace the repetitive assert_eq! loops with single parameterized test
functions referencing associated_document_type_name and format_note.
🪄 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: 698d2271-cea1-444c-8fc9-946cf7c8f4a3
📒 Files selected for processing (15)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rspackages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rspackages/rs-dpp/src/fee/fee_result/mod.rspackages/rs-dpp/src/identity/identity_public_key/key_type.rspackages/rs-dpp/src/identity/identity_public_key/purpose.rspackages/rs-dpp/src/identity/identity_public_key/security_level.rspackages/rs-dpp/src/tokens/gas_fees_paid_by.rspackages/rs-dpp/src/tokens/token_event.rspackages/rs-dpp/src/tokens/token_pricing_schedule.rspackages/rs-dpp/src/util/vec.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-drive/src/util/common/encode.rspackages/rs-platform-value/src/display.rspackages/rs-platform-value/src/eq.rspackages/rs-platform-value/src/string_encoding.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3427 +/- ##
============================================
+ Coverage 79.93% 80.15% +0.22%
============================================
Files 2861 2855 -6
Lines 280236 281386 +1150
============================================
+ Hits 223993 225547 +1554
+ Misses 56243 55839 -404
🚀 New features to boost your workflow:
|
- Add prefix assertion to display_unfreeze_with_note test - Add odd-length hex test documenting panic behavior (#should_panic) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-dpp/src/tokens/token_event.rs (1)
554-558:⚠️ Potential issue | 🟡 MinorAdd the missing main-prefix assertion in
display_unfreeze_with_note.On Line 557, this test only validates the note suffix; it should also assert the formatted output includes the
"Unfreeze "prefix like other “with note” Display tests.🧪 Suggested test tightening
#[test] fn display_unfreeze_with_note() { let event = TokenEvent::Unfreeze(test_id(), Some("thawed".to_string())); let s = format!("{}", event); + assert!(s.starts_with("Unfreeze ")); assert!(s.contains("(note: thawed)")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/tokens/token_event.rs` around lines 554 - 558, The test display_unfreeze_with_note only checks the note suffix; update it to also assert the main prefix by verifying the formatted string includes the "Unfreeze " prefix (e.g., assert!(s.contains("Unfreeze "))) for the TokenEvent::Unfreeze case using test_id() so it matches the other "with note" Display tests; modify the display_unfreeze_with_note function to add this assertion alongside the existing note check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/rs-dpp/src/tokens/token_event.rs`:
- Around line 554-558: The test display_unfreeze_with_note only checks the note
suffix; update it to also assert the main prefix by verifying the formatted
string includes the "Unfreeze " prefix (e.g., assert!(s.contains("Unfreeze ")))
for the TokenEvent::Unfreeze case using test_id() so it matches the other "with
note" Display tests; modify the display_unfreeze_with_note function to add this
assertion alongside the existing note check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a19eea32-57a9-4e63-8748-b1810529d0a3
📒 Files selected for processing (8)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rspackages/rs-dpp/src/data_contract/change_control_rules/authorized_action_takers.rspackages/rs-dpp/src/identity/identity_public_key/key_type.rspackages/rs-dpp/src/identity/identity_public_key/purpose.rspackages/rs-dpp/src/tokens/token_event.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-platform-value/src/display.rspackages/rs-platform-value/src/string_encoding.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-platform-value/src/display.rs
- packages/rs-dpp/src/identity/identity_public_key/purpose.rs
- packages/rs-dpp/src/identity/identity_public_key/key_type.rs
- packages/rs-platform-value/src/string_encoding.rs
- packages/rs-dpp/src/validation/validation_result.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs
Remove tests for enum TryFrom/Display boilerplate and trivial formatting that should be excluded from coverage rather than tested: - security_level.rs, purpose.rs, gas_fees_paid_by.rs (enum conversions) - display.rs, string_encoding.rs (Value formatting, encoding wrappers) - Display tests from token_event.rs and token_configuration_item.rs Add corresponding .codecov.yml exclusions. Keep tests with real logic: validation_result, encode.rs, fee_result, authorized_action_takers, token_event associated_document_type_name, index uniqueness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs (1)
349-349: Prefer.copied()over.cloned()forCopytypes.For primitive types like
u8that implementCopy, using.copied()is more idiomatic and signals intent more clearly.Suggested change
- let unique: BTreeSet<u8> = indices.iter().cloned().collect(); + let unique: BTreeSet<u8> = indices.iter().copied().collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs` at line 349, Replace the use of .cloned() with .copied() when building the BTreeSet of u8 values: in the code that creates unique (currently let unique: BTreeSet<u8> = indices.iter().cloned().collect();), use indices.iter().copied().collect() instead to be more idiomatic for the Copy type u8.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rs`:
- Line 349: Replace the use of .cloned() with .copied() when building the
BTreeSet of u8 values: in the code that creates unique (currently let unique:
BTreeSet<u8> = indices.iter().cloned().collect();), use
indices.iter().copied().collect() instead to be more idiomatic for the Copy type
u8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a3ad978-50fc-4371-9b76-96cd6c8de4bc
📒 Files selected for processing (4)
.codecov.ymlpackages/rs-dpp/src/data_contract/associated_token/token_configuration_item.rspackages/rs-dpp/src/tokens/token_event.rspackages/rs-dpp/src/util/vec.rs
✅ Files skipped from review due to trivial changes (3)
- .codecov.yml
- packages/rs-dpp/src/tokens/token_event.rs
- packages/rs-dpp/src/util/vec.rs
|
Self Reviewed |
Summary
Adds 466 unit tests across 15 previously untested or under-tested files in rs-dpp, rs-drive, and rs-platform-value. Target: ~1% coverage improvement from 79.93%.
Files tested
validation/validation_result.rsutil/vec.rsidentity/.../security_level.rsidentity/.../purpose.rsidentity/.../key_type.rstokens/gas_fees_paid_by.rstokens/token_pricing_schedule.rstokens/token_event.rstokens/token_configuration_item.rschange_control_rules/authorized_action_takers.rsfee/fee_result/mod.rsutil/common/encode.rsstring_encoding.rsdisplay.rseq.rsTest plan
cargo test -p dpp --lib— 1808 passedcargo test -p drive --lib— 1818 passedcargo test -p platform-value --lib— 689 passed🤖 Generated with Claude Code
Summary by CodeRabbit