test(platform): improve platform-value coverage for inner_value, system_bytes, and serde#3384
Merged
Merged
Conversation
…em_bytes, and serde Add comprehensive unit tests for four modules with low test coverage: - inner_value.rs: tests for all map accessor/mutator methods (get_str, get_integer, get_bool, get_bytes, get_array, get_hash256, get_identifier, etc.) with both correct and wrong type inputs, plus insert/remove operations and error cases on non-map values. - system_bytes.rs: tests for encode/decode round-trips across all system byte types (identifier_bytes, binary_bytes, hash256, bytes_20, bytes_32, bytes_36, identifier) from multiple Value variants (Bytes, Text, Array, Identifier, Bytes32) with error cases. - value_serialization/de.rs: tests for serde Deserializer covering all integer types, floats, bools, strings, chars, options, arrays, maps, structs, tuples, enums, unit types, newtype structs, and round-trip Rust-to-Value-to-Rust deserialization. - value_serialization/ser.rs: tests for serde Serializer covering all primitive types, sequences, tuples, maps, structs, enums (unit/newtype/tuple/struct variants), bytes serialization with size-based dispatch, Value self-serialization, and MapKeySerializer error cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughFour test modules have been added to the rs-platform-value crate, establishing comprehensive coverage for Value operations, byte conversions, and serde serialization/deserialization. No production code modifications were made. 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)
📝 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-value/src/system_bytes.rs (1)
1459-1464: Minor: Unnecessary clone.The
arr.clone()on line 1461 is unnecessary sincearris not used after theValue::Array(arr.clone())call. This is a very minor nit.Suggested fix
#[test] fn to_identifier_from_array_of_32_u8s() { let arr: Vec<Value> = (0..32).map(|i| Value::U8(i as u8)).collect(); - let val = Value::Array(arr.clone()); + let val = Value::Array(arr); let expected: [u8; 32] = core::array::from_fn(|i| i as u8); assert_eq!(val.to_identifier().unwrap(), Identifier::new(expected)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-value/src/system_bytes.rs` around lines 1459 - 1464, In the test function to_identifier_from_array_of_32_u8s, remove the unnecessary arr.clone() when constructing Value::Array since arr isn’t used afterward; change Value::Array(arr.clone()) to Value::Array(arr) so ownership moves and avoids an extra clone of arr.
🤖 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-platform-value/src/system_bytes.rs`:
- Around line 1459-1464: In the test function
to_identifier_from_array_of_32_u8s, remove the unnecessary arr.clone() when
constructing Value::Array since arr isn’t used afterward; change
Value::Array(arr.clone()) to Value::Array(arr) so ownership moves and avoids an
extra clone of arr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3c2d531-22ba-4db7-ae17-5a455d45db17
📒 Files selected for processing (4)
packages/rs-platform-value/src/inner_value.rspackages/rs-platform-value/src/system_bytes.rspackages/rs-platform-value/src/value_serialization/de.rspackages/rs-platform-value/src/value_serialization/ser.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3384 +/- ##
============================================
+ Coverage 75.87% 76.84% +0.97%
============================================
Files 2912 2914 +2
Lines 283860 287933 +4073
============================================
+ Hits 215375 221260 +5885
+ Misses 68485 66673 -1812
🚀 New features to boost your workflow:
|
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
1 participant
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.
Issue being fixed or feature implemented
Improves unit test coverage for
rs-platform-valuemodules that had low coverage:inner_value.rs(65.8% -> improved)system_bytes.rs(32.2% -> improved)value_serialization/de.rs(54.6% -> improved)value_serialization/ser.rs(55.8% -> improved)What was done?
Added comprehensive
#[cfg(test)] mod testsblocks to four files:inner_value.rs(93 new tests):get_str,get_integer,get_bool,get_bytes,get_array,get_hash256,get_identifier, etc.get_optional_*for all typesset_into_value,set_value,insert,insert_at_endremove,remove_many,remove_optional_value,remove_integer,remove_identifier,remove_bytes_32,remove_hash256_bytes,remove_bytes,remove_binary_data,remove_arrayget_from_map,get_optional_from_map,insert_in_map,push_to_map_string_valuesystem_bytes.rs(73 new tests):into_identifier_bytes/to_identifier_bytesfrom Bytes, Text (bs58), Array, Identifier, Bytes32into_binary_bytes/to_binary_bytesfrom Bytes, Text (b64), Array, Bytes20/32/36, Identifierinto_hash256/to_hash256from Bytes32, Identifier, Bytes, Text (bs58), Array, with wrong-length errorsinto_bytes_20/to_bytes_20,into_bytes_32/to_bytes_32,into_bytes_36/to_bytes_36with round-trip b64 encodinginto_identifier/to_identifierfrom all source types with error casesvalue_serialization/de.rs(56 new tests):value_serialization/ser.rs(56 new tests):How Has This Been Tested?
Breaking Changes
None. Only test code was added.
Checklist:
Generated with Claude Code
Summary by CodeRabbit