Skip to content

Test ordered_json binary formats, flatten, and patch - #5428

Closed
22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5421-ordered-json-coverage
Closed

22elix3r wants to merge 1 commit into
nlohmann:developfrom
22elix3r:fix/5421-ordered-json-coverage

Conversation

@22elix3r

Copy link
Copy Markdown
Contributor

Summary

unit-ordered_json.cpp only checked insertion order and one compile regression. Binary formats, flatten/unflatten, patch, and update(merge_objects=true) were exercised almost exclusively with default nlohmann::json, so ordered_map-specific behavior could regress silently.

Related Issue

Fixes #5421

Changes Made

  • CBOR, MessagePack, BSON, and UBJSON round-trips that assert insertion order is preserved
  • flatten/unflatten key-order checks
  • diff/patch and patch_inplace round-trips
  • update(..., true) recursive object merge on ordered_json

Testing

Commands executed:

  • cmake -S . -B build -DJSON_BuildTests=ON -DCMAKE_BUILD_TYPE=Debug
  • cmake --build build --target test-ordered_json_cpp11
  • ./tests/test-ordered_json_cpp11 --no-skip -tce='*downloaded*'

Results:

  • 4 test cases passed, 31 assertions, 0 failed

Notes

This does not add every binary-format edge case listed on the issue (duplicate keys in binary maps, BJData, std::format). It pins the order-sensitive APIs most likely to differ from std::map.

No amalgamation change: tests only.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate. (n/a — test-only)

ordered_json was only covered by insertion-order and one compile
regression. Binary round-trips, flatten/unflatten, and patch/update
with merge_objects were untested for ordered_map.

Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
@22elix3r
22elix3r requested a review from nlohmann as a code owner August 27, 2026 02:14
Copilot AI lite review requested due to automatic review settings August 27, 2026 02:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #5421 also mentions testing std::format. Please add this test or remove the "fixes #5421" from the description.

Copy link
Copy Markdown
Owner

Adding to the earlier review — std::format is not the only gap against #5421. Beyond it:

Binary formats. The issue asks for a matrix over {ordered_json, alt_json} × {cbor, msgpack, ubjson, bson}. This PR covers ordered_json × four formats but BJData is missing, and alt_json is absent entirely — which is the half most likely to break, since the binary writers call .size() and iterate string_t byte-wise.

Also still untested with ordered_json:

  • duplicate-key handling in a binary map (the issue calls this out specifically, since ordered_map::emplace differs from std::map's)
  • merge_patch()
  • at / value / contains with json_pointer

Other specializations from the issue, both untouched: the custom-allocator basic_json failure harness never reaching the binary readers, and alt_json with update(), merge_patch(), unflatten(), emplace/emplace_back, std::hash, three-way comparison.

The coverage that is here checks out — I ran all ten new assertions against develop and they pass, including the flatten() key order and the update(merge_objects=true) ordering.


Generated by Claude Code

@nlohmann

nlohmann commented Sep 2, 2026

Copy link
Copy Markdown
Owner

@22elix3r My review comment from Aug 27 hasn't been addressed yet: issue #5421 also asks for a test of std::format — could you add it, or drop the "fixes #5421" from the description?

@nlohmann

nlohmann commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks for this — good coverage of binary-format round trips and flatten/diff/patch for ordered_json.

We landed a more extensive version in #5480: same behaviors plus BJData (in addition to CBOR/MessagePack/UBJSON/BSON), an alt_json (custom string_t) sweep across all five binary formats, explicit key-order-preservation checks via iteration order (not just dump() string equality), a duplicate-key binary-decode regression test, merge_patch() coverage, and a std::format instantiation check. We're moving forward with #5480 and closing this one. Appreciate the contribution.

— closed by Claude Code on behalf of @nlohmann

@nlohmann nlohmann closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing test coverage: ordered_json — binary formats, patch/diff, flatten/unflatten, and other non-default basic_json specializations

3 participants