Uh oh!
There was an error while loading. Please reload this page.
test(platform): improve platform-value coverage for patch, value_map, converters, and replacement - #3428
Merged
Merged
Conversation
…and replacement Add 219 tests across 5 files covering real logic: - patch/mod.rs: RFC 6902 JSON Patch operations, rollback, merge - value_map.rs: sorting, key lookup, conditional removal, map conversion - converter/serde_json.rs: JSON conversion, U128 bounds, byte heuristic - converter/ciborium.rs: CBOR round-trips, tag rejection, key sorting - btreemap_field_replacement.rs: type replacement, path traversal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request adds extensive unit tests (2,559 lines) across rs-platform-value converters, patching, and map helpers, and removes a 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 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:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## v3.1-dev #3428 +/- ##
============================================
+ Coverage 80.22% 80.63% +0.41%
============================================
Files 2855 2855 Lines 281685 285727 +4042 ============================================
+ Hits 225983 230405 +4422 + Misses 55702 55322 -380
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/rs-platform-value/src/btreemap_extensions/btreemap_field_replacement.rs (1)
684-692: Rename this test to match its expected behavior.The name says “error” but the assertion expects
Ok(()). Renaming avoids confusion during failures and triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-value/src/btreemap_extensions/btreemap_field_replacement.rs` around lines 684 - 692, The test function replace_at_path_empty_path_error has a misleading name because it asserts Ok(()); rename the function to reflect successful behavior (e.g., replace_at_path_empty_path_ok or replace_at_path_empty_path_returns_ok) so the name matches the assertion; locate the test function replace_at_path_empty_path_error and update its identifier accordingly, keeping the body and assertion unchanged.packages/rs-platform-value/src/converter/ciborium.rs (1)
530-535: Make this a real round-trip assertion.Right now any non-empty byte slice passes. Decoding the buffer and comparing it with the original value would actually verify serialization correctness.
Suggested change
#[test] fn to_cbor_buffer_roundtrip() { let val = Value::Text("cbor buffer test".into()); let buf = val.to_cbor_buffer().unwrap(); - assert!(!buf.is_empty());+ let decoded: CborValue = ciborium::de::from_reader(buf.as_slice()).unwrap();+ let back: Value = decoded.try_into().unwrap();+ assert_eq!(back, val); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-value/src/converter/ciborium.rs` around lines 530 - 535, The test to_cbor_buffer_roundtrip currently only asserts the buffer is non-empty; change it to perform a real round-trip by decoding the buffer back into a Value and asserting equality with the original Value; specifically call the corresponding deserializer (e.g., Value::from_cbor_buffer or Value::from_cbor_bytes) on the buf produced by Value::to_cbor_buffer(), unwrap the result, and use assert_eq!(decoded, val) so the test verifies correct serialization/deserialization for the Value type.packages/rs-platform-value/src/value_map.rs (1)
523-605: LGTM! Miscellaneous tests round out the coverage.Tests validate:
- Value-based key operations
- String insertion
BTreeMapconversion- Recursive sorting (verifies both outer and inner map ordering)
Minor: Simplify panic message (optional)
Line 603 could use
unreachable!()or a shorter message:} else { - panic!("expected inner map");+ unreachable!("expected inner map"); }This is purely stylistic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-value/src/value_map.rs` around lines 523 - 605, Replace the explicit panic! call in the test sort_by_keys_and_inner_maps_sorts_recursively with a shorter assertion helper like unreachable!() (or panic! with a shorter message) to simplify the panic message; locate the branch that currently does panic!("expected inner map") inside that test and change it to unreachable!() so the failure is clearer and more idiomatic.
🤖 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-platform-value/src/btreemap_extensions/btreemap_field_replacement.rs`:
- Around line 441-445: The test replace_for_bytes_20_identifier_error asserts
the wrong error variant for 20-byte input; update the expectation so
ReplacementType::Identifier.replace_for_bytes_20(bytes) asserts
Err(Error::ByteLengthNot20BytesError(_)) (or the project's canonical
20-byte-length error variant) instead of
Err(Error::ByteLengthNot36BytesError(_)); locate this in the test function
replace_for_bytes_20_identifier_error and change the matches! pattern
accordingly to reflect the correct length/error contract.
In `@packages/rs-platform-value/src/converter/ciborium.rs`:
- Around line 367-380: The CBOR conversion currently narrows Value::U128 and
Value::I128 into 64-bit without checking overflow; add tests and/or change the
converter to detect out-of-range 128-bit values and return an error like the
JSON converter. Specifically, add unit tests that convert Value::U128(u128::MAX)
and Value::I128(i128::MAX) (and the negative extreme for I128) using try_into()
and assert that conversion fails (Err) or panics as your API dictates;
alternatively, modify the CBOR conversion logic (the TryFrom/try_into
implementation that maps Value::U128/Value::I128 to CborValue::Integer) to check
value > u64::MAX or value < i64::MIN/i64::MAX and return a suitable error
instead of doing an unchecked cast, then update existing tests
(u128_narrowed_to_u64_in_cbor and i128_narrowed_to_i64_in_cbor) to still cover
in-range cases.
In `@packages/rs-platform-value/src/converter/serde_json.rs`:
- Around line 1117-1122: The test currently expects Value::Float(f64::NAN) to
become 0, but the validating conversion should reject non-finite floats instead;
update the Float handling in try_into_validating_json() so it checks
f.is_finite() and returns an Err (propagate a suitable validation error) rather
than coercing to 0, and likewise fix the other conversion paths in this file
where non-finite floats are coerced (the three Float conversion branches) to
either return errors or be explicitly lossy only in the lossy conversion; remove
the test validating_json_float_nan_becomes_zero or update it to assert an error
from try_into_validating_json().
In `@packages/rs-platform-value/src/patch/mod.rs`:
- Around line 765-767: Add a regression test to cover map rollback for `add`
failures instead of skipping it: implement a new #[test] named
`apply_patches_rollback_add_map_on_failure` (analogous to
`apply_patches_rollback_add_array_on_failure`) that creates a map doc (e.g.,
platform_value!({"a": 1})), clones the original, constructs a Patch of two ops —
an `add` to "/a" with a different value followed by a `test` that will fail —
then assert that `patch(&mut doc, &p)` returns an error and that `doc ==
original`, ensuring map state is restored on failure.
---
Nitpick comments:
In
`@packages/rs-platform-value/src/btreemap_extensions/btreemap_field_replacement.rs`:
- Around line 684-692: The test function replace_at_path_empty_path_error has a
misleading name because it asserts Ok(()); rename the function to reflect
successful behavior (e.g., replace_at_path_empty_path_ok or
replace_at_path_empty_path_returns_ok) so the name matches the assertion; locate
the test function replace_at_path_empty_path_error and update its identifier
accordingly, keeping the body and assertion unchanged.
In `@packages/rs-platform-value/src/converter/ciborium.rs`:
- Around line 530-535: The test to_cbor_buffer_roundtrip currently only asserts
the buffer is non-empty; change it to perform a real round-trip by decoding the
buffer back into a Value and asserting equality with the original Value;
specifically call the corresponding deserializer (e.g., Value::from_cbor_buffer
or Value::from_cbor_bytes) on the buf produced by Value::to_cbor_buffer(),
unwrap the result, and use assert_eq!(decoded, val) so the test verifies correct
serialization/deserialization for the Value type.
In `@packages/rs-platform-value/src/value_map.rs`:
- Around line 523-605: Replace the explicit panic! call in the test
sort_by_keys_and_inner_maps_sorts_recursively with a shorter assertion helper
like unreachable!() (or panic! with a shorter message) to simplify the panic
message; locate the branch that currently does panic!("expected inner map")
inside that test and change it to unreachable!() so the failure is clearer and
more idiomatic.
🪄 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: 8fcb46e7-1a03-4fff-82b0-dc8403292c2b
📒 Files selected for processing (5)
packages/rs-platform-value/src/btreemap_extensions/btreemap_field_replacement.rspackages/rs-platform-value/src/converter/ciborium.rspackages/rs-platform-value/src/converter/serde_json.rspackages/rs-platform-value/src/patch/mod.rspackages/rs-platform-value/src/value_map.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
5 tasks
Adds a test for map-based add rollback that documents the current behavior — rollback after adding a new map key doesn't fully restore the original due to ValueMap's append-only insert. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes cargo-machete CI failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Excellent test coverage PR adding 219 tests across platform-value modules. Agent findings about 'misleading round_trip names' and 'tests documenting bugs' were verified against the code and found to be intentional: the tests explicitly document known conversion behaviors (lossy 128-bit integers in CBOR, NaN normalization in JSON) with clear comments explaining why. The rollback tests correctly capture the current partial-rollback semantics. No actionable issues found — the tests are well-structured, clearly named within their module context, and provide genuine coverage improvement.
Reviewed commit: 40bfc2f
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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 219 tests across 5 previously untested files in rs-platform-value, covering real logic.
patch/mod.rsvalue_map.rsconverter/serde_json.rsconverter/ciborium.rsbtreemap_field_replacement.rsTest plan
cargo test -p platform-value --libpassescargo fmt --allclean🤖 Generated with Claude Code
Summary by CodeRabbit