test(dpp): restore ~60 commented-out/ignored tests#3122
test(dpp): restore ~60 commented-out/ignored tests#3122thepastaclaw wants to merge 9 commits intodashpay:v3.1-devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMultiple test modules across the rs-dpp package were activated or rewritten (removed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (1)
843-854:⚠️ Potential issue | 🔴 CriticalCompile error: missing
token_configurationsargument in firstDocumentTypeV1::try_from_schemacall.
DocumentTypeV1::try_from_schemarequires 11 arguments (see line 80–92). This call passes only 10:&BTreeMap::new()fortoken_configurationswas added to the second call in this test (line 878) but omitted here, shifting&config,true,&mut vec![], andplatform_versionall into the wrong parameter slots.cargo checkskips#[cfg(test)]by default, so this won't surface untilcargo testcompiles the test code.🐛 Proposed fix
let result = DocumentTypeV1::try_from_schema( Identifier::new([1; 32]), 1, config.version(), "invalid name", schema.clone(), None, + &BTreeMap::new(), &config, true, &mut vec![], platform_version, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs` around lines 843 - 854, The first call to DocumentTypeV1::try_from_schema is missing the token_configurations argument causing all subsequent parameters to be misaligned; update the call in the test to pass an appropriate token_configurations value (e.g., &BTreeMap::new()) as the token_configurations parameter so the signature for DocumentTypeV1::try_from_schema (the 11-argument overload used in tests) matches the second call and the following arguments (&config, true, &mut vec![], platform_version) remain in their correct positions.
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs (1)
430-435: Tautologicalassert_eq!(33, compressed.len())— the meaningful assertion is alreadyassert_ne!.
get_compressed_public_ec_keyreturnsResult<[u8; 33], _>. After unwrapping,compressedhas type[u8; 33], socompressed.len()is a compile-time constant33. The assertion can never fail; only theassert_ne!([0; 33], compressed)below it actually exercises anything. Consider replacing with a comment or removing the length assertion.♻️ Suggested cleanup
let compressed = get_compressed_public_ec_key(&[1; 32]).expect("expected key"); - - assert_eq!(33, compressed.len()); assert_ne!([0; 33], compressed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs` around lines 430 - 435, The test function should_get_compressed_public_key_for_valid_private_key contains a tautological assertion asserting the length is 33 even though get_compressed_public_ec_key returns [u8; 33] (compile-time fixed); remove the meaningless assert_eq!(33, compressed.len()) and either leave a short comment noting the array length is guaranteed by the return type or just keep the meaningful assertion assert_ne!([0; 33], compressed) to validate the key bytes; locate this change in the test function name should_get_compressed_public_key_for_valid_private_key and the call to get_compressed_public_ec_key.
🤖 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/state_transition/state_transitions/contract/data_contract_update_transition/json_conversion.rs`:
- Around line 49-83: The binding json_object in the test is declared mutable but
never mutated, causing an unused_mut lint; update the test by removing the mut
qualifier from json_object (the variable created by
get_test_data().to_json(...)) so it is immutable, leaving all subsequent .get()
assertions intact; reference json_object, get_test_data, to_json, and
JsonStateTransitionSerializationOptions to locate and change the declaration.
---
Outside diff comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 843-854: The first call to DocumentTypeV1::try_from_schema is
missing the token_configurations argument causing all subsequent parameters to
be misaligned; update the call in the test to pass an appropriate
token_configurations value (e.g., &BTreeMap::new()) as the token_configurations
parameter so the signature for DocumentTypeV1::try_from_schema (the 11-argument
overload used in tests) matches the second call and the following arguments
(&config, true, &mut vec![], platform_version) remain in their correct
positions.
---
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs`:
- Around line 430-435: The test function
should_get_compressed_public_key_for_valid_private_key contains a tautological
assertion asserting the length is 33 even though get_compressed_public_ec_key
returns [u8; 33] (compile-time fixed); remove the meaningless assert_eq!(33,
compressed.len()) and either leave a short comment noting the array length is
guaranteed by the return type or just keep the meaningful assertion
assert_ne!([0; 33], compressed) to validate the key bytes; locate this change in
the test function name should_get_compressed_public_key_for_valid_private_key
and the call to get_compressed_public_ec_key.
...ate_transition/state_transitions/contract/data_contract_update_transition/json_conversion.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs (1)
17-37:⚠️ Potential issue | 🟡 MinorRename test functions: four tests don't reflect current behavior and two violate naming conventions.
Tests at lines 90 (
invalid_result_for_array_of_object) and 101 (invalid_result_for_array_of_objects) don't follow theshould_...naming convention required for new test functions. Rename toshould_be_valid_result_for_array_of_objectandshould_be_valid_result_for_array_of_objects.Tests at lines 17 (
should_return_error_if_bytes_array_parent_contains_items_or_prefix_items) and 61 (should_return_invalid_result) have names that contradict their assertions—both claim errors/invalid results but assert.is_valid()returnstrue. Since the validator with an empty validator list now returns valid results, rename these to accurately reflect the tested behavior:should_return_valid_result_for_bytes_array_parent_with_itemsandshould_return_valid_result_for_pattern.All four tests should clearly indicate they expect valid results, matching the assertions they contain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs` around lines 17 - 37, Rename the four test functions to match the current behavior and naming conventions: change should_return_error_if_bytes_array_parent_contains_items_or_prefix_items to should_return_valid_result_for_bytes_array_parent_with_items, change the test at line 61 named should_return_invalid_result to should_return_valid_result_for_pattern, and rename invalid_result_for_array_of_object and invalid_result_for_array_of_objects to should_be_valid_result_for_array_of_object and should_be_valid_result_for_array_of_objects respectively; update the function identifiers (fn ...) and any internal references or test attribute names so the assertions that expect .is_valid() remain correct and the names follow the should_... convention.
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs (1)
429-435:assert_eq!(33, compressed.len())is a tautology on[u8; 33].
get_compressed_public_ec_keyreturns[u8; 33]; the Rust compiler knowslen()is33at compile time. The assertion will always pass regardless of what the function returns, so it adds no test value. The meaningful check is theassert_ne!on the next line.♻️ Suggested fix
- assert_eq!(33, compressed.len()); assert_ne!([0; 33], compressed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs` around lines 429 - 435, The test should_remove the tautological length check because get_compressed_public_ec_key returns a [u8; 33] (len() is always 33); update the test function should_get_compressed_public_key_for_valid_private_key to drop the assert_eq!(33, compressed.len()) and instead keep/strengthen the meaningful assertion(s) (e.g., assert_ne!([0; 33], compressed) or add a content-based validation) so the test verifies the actual output of get_compressed_public_ec_key rather than a compile-time property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/rs-dpp/src/data_contract/document_type/schema/recursive_schema_validator/mod.rs`:
- Around line 17-37: Rename the four test functions to match the current
behavior and naming conventions: change
should_return_error_if_bytes_array_parent_contains_items_or_prefix_items to
should_return_valid_result_for_bytes_array_parent_with_items, change the test at
line 61 named should_return_invalid_result to
should_return_valid_result_for_pattern, and rename
invalid_result_for_array_of_object and invalid_result_for_array_of_objects to
should_be_valid_result_for_array_of_object and
should_be_valid_result_for_array_of_objects respectively; update the function
identifiers (fn ...) and any internal references or test attribute names so the
assertions that expect .is_valid() remain correct and the names follow the
should_... convention.
---
Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/traits/state_transition_identity_signed.rs`:
- Around line 429-435: The test should_remove the tautological length check
because get_compressed_public_ec_key returns a [u8; 33] (len() is always 33);
update the test function should_get_compressed_public_key_for_valid_private_key
to drop the assert_eq!(33, compressed.len()) and instead keep/strengthen the
meaningful assertion(s) (e.g., assert_ne!([0; 33], compressed) or add a
content-based validation) so the test verifies the actual output of
get_compressed_public_ec_key rather than a compile-time property.
thepastaclaw
changed the title
thepastaclaw
force-pushed
the
fix/restore-commented-tests
branch
from
9e44333 to
8476dc8
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
force-pushed
the
fix/restore-commented-tests
branch
from
9c7a22c to
74b6bf8
Compare
- Restore DATA_CONTRACT_IDENTIFIER_FIELDS_V0 constant in value v0 module - Fix u32 -> u16 return type for state_transition_protocol_version mock - Add missing token_configurations argument to try_from_schema v1 test - Construct BatchTransition mock via BatchTransitionV0::default()
- recursive_schema_validator: empty validator list now yields valid results, update 4 tests to assert Ok instead of expecting errors - should_set_empty_schema_defs (v0+v1): add minimal document type to fixture to satisfy new DocumentTypesAreMissingError validation - data_contract_update_transition json_conversion: transition type not present in non-validating JSON serialization All 523 tests now pass: cargo test -p dpp
The #[test] attribute fix and schema_defs test improvements for should_set_empty_schema_defs are already covered by PR dashpay#3101. Removing duplicates to keep PRs independent.
thepastaclaw
force-pushed
the
fix/restore-commented-tests
branch
from
74b6bf8 to
bb4ceaf
Compare
1 participant
Issue being fixed or feature implemented
Restore ~60 commented-out, ignored, and broken tests in
rs-dppthat were silently disabled during API migrations. These tests cover critical paths: signing/validation, serialization, schema validation, and document factory operations.Tracking issue: thepastaclaw/tracker#12.
What was done?
Nine incremental commits, one per fix area:
#[test]attributes toschema_defstests in v0/v1DocumentTypeV0instead ofDocumentTypeV1intry_from_schema/v1#[ignore]from 6recursive_schema_validatortestsDataContractV0serialization tests (updated to current API)DataContractUpdateTransitionJSON conversion testDocumentFactorytests (updated to current API)How Has This Been Tested?
cargo test -p dpp --lib→ 523 passed, 0 failedcargo test -p dpp(including doctests) → all passValidation
All DPP-related CI checks pass on the head commit (
893becfe):Downstream packages also pass (confirming no ripple effects from test-only changes):
Two pre-existing CI failures unrelated to this PR:
Local validation (pre-push):
cargo test -p dpp --lib— 523 tests passed, 0 failedcargo test -p dpp(full, including doctests) — all passBreaking Changes
None. This PR contains only test code changes.
Checklist:
For repository code-owners and collaborators only