Repository navigation
Cut test suite runtime in binary roundtrips and integer sweeps - #5519
Conversation
28c9b26 to
bd223ac
Compare
|
Please rebase to the latest develop branch as I had to fix some tests. |
The Linux CI jobs pass --no-skip, so skip() does not help there. Parse each corpus file once in the binary roundtrip loops instead of four times. Sample the 16-bit integer ranges with stride 7 (still hits every low byte) and always keep the endpoints. Also drop the 5M-node parse test to 500k, which still covers the non-recursive destructor, and move jeopardy.json into its own skipped test so the cheaper binary-format size checks actually run. See nlohmann#5418. Signed-off-by: ayush-singh-0601 <singhayush062006@gmail.com>
ci_test_gcc compiles with -Werror=useless-cast. On that compiler int32_t is int, so static_cast<int32_t> of the loop bound is an error. The bounds are already int, and the sampled values do not change. Signed-off-by: ayush-singh-0601 <singhayush062006@gmail.com>
bd223ac to
60eb722
Compare
|
Rebased onto the latest develop. The gcc job was failing as well, so I fixed that in the same push. |
nlohmann
left a comment
There was a problem hiding this comment.
Thanks! Most of this PR is fine to merge. The stride-7 integer sweeps still hit every boundary and every byte value, the roundtrip hoisting only removes duplicated parsing, and 500k depth is still plenty for unit-large_json.
I'd like one change: please revert tests/src/unit-binary_formats.cpp to develop's version.
The split doesn't save time, because of how skipped tests run in CI (cmake/test.cmake):
| Job type | Command | Runs skip() tests? |
|---|---|---|
| Default (gcc/clang/sanitizer/coverage) | test-foo --no-skip |
yes |
JSON_FastTests=ON |
test-foo |
no |
| Valgrind | valgrind test-foo |
no |
On develop, "Binary Formats" is skipped, so it runs only in the default jobs. With this PR:
- Default
--no-skipjobs still run both test cases,jeopardy.jsonincluded, so there is no saving. - FastTests and valgrind jobs never ran any of this before. Now they run canada/twitter/citm/sample. Under valgrind,
test-binary_formats_valgrindwent from about 2 s to about 223 s (compared with develop CI run 35829411620). That makes the valgrind job slower overall (ctest time 390 s → 527 s), even though the other changes should have made it faster.
So the cheaper files don't "keep running": they never ran in those jobs. It isn't a coverage gain either. The only checks that run in more places are the encoded-size checks for four files, and the default and coverage jobs already run those.
If you'd like something from that file in the fast jobs, un-skipping just sample.json would be acceptable, since it's small. Otherwise, a plain revert is best.
Nit: the comment on next_integer_sample in tests/src/test_utils.hpp says "Walk [first, last]", but the function has no first parameter.
This review was written by Claude Code on behalf of @nlohmann.
Revert tests/src/unit-binary_formats.cpp to its develop state. The test-case split made valgrind jobs slower instead of faster, because the cheaper corpus files (canada/twitter/citm/sample) now ran under valgrind where they never did before. Fix the next_integer_sample comment: the function has no 'first' parameter, so describe what the function actually does. Signed-off-by: ayush-singh-0601 <singhayush062006@gmail.com>
|
Addressed both points from the review:
No other files were touched. Ready for another look when you have a chance. |
See #5418.
Linux CI runs
--no-skip, so the expensive binary roundtrip cases actually run there. Each corpus file was parsed four times (once per input adapter). Parse once and reuse it.The 16-bit integer sweeps still prove width selection: stride 7 hits every low-byte residue, and the endpoints stay in the loop.
unit-large_jsonused depth 5,000,000 to check the non-recursive destructor from #1419. 500,000 is enough.jeopardy.jsonis ~500 MB of serialization output, so it is now its own skipped test; canada/twitter/citm/sample keep running.Unicode byte sweeps are left to #5426.
make amalgamate.