test: add cjson and simdjson fixture coverage#43
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cJSON and simdjson git submodules and README; updates CI to checkout submodules; extends Lua tests with fixture-driven equivalence checks; and adds extensive Rust tests and FFI helpers that parse and validate many cJSON/simdjson fixtures and edge cases in eager and lazy modes. ChangesThird-party fixture corpus and compatibility tests
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds third-party JSON fixture coverage (from cJSON and simdjson) and expands both Rust and Lua test suites to validate qjson parsing, path access, array handling, string escaping, booleans, and big-integer i64 overflow behavior against a known corpus.
Changes:
- Added a new Rust integration test that exercises cJSON + simdjson fixtures through both the Rust API and the C FFI surface.
- Expanded the Lua
qjsonvslua-cjsoncompatibility spec to materialize/encode round-trip against the same fixture corpus. - Vendored selected third-party JSON fixture files plus a provenance README.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/third_party_fixtures.rs | New Rust integration test covering fixture parsing, accessors, cursor traversal, and big-int overflow behavior. |
| tests/lua/cjson_compat_spec.lua | Adds fixture-driven parity checks between qjson and lua-cjson (materialization + encode/decode equivalence). |
| tests/fixtures/third_party/README.md | Documents fixture provenance and licensing intent. |
| tests/fixtures/third_party/simdjson/example_config.json | Adds simdjson example fixture. |
| tests/fixtures/third_party/cjson/test1.json | Adds cJSON fixture. |
| tests/fixtures/third_party/cjson/test2.json | Adds cJSON fixture. |
| tests/fixtures/third_party/cjson/test9.json | Adds cJSON fixture. |
| tests/fixtures/third_party/cjson/test10.json | Adds cJSON fixture. |
| tests/fixtures/third_party/cjson/test11.json | Adds cJSON fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/third_party_fixtures.rs (1)
211-226: ⚡ Quick winStrengthen big-integer regression coverage across all fixture cases.
This test parses 3 literals but asserts
QJSON_OUT_OF_RANGEonly forcases[0]. Please add retrieval/assertion checks for the negative-object case and the array-literal case too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/third_party_fixtures.rs` around lines 211 - 226, The test function simdjson_big_integer_literals_parse_but_do_not_fit_i64 currently only checks qjson_get_i64 for cases[0]; update it to also parse and assert retrieval for the negative-object case (cases[1]) and the array-literal case (cases[2]). For each case call parse(...) or Document::parse(...).unwrap(), call qjson_get_i64(doc, b"val".as_ptr() as *const i8, b"val".len(), &mut v) for object cases and the appropriate key/index retrieval for the array case, assert rc == qjson_err::QJSON_OUT_OF_RANGE as c_int, and call qjson_free(doc) after each check (use the existing symbols qjson_get_i64, qjson_err::QJSON_OUT_OF_RANGE, qjson_free to locate and modify the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/third_party_fixtures.rs`:
- Around line 211-226: The test function
simdjson_big_integer_literals_parse_but_do_not_fit_i64 currently only checks
qjson_get_i64 for cases[0]; update it to also parse and assert retrieval for the
negative-object case (cases[1]) and the array-literal case (cases[2]). For each
case call parse(...) or Document::parse(...).unwrap(), call qjson_get_i64(doc,
b"val".as_ptr() as *const i8, b"val".len(), &mut v) for object cases and the
appropriate key/index retrieval for the array case, assert rc ==
qjson_err::QJSON_OUT_OF_RANGE as c_int, and call qjson_free(doc) after each
check (use the existing symbols qjson_get_i64, qjson_err::QJSON_OUT_OF_RANGE,
qjson_free to locate and modify the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5d06a0f-d880-46af-a7ec-9ef2e108221f
📒 Files selected for processing (9)
tests/fixtures/third_party/README.mdtests/fixtures/third_party/cjson/test1.jsontests/fixtures/third_party/cjson/test10.jsontests/fixtures/third_party/cjson/test11.jsontests/fixtures/third_party/cjson/test2.jsontests/fixtures/third_party/cjson/test9.jsontests/fixtures/third_party/simdjson/example_config.jsontests/lua/cjson_compat_spec.luatests/third_party_fixtures.rs
Contributor
Author
|
Good catch on the big-integer test. I added retrieval checks for the negative object case and the array element case too, and verified the focused test with |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/third_party_fixtures.rs (1)
166-170: ⚡ Quick winAvoid exact upstream fixture count coupling.
assert_eq!(cases.len(), 3, ...)is fragile if simdjson adds valid single-document fixtures. Prefer a lower-bound or explicit filename allowlist.Proposed tweak
- assert_eq!( - cases.len(), - 3, - "simdjson jsonexamples currently has three single-document JSON files" - ); + assert!( + cases.len() >= 3, + "expected at least the known simdjson single-document fixtures, got {}", + cases.len() + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/third_party_fixtures.rs` around lines 166 - 170, The test currently asserts an exact upstream fixture count via assert_eq!(cases.len(), 3, ...), which is brittle; change it to a non-strict check such as assert!(cases.len() >= 3, "...") to allow new upstream fixtures, or implement an explicit allowlist by comparing the discovered filenames (the cases collection) against a fixed set of expected filenames (e.g., via contains or set equality) so tests only fail for missing required fixtures; update the assertion surrounding cases.len() in tests/third_party_fixtures.rs accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/third_party_fixtures.rs`:
- Around line 166-170: The test currently asserts an exact upstream fixture
count via assert_eq!(cases.len(), 3, ...), which is brittle; change it to a
non-strict check such as assert!(cases.len() >= 3, "...") to allow new upstream
fixtures, or implement an explicit allowlist by comparing the discovered
filenames (the cases collection) against a fixed set of expected filenames
(e.g., via contains or set equality) so tests only fail for missing required
fixtures; update the assertion surrounding cases.len() in
tests/third_party_fixtures.rs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66e31e55-7749-4a31-b4d9-b86ec0ae1fb7
📒 Files selected for processing (7)
.github/workflows/ci.yml.gitmodulestests/fixtures/third_party/README.mdtests/lua/cjson_compat_spec.luatests/third_party_fixtures.rstests/vendor/cJSONtests/vendor/simdjson
✅ Files skipped from review due to trivial changes (3)
- .gitmodules
- tests/vendor/simdjson
- tests/fixtures/third_party/README.md
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.
Adds cJSON and simdjson fixture coverage by vendoring both upstream repositories as test submodules instead of copying a small hand-picked subset.
The Rust integration test now walks every cJSON
tests/inputs/test*JSON fixture with a matching.expectedfile, parses every.jsonfile under cJSONtests/json-patch-tests/, keeps cJSONtests/inputs/test6as a negative non-JSON parse case, parses every simdjson single-documentjsonexamples/*.json, and splits simdjsonamazon_cellphones.ndjsonso each record is parsed as an individual JSON document. It also ports selected cJSON parser literals for values, numbers, arrays, objects, escaped strings, malformed containers, empty/null values, invalid paths, and type mismatches, plus qjson-specific accessor checks on representative nested fixtures and simdjson big-integer literals.The Lua compatibility spec reads the same upstream JSON fixture paths and compares
qjson.materialize(qjson.decode(...))plusqjson.encode(qjson.decode(...))againstlua-cjson; it also checks every simdjson NDJSON record line-by-line. The Lua CI checkout now initializes submodules so those fixtures are available.Current upstream fixture coverage includes 10 cJSON input files, 10 cJSON expected files, 4 cJSON json-patch JSON files, 1 cJSON negative non-JSON input, 3 simdjson JSON examples, and 793 simdjson NDJSON records, plus the ported literal/regression cases.
Tested with
git diff --checkandmake test.Summary by CodeRabbit