Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage 100.00% diff coverage · +5.69% coverage variation
Metric Results Coverage variation ✅ +5.69% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8d82a4a) 5048 4651 92.14% Head commit (fe3b832) 5048 (+0) 4938 (+287) 97.82% (+5.69%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4) 3 3 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
TIP This summary will be updated as you push new changes. Give us feedback
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR expands test coverage across the decode/scan/accumulate/compact pipelines (plus some encode and temporal helpers) to harden edge-case behavior and error/limit handling, without changing production logic.
Changes:
- Adds new targeted test modules for decode key parsing, part scanning, and scalar helper behavior (custom decoders, numeric entities, delimiter edge cases).
- Extends existing decode/encode/compact/temporal tests to cover additional helper paths, limit/error branches, and root-replacement filter behavior.
- Adjusts internal visibility/re-exports under
cfg(test)(and a couple ofpub(super)helpers) to make helper functions accessible to the new tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/temporal/tests.rs | Adds coverage for FromStr, public accessors, and remaining parse/format helper edge cases. |
| src/structured_scan/tests.rs | Adds a small helper-focused test for percent inputs and empty segments. |
| src/options/encode.rs | Adds EncodeOptions builder/getter/validation tests. |
| src/key_path/tests.rs | Adds tests for node reuse in append/dot-encoding helpers. |
| src/encode/tests/scalar.rs | New tests for scalar helper behavior (arrays/objects/bytes/temporal null-likes). |
| src/encode/tests/mod.rs | Re-exports additional encode scalar helpers and registers new test module. |
| src/encode/tests/filters.rs | Adds test ensuring root function filters can replace the root container. |
| src/encode/scalar.rs | Broadens plain_scalar_text visibility for test use. |
| src/encode/filter.rs | Adds unit tests for filter/filtered-encode helper paths (depth, omit, root replacement). |
| src/encode.rs | Imports scalar helpers under cfg(test) for tests. |
| src/decode/tests/scanner.rs | Expands scanner tests for charset sentinel, regex delimiter, and metadata edge cases. |
| src/decode/tests/scalar_helpers.rs | New tests for decode scalar helpers and recursive numeric-entity interpretation. |
| src/decode/tests/parts.rs | New tests for part scanners (empty segments, multi-byte delimiters) and default byte scanner routing. |
| src/decode/tests/mod.rs | Re-exports additional internal decode helpers and registers new test modules. |
| src/decode/tests/keys.rs | New tests for key parsing/recovery, dot-decoding, and list-limit error reporting. |
| src/decode/tests/flat.rs | Adds tests for decode_pairs empty input and flat-value helper behaviors/limits. |
| src/decode/scan/parts.rs | Broadens scanner helper visibility within crate::decode for tests. |
| src/decode/scan.rs | Re-exports scan helpers under cfg(test) for test modules. |
| src/decode/accumulate/process/tests.rs | Adds extensive tests for duplicate modes, storage promotions, and hard-limit error propagation. |
| src/decode/accumulate/insert/tests.rs | Adds regression test for Duplicates::First behavior when late parsed values arrive. |
| src/decode/accumulate/build.rs | Adds tests for list parsing/building, limit overflow/errors, and null/decoder edge paths. |
| src/decode.rs | Extends cfg(test) imports to support new test modules. |
| src/compact/tests.rs | Adds tests covering node_to_value / node_to_object conversions for arrays/scalars/undefined. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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 13 minutes and 46 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 (3)
📝 WalkthroughWalkthroughExpanded test coverage and test-scoped visibility across many modules; added numerous new unit tests and test submodules; adjusted helper visibilities for testing; and updated GitHub Actions to extract MSRV dynamically and add a required-job aggregator that fails CI when dependencies do not all succeed. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/encode/filter/tests.rs (1)
12-98: Consider splitting this broad test into smaller focused cases.This single test covers multiple independent behaviors; splitting would make failures more actionable and reduce triage time.
✂️ Suggested structure
-#[test] -fn filtered_encode_helpers_cover_fast_path_depth_and_child_traversal() { +#[test] +fn encode_node_filtered_encodes_nested_object() { + // fast path assertion +} + +#[test] +fn encode_node_filtered_returns_empty_for_omitted_input() { + // omitted assertion +} + +#[test] +fn encode_node_filtered_returns_depth_exceeded_error() { + // depth assertion +} + +#[test] +fn encode_node_filtered_handles_empty_list_when_allowed() { + // empty list assertion +} + +#[test] +fn encode_node_filtered_encodes_dotted_keys_and_skips_null_object_fields() { + // dotted + null skip assertion +} + +#[test] +fn encode_node_filtered_skips_null_array_entries_without_reindexing() { + // array null skip assertion +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/encode/filter/tests.rs` around lines 12 - 98, The test function filtered_encode_helpers_cover_fast_path_depth_and_child_traversal is exercising many independent cases; split it into multiple focused #[test] functions each targeting one behavior (e.g., fast-path object traversal using encode_node_filtered with EncodeInput::Borrowed and KeyPathNode::from_raw("a"), omitted input behavior with EncodeInput::Omitted, max-depth error with EncodeOptions::with_max_depth, empty-list handling with with_allow_empty_lists + ListFormat::Indices, dotted-key encoding with with_allow_dots/with_encode_dot_in_keys/with_skip_nulls, and skipping nulls in arrays with with_skip_nulls). For each new test, keep setup minimal, assert only the single expected output or error (using assert_eq! or assert!(error.is_depth_exceeded())), and preserve the same calls to encode_node_filtered, EncodeInput, KeyPathNode, EncodeOptions, and ListFormat so existing helpers and types remain reusable..github/workflows/test.yml (1)
33-40: Fail immediately ifrust-versionis not extracted.If the
awkmatcher stops matching after a futureCargo.tomledit, this step still succeeds and the MSRV job gets an empty toolchain value. A guard here keeps the failure local and obvious.Suggested change
msrv="$( awk -F'"' ' /^\[package\]$/ { in_package = 1; next } /^\[/ { in_package = 0 } in_package && /^rust-version[[:space:]]*=/ { print $2; exit } ' Cargo.toml )" + if [[ -z "${msrv}" ]]; then + echo "::error::rust-version not found in Cargo.toml" + exit 1 + fi echo "msrv=${msrv}" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 33 - 40, The msrv extraction can yield an empty string and should fail fast: after the awk extraction that assigns msrv, add a guard that checks if msrv is empty (e.g., test -z "$msrv") and if so emit an error to stderr and exit non‑zero so the job fails locally; only write msrv to GITHUB_OUTPUT when the check passes. This references the msrv variable assignment and the echo "msrv=${msrv}" >> "$GITHUB_OUTPUT" line—ensure the empty-check occurs before that echo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 180-182: Replace the branch reference for the GitHub Action usage
of dtolnay/rust-toolchain (the line currently using
dtolnay/rust-toolchain@master) with the action's full 40-character commit SHA
while preserving the dynamic input to toolchain (the existing toolchain: ${{
needs.msrv-meta.outputs.msrv }}), and add an inline comment after the SHA with
the human-readable version tag (e.g., # vX.Y.Z) for readability; ensure the
unique symbol dtolnay/rust-toolchain is updated only to the SHA and no other
workflow logic is changed.
In `@src/encode/tests/scalar.rs`:
- Around line 40-51: The test uses ASCII 'A' which doesn't differentiate
charsets; update the second case in src/encode/tests/scalar.rs to use a
non-ASCII single-byte value (e.g., 0xE9) so Iso88591 decoding is actually
validated: pass Value::Bytes(vec![0xE9]) to plain_string_for_comma with
EncodeOptions::new().with_encode(false).with_charset(Charset::Iso88591) and
assert the result equals "é"; keep the existing utf8_bytes case unchanged. This
targets the plain_string_for_comma function and EncodeOptions/Charset::Iso88591
usage so the test verifies decoding differences.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 33-40: The msrv extraction can yield an empty string and should
fail fast: after the awk extraction that assigns msrv, add a guard that checks
if msrv is empty (e.g., test -z "$msrv") and if so emit an error to stderr and
exit non‑zero so the job fails locally; only write msrv to GITHUB_OUTPUT when
the check passes. This references the msrv variable assignment and the echo
"msrv=${msrv}" >> "$GITHUB_OUTPUT" line—ensure the empty-check occurs before
that echo.
In `@src/encode/filter/tests.rs`:
- Around line 12-98: The test function
filtered_encode_helpers_cover_fast_path_depth_and_child_traversal is exercising
many independent cases; split it into multiple focused #[test] functions each
targeting one behavior (e.g., fast-path object traversal using
encode_node_filtered with EncodeInput::Borrowed and KeyPathNode::from_raw("a"),
omitted input behavior with EncodeInput::Omitted, max-depth error with
EncodeOptions::with_max_depth, empty-list handling with with_allow_empty_lists +
ListFormat::Indices, dotted-key encoding with
with_allow_dots/with_encode_dot_in_keys/with_skip_nulls, and skipping nulls in
arrays with with_skip_nulls). For each new test, keep setup minimal, assert only
the single expected output or error (using assert_eq! or
assert!(error.is_depth_exceeded())), and preserve the same calls to
encode_node_filtered, EncodeInput, KeyPathNode, EncodeOptions, and ListFormat so
existing helpers and types remain reusable.
🪄 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: f99e91d7-09eb-4382-8507-9d5a49967d8c
📒 Files selected for processing (27)
.github/workflows/test.ymlsrc/compact/tests.rssrc/decode.rssrc/decode/accumulate/build.rssrc/decode/accumulate/build/tests.rssrc/decode/accumulate/insert/tests.rssrc/decode/accumulate/process/tests.rssrc/decode/scan.rssrc/decode/scan/parts.rssrc/decode/tests/flat.rssrc/decode/tests/keys.rssrc/decode/tests/mod.rssrc/decode/tests/parts.rssrc/decode/tests/scalar_helpers.rssrc/decode/tests/scanner.rssrc/encode.rssrc/encode/filter.rssrc/encode/filter/tests.rssrc/encode/scalar.rssrc/encode/tests/filters.rssrc/encode/tests/mod.rssrc/encode/tests/scalar.rssrc/key_path/tests.rssrc/options/encode.rssrc/options/encode/tests.rssrc/structured_scan/tests.rssrc/temporal/tests.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/decode/scan/parts.rs:16
scan_string_partswas widened topub(in crate::decode)primarily to support tests. To keep the decode scanner internals encapsulated (and avoid non-test code gaining additional access), consider moving these unit tests undersrc/decode/scan/...with#[cfg(test)] mod tests;so the function can remainpub(super)/private.
pub(in crate::decode) fn scan_string_parts<F>(
input: &str,
delimiter: &str,
mut visit: F,
) -> Result<(), DecodeError>
src/decode/scan/parts.rs:77
scan_default_parts_by_byte_delimiteris nowpub(in crate::decode)to make it reachable from higher-level tests. If this is only for unit testing, prefer colocating those tests within thescanmodule (using#[cfg(test)] mod tests;) so this helper doesn’t need broader visibility in non-test builds.
pub(in crate::decode) fn scan_default_parts_by_byte_delimiter(
input: &str,
delimiter: u8,
effective_charset: Charset,
options: &DecodeOptions,
values: &mut DefaultAccumulator,
token_count: &mut usize,
has_any_structured_syntax: &mut bool,
) -> Result<(), DecodeError> {
src/encode/scalar.rs:202
plain_scalar_textwas promoted topub(super)to support tests insrc/encode/tests/*, which increases the internal surface area of the scalar module for non-test builds. Consider moving the scalar helper tests undersrc/encode/scalar/...(via#[cfg(test)] mod tests;) so this can remain private and avoid accidental coupling from other encode submodules.
pub(super) fn plain_scalar_text(value: &Value, options: &EncodeOptions) -> Option<String> {
match value {
Value::Null => None,
Value::Bool(boolean) => Some(boolean.to_string()),
Value::I64(number) => Some(number.to_string()),
Value::U64(number) => Some(number.to_string()),
Value::F64(number) => Some(number.to_string()),
Value::String(text) => Some(text.clone()),
Value::Temporal(temporal) => plain_temporal_text(temporal, options),
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 31-38: The MSRV extraction awk used to set msrv uses patterns that
require `[package]` and `rust-version = ...` at column 1, so update the awk
script to accept optional leading/trailing whitespace when matching those tokens
(e.g., change the `/^\[package\]$/` and `/^rust-version[[:space:]]*=/` tests to
allow leading spaces) so the in_package flag and the rust-version capture still
work even if Cargo.toml lines are indented; keep the same variables (`msrv`, the
awk block) and logic for setting in_package and printing $2.
- Around line 204-208: The package guard's regex 'src/.*/tests(/|\.rs$)'
requires a subdirectory under src and misses root-level paths like src/tests and
src/tests.rs; update the grep pattern in the "Package contents" step to make the
intermediate subdirectory optional (for example replace 'src/.*/tests(/|\.rs$)'
with 'src(/.*)?/tests(/|\.rs$)') so root-level src/tests and src/tests.rs are
correctly matched and excluded.
🪄 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: 2882544e-355d-477c-a432-83ecd486fc57
📒 Files selected for processing (28)
.codacy.yml.github/workflows/test.ymlsrc/compact/tests.rssrc/decode.rssrc/decode/accumulate/build.rssrc/decode/accumulate/build/tests.rssrc/decode/accumulate/insert/tests.rssrc/decode/accumulate/process/tests.rssrc/decode/scan.rssrc/decode/scan/parts.rssrc/decode/tests/flat.rssrc/decode/tests/keys.rssrc/decode/tests/mod.rssrc/decode/tests/parts.rssrc/decode/tests/scalar_helpers.rssrc/decode/tests/scanner.rssrc/encode.rssrc/encode/filter.rssrc/encode/filter/tests.rssrc/encode/scalar.rssrc/encode/tests/filters.rssrc/encode/tests/mod.rssrc/encode/tests/scalar.rssrc/key_path/tests.rssrc/options/encode.rssrc/options/encode/tests.rssrc/structured_scan/tests.rssrc/temporal/tests.rs
✅ Files skipped from review due to trivial changes (16)
- src/decode/accumulate/build.rs
- src/options/encode.rs
- src/encode/filter.rs
- .codacy.yml
- src/decode/scan.rs
- src/structured_scan/tests.rs
- src/encode.rs
- src/key_path/tests.rs
- src/encode/filter/tests.rs
- src/encode/tests/scalar.rs
- src/decode/tests/parts.rs
- src/decode/tests/scalar_helpers.rs
- src/decode/accumulate/build/tests.rs
- src/decode/tests/keys.rs
- src/decode/accumulate/insert/tests.rs
- src/decode/tests/scanner.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/compact/tests.rs
- src/decode/scan/parts.rs
- src/decode.rs
- src/encode/tests/filters.rs
- src/encode/tests/mod.rs
- src/decode/tests/flat.rs
- src/temporal/tests.rs
- src/decode/tests/mod.rs
- src/decode/accumulate/process/tests.rs
This pull request significantly expands and improves the test coverage for the decoding and compaction logic, adds new helper functions for internal testing, and enhances CI configuration for robustness and maintainability. The changes are grouped into three main themes: testing improvements, CI enhancements, and minor codebase improvements.
Summary of changes:
The main focus of this PR is to add comprehensive tests for various decoding edge cases, internal helpers, and error handling, ensuring better code reliability. It also refines the CI workflow to dynamically determine the minimum supported Rust version (MSRV) and adds a required CI job for stricter checks. Additionally, some test helper visibility and imports are improved for better modularity.
Testing improvements:
src/compact/tests.rs,src/decode/accumulate/build/tests.rs,src/decode/accumulate/insert/tests.rs,src/decode/tests/flat.rs, andsrc/decode/tests/keys.rs[1] [2] [3] [4] [5] [6].CI and code quality enhancements:
.github/workflows/test.ymlto dynamically read the MSRV fromCargo.toml, use it in the MSRV job, and add a required CI job that ensures all critical jobs succeed before passing [1] [2] [3]..codacy.ymlto exclude test sources from Codacy code-pattern analysis for more accurate code quality reporting.Internal helper visibility and imports:
scan_string_parts,scan_default_parts_by_byte_delimiter) to allow direct testing and improved test imports for modularity [1] [2] [3] [4] [5] [6].These changes collectively improve code reliability, make the CI pipeline stricter and more maintainable, and ensure that edge cases and helper functions are thoroughly tested.
Summary by CodeRabbit
Tests
Chores