✅ increase coverage #40
Conversation
WalkthroughAdds and updates unit tests across decode, encode, list format, utils, and decode options, increasing edge-case coverage for comma/list limits, depth/strictDepth semantics, allowDots behavior, legacy decoder fallback, surrogate/charset handling and many encode edge cases. No runtime or public API changes except a test-only Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestCaller
participant QS
participant DecodeOptions
participant PrimaryDecoder as Primary decoder
participant LegacyDecoder as Legacy decoder
TestCaller->>QS: QS.decode(input, options)
QS->>DecodeOptions: resolve decoders, limits, flags
alt primary decoder present
QS->>PrimaryDecoder: decode(value, charset, kind?)
PrimaryDecoder-->>QS: decoded value
else fallback to legacy decoder
QS->>LegacyDecoder: decode(value, charset)
LegacyDecoder-->>QS: decoded value
end
QS-->>TestCaller: decoded structure (applying depth/list limits)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
+ Coverage 94.52% 98.10% +3.57%
==========================================
Files 14 14
Lines 895 895
==========================================
+ Hits 846 878 +32
+ Misses 49 17 -32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (3)
test/unit/models/decode_options_test.dart (1)
351-363: Rename test for accuracy (it’s not using the deprecated path)This test directly invokes
opts.decoder(...). It doesn’t validate any “deprecated” forwarding; it asserts the primary decoder receives kind/charset/value. Consider renaming for clarity.Apply this diff:
- test('deprecated decoder forwards to decode implementation', () { + test('decoder exposes kind and charset arguments', () {test/unit/encode_test.dart (1)
18-23: Optional: make the helper less throwy
operator []throwing on unknown keys can hide unrelated failures if the object leaks into encode without the filter. Returningnullis often safer in test doubles.- String? operator [](String key) { - if (key == 'prop') return value; - throw UnsupportedError('Only prop supported'); - } + String? operator [](String key) => key == 'prop' ? value : null;test/unit/utils_additional_test.dart (1)
17-19: Avoid asserting on concrete Map subtypeCasting to
SplayTreeMapmakes the test brittle against internal changes. Assert on Map emptiness instead.- final splay = result as SplayTreeMap; - expect(splay.isEmpty, isTrue); + expect(result, isA<Map>()); + expect((result as Map).isEmpty, isTrue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/unit/decode_test.dart(1 hunks)test/unit/encode_test.dart(2 hunks)test/unit/list_format_test.dart(1 hunks)test/unit/models/decode_options_test.dart(1 hunks)test/unit/utils_additional_test.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
test/unit/list_format_test.dart (1)
22-24: LGTMGood coverage for generator outputs and enum string representation.
test/unit/encode_test.dart (1)
97-110: LGTMNice, concise filter-based expansion path; asserts the exact encoded shape.
test/unit/decode_test.dart (1)
2796-2874: LGTMUseful, targeted additions for comma limits, depth strictness, and parameter limit coercion.
…m lists, dot encoding in keys, and cycle detection
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
lib/src/extensions/encode.dart (1)
131-148: Optional: extract dot-in-keys handling into a small helper.The primitive fast‑path now has its own dot-encoding logic in addition to the non‑primitive path. Extracting a
String _applyDotEncoding(String prefix, {required bool allowDots, required bool encodeDotInKeys, required bool isLeaf})would centralise the rules and reduce future drift.test/unit/encode_test.dart (1)
18-23: Tighten operator return type.
operator []never returnsnull(it throws for unsupported keys), so the return type can beStringinstead ofString?.Apply this diff:
- String? operator [](String key) { + String operator [](String key) { if (key == 'prop') return value; throw UnsupportedError('Only prop supported'); }test/unit/utils_test.dart (1)
1243-1273: Avoid coupling tests to internal chunk size (1024).Hard-coding
segmentLimit = 1024makes the test brittle if the implementation changes its internal chunking. Prefer an assertion that verifies surrogate pairs aren’t split and the encoded emoji sequence appears intact, without depending on the exact boundary.For example, drop the boundary constant and only assert:
- startsWith the expected run of 'a's,
- contains the
%F0%9F%98%80sequence,- endsWith the tail.
test/unit/encode_edge_cases_test.dart (2)
68-82: Make percent-encoding checks case-insensitive to avoid brittle failuresSome encoders may emit lowercase hex. Use case-insensitive regex instead of literal .contains.
- expect(encoded.contains('a%5Bx%5D%5Bk%5D=v'), isTrue); - expect(encoded.contains('b%5By%5D%5Bz%5D%5Bk%5D=v'), isTrue); + expect(RegExp(r'a%5Bx%5D%5Bk%5D=v', caseSensitive: false).hasMatch(encoded), isTrue); + expect(RegExp(r'b%5By%5D%5Bz%5D%5Bk%5D=v', caseSensitive: false).hasMatch(encoded), isTrue);
25-26: Tighten the occurrence check to the specific key pathCounting every "=1" is permissive and could false-positive. Count the exact 'z' path occurrences instead.
- final occurrences = '=1'.allMatches(encoded).length; + final occurrences = + RegExp(r'%5Bz%5D=1', caseSensitive: false).allMatches(encoded).length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/encode.dart(1 hunks)test/unit/decode_test.dart(1 hunks)test/unit/encode_edge_cases_test.dart(1 hunks)test/unit/encode_test.dart(3 hunks)test/unit/utils_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/decode_test.dart
🔇 Additional comments (9)
lib/src/extensions/encode.dart (1)
150-155: LGTM: key/value emission respects encoder/formatter semantics.Using
encoderfor values (and for keys whenencodeValuesOnly == false) and always passing throughformatteraligns with the rest of the encoder.test/unit/encode_test.dart (2)
97-110: LGTM: filter-based expansion of custom objects.The test clearly validates the filter hook maps
CustomObjectinto a serialisable map, matching encoder expectations.
5240-5283: LGTM: additional coverage meaningfully exercises edge paths.Good assertions for
allowEmptyLists, comma round-trip, dot-in-keys withallowDots, and cycle detection.test/unit/utils_test.dart (4)
3-3: LGTM: dart:collection import is appropriate.Required for
SplayTreeMapuse in merge tests.
1164-1184: LGTM: surrogate and charset edge cases improve decoder/encoder robustness.Good coverage for lone surrogates and latin1 behaviour on invalid escapes.
1186-1241: LGTM: merge edge branches are well covered.Solid scenarios around
Undefined, scalar/iterable combinations, and index map promotion.
1275-1295: LGTM: helper coverage is precise.Nice checks on
isNonNullishPrimitivewithUri, astral-plane numeric entity handling, and index map materialisation.test/unit/encode_edge_cases_test.dart (2)
110-121: Confirm intended behaviour: encoder mutates keys when strictNullHandling omits the valueThis asserts the encoder is applied to the key (yielding 'X_nil') when the value is null and strictNullHandling is true. Please confirm this is an intentional, documented behaviour so future refactors don’t break it silently. If intentional, consider adding a brief note in the EncodeOptions/encoder docs.
84-97: LGTM: good fast‑path coverage for dot-in-keys with encodeValuesOnlyThis nicely exercises dot encoding on keys while restricting the encoder to values.
… with optional trailing '='
…ing encoded strings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/unit/encode_edge_cases_test.dart (4)
10-10: Guarantee the throwing indexer path is exercisedIf the encoder checks membership (e.g., containsKey/keys) before indexing, 'boom' not being present in _store could skip operator[] and not hit the try/catch path. Seed 'boom' with a null entry so the indexer is invoked and throws as intended.
Apply:
- final Map<String, dynamic> _store = {'ok': 42}; + final Map<String, dynamic> _store = {'ok': 42, 'boom': null};
134-137: Tighten assertion to expect percent-encoded bracketsThe suite generally asserts percent-encoded brackets. Make the check strict to avoid masking regressions.
- // Allow either percent-encoded or raw bracket form (both are acceptable depending on encoding path), - // and an optional trailing '=' if future changes emit an explicit empty value. - final pattern = RegExp(r'^(outer%5Bp%5D%5B%5D=?|outer\[p\]\[\](=?))$'); - expect(encoded, matches(pattern)); + // Expect percent-encoded bracket form; allow optional trailing '='. + expect(encoded.startsWith('outer%5Bp%5D%5B%5D'), isTrue);As per previous review feedback.
46-49: Prefer asserting explicit key paths over counting '=1'Counting '=1' is brittle. Assert both expected encoded paths to remove ambiguity.
- // Accept either ordering; just verify two occurrences of '=1'. - final occurrences = '=1'.allMatches(encoded).length; - expect(occurrences, 2); + expect(encoded.contains('a%5Bz%5D=1'), isTrue); + expect(encoded.contains('b%5Bz%5D=1'), isTrue);
171-172: Make the mutating encoder null-safeFuture changes could accidentally pass null to the encoder; avoid a null dereference.
-String _mutatingEncoder(dynamic v, {Encoding? charset, Format? format}) => - 'X_${v.toString()}'; +String _mutatingEncoder(dynamic v, {Encoding? charset, Format? format}) => + v == null ? '' : 'X_${v.toString()}';test/unit/encode_test.dart (2)
97-110: Filter expansion of custom objects looks goodGood coverage of the filter=function path. Optionally add a negative case demonstrating that unsupported keys on CustomObject are never accessed when the filter expands it, to guard against future regressions in filter handling.
5240-5271: Avoid redundant coverage; consider consolidating with existing testsThe added tests duplicate existing cases (allowEmptyLists, commaRoundTrip single item, cyclic RangeError). Consolidate or reference earlier tests to keep the suite lean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/unit/encode_edge_cases_test.dart(1 hunks)test/unit/encode_test.dart(3 hunks)
🔇 Additional comments (1)
test/unit/encode_test.dart (1)
18-23: CustomObject: clear contract for supported keysNon-nullable operator[] with UnsupportedError for unsupported keys is appropriate and matches the updated tests.
…t for shared objects
This pull request significantly expands unit test coverage for the
qs_dartpackage, focusing on edge cases, error handling, and utility behaviors in both encoding and decoding logic. The changes add new test groups and cases across several test files, targeting areas such as cycle detection, list formatting, surrogate character encoding, custom object handling, and decoder fallback logic.Encode/Decode Logic Coverage:
decode_test.dartfor comma splitting with list limits, strict/non-strict depth handling, parameter limit coercion, and dot segment parsing, improving coverage of edge behaviors in decoding (test/unit/decode_test.dart).encode_edge_cases_test.dart, including cycle detection, strict null handling, custom encoder/filter behaviors, empty list handling, and shared object serialization (test/unit/encode_edge_cases_test.dart).encode_test.dart(test/unit/encode_test.dart). [1] [2] [3]Utility Function Coverage:
utils_test.dartfor surrogate character encoding, charset edge cases, merge logic (scalars, iterables, maps), encoding boundaries, primitive detection, numeric entity interpretation, and index map creation (test/unit/utils_test.dart).Options and List Format Testing:
decode_options_test.dart(test/unit/models/decode_options_test.dart).ListFormatgenerator variants and their string representation inlist_format_test.dart(test/unit/list_format_test.dart).These additions substantially improve code coverage, especially for error branches, boundary cases, and custom configuration paths.