Add test coverage for ordered_json/alt_json across binary formats and patch/diff/flatten APIs - #5480
Open
nlohmann wants to merge 2 commits into
Open
Add test coverage for ordered_json/alt_json across binary formats and patch/diff/flatten APIs#5480nlohmann wants to merge 2 commits into
nlohmann wants to merge 2 commits into
Conversation
… patch/diff/flatten APIs Closes a test-coverage gap from #5421: ordered_json (and the alt_string-based basic_json specialization from unit-alt-string.cpp) were never round-tripped through the binary formats (CBOR/MessagePack/UBJSON/BSON/BJData), nor through flatten()/unflatten(), diff()/patch()/patch_inplace(), or merge_patch(). Also adds a std::formatter<ordered_json> spot-check, mirroring the precedent set by the format_as() ADL-deduction test. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
5 tasks
…dy modernize-pass-by-value) Signed-off-by: Niels Lohmann <mail@nlohmann.me>
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
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
Fixes #5421.
tests/src/unit-ordered_json.cppis only 83 lines and mostly checks basic insertion order; the rest of the test suite exercisesordered_jsonalmost nowhere. A repo-wide grep for::(to|from)_(cbor|msgpack|ubjson|bson|bjdata)shows ~2000 hits forjson::*and zero forordered_json. Thealt_jsoncustom-string_tspecialization fromtests/src/unit-alt-string.cppwas likewise never exercised through the binary formats.This PR adds a new test file,
tests/src/unit-ordered_json2.cpp(picked up automatically by thefile(GLOB ... src/unit-*.cpp)intests/CMakeLists.txt— no build-file changes needed), plus a small addition to the existingtests/src/unit-std-format.cpp.What's covered
ordered_jsonandalt_json, across CBOR, MessagePack, UBJSON, BSON, and BJData: a representative nested value (object + array + nested object + bool/null/number/string), with keys inserted in non-alphabetical order.ordered_json, in addition to==equality, the actual iteration order of keys (top-level and nested) is compared explicitly before/after the round trip, since preserving that order is the whole point ofordered_json.alt_jsonneeded two additions to thealt_stringhelper (duplicated locally in the new file rather than shared, since everyunit-*.cppcompiles into its own standalone test binary): astd::stringconstructor and afind(char, pos)overload. Without them,to_bson/from_ubjsonfail to compile — presumably why this gap existed, sinceunit-alt-string.cppnever exercises the binary readers/writers.ordered_json::operator==is order-sensitive (unlikejson, whoseobject_tisstd::map):ordered_mapdoesn't define its ownoperator==, so it inheritsstd::vector's element-wise comparison. This is worth a maintainer's attention as a documented/intentional behavior difference — see "Surprises" below.jsonandordered_json. Both end up with a single entry holding the last value (viaoperator[]), which is worth contrasting with the first-value-wins behavior of the initializer-list construction path already pinned inunit-ordered_json.cpp.flatten()/unflatten()round trip onordered_json, including a top-level and nested key-order check.diff()/patch()/patch_inplace()round trip onordered_json(replace + remove + add).merge_patch()onordered_json, including a key-order check after a patch that removes and adds keys.std::formatter<ordered_json>instantiation check added tounit-std-format.cpp(guarded by the same#if JSON_HAS_STD_FORMAT), confirming the formatter — written against the genericNLOHMANN_BASIC_JSON_TPL_DECLARATION— actually instantiates and works for a non-defaultbasic_jsontemplate argument. This mirrors the precedent set byunit-format-as.cpp's ADL-deduction test forformat_as().Surprises / findings worth flagging
ordered_json::operator==is order-sensitive. Twoordered_jsonobjects with identical key/value pairs inserted in different order compare unequal, becauseordered_maphas no customoperator==and inheritsstd::vector's element-wise comparison.json'soperator==(viastd::map) never has this issue. This isn't necessarily a bug, but it's a sharp edge worth documenting explicitly if it isn't already, since it's easy to assumeordered_jsonequality behaves likejson's aside fromdump()output order.alt_string(inunit-alt-string.cpp) doesn't compile against the binary writers/readers as-is — it's missing astd::stringconstructor (needed by the UBJSON/BSON high-precision-number SAX path) and afind(char, pos)overload (needed by BSON's embedded-NUL check on string keys). I worked around this locally in the new file rather than touchingunit-alt-string.cpp, to keep this PR strictly additive/test-only. Might be worth adding those two members to the canonicalalt_stringin a follow-up so other tests can exercise it against the binary formats too.jsonandordered_json(last value wins, single entry) but differs from the initializer-list construction path (first value wins) — already implicitly documented inunit-ordered_json.cpp, now explicitly cross-referenced.Left out of scope
Per the issue's own list of stretch goals, the following were intentionally left out to keep this PR focused:
alt_jsonnon-binary APIs (update(),merge_patch(),unflatten(),emplace/emplace_back,std::hash, three-way comparison).basic_json(fromunit-allocator.cpp) through the binary readers.Does this PR introduce a breaking change?
No. This PR only adds tests (
tests/src/unit-ordered_json2.cppis new, andtests/src/unit-std-format.cppgains one additional guardedTEST_CASE). No files underinclude/were touched, somake amalgamatewas not needed and the single-header/public API is unaffected.— opened by Claude Code on behalf of @nlohmann