Skip to content

Add unit tests for JSON serialization#941

Merged
tmadlener merged 4 commits intoAIDASoft:masterfrom
aashirvad08:tests/json-output
Mar 24, 2026
Merged

Add unit tests for JSON serialization#941
tmadlener merged 4 commits intoAIDASoft:masterfrom
aashirvad08:tests/json-output

Conversation

@aashirvad08
Copy link
Copy Markdown
Contributor

This PR adds unit tests covering the JSON serialization functionality.

The tests verify that expected fields are correctly serialized to JSON,
following the discussion in #517. The tests run entirely in memory and
check that struct members are properly converted to JSON.

Closes #517

@andresailer
Copy link
Copy Markdown
Member

Please drop the two commits from the TransportFrameData PR

Copy link
Copy Markdown
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Seconding the comment from @andresailer to drop the TransportFrameData commit(s) from this PR to have it only contain JSON related things.

A few more comments / suggestions inline.

Comment thread tests/unittests/test_json_output.cpp
Comment thread tests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp
Comment thread tests/test_json_output.cpp Outdated
Comment thread tests/test_json_output.cpp Outdated
Comment thread tests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp Outdated
Comment thread tests/test_json_output.cpp Outdated
Comment thread tests/test_json_output.cpp Outdated
@aashirvad08
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review,
I’ve addressed the comments:
moved the JSON test into tests/unittests and integrated it with the unittest_podio target
removed the SPDX header
replaced the magic number with podio::ObjectID::untracked
added the index == 0 check
replaced manual vector checks with Catch2 matchers
removed unnecessary Approx usage
Locally the tests build and run successfully (unittest_podio). Please let me know if any further adjustments are needed.

Copy link
Copy Markdown
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

It loosk like you picked up a few files that are generated by cmake in your last commit. Please remove them again.

  • tests/CMakeFiles as a whole
  • tests/unittests/CMakeCache
  • tests/CMakeLists.txt.save
  • tests/tmp_peering_forward.pdf

@aashirvad08
Copy link
Copy Markdown
Contributor Author

Thanks for pointing this out.
I’ve removed the accidentally committed CMake-generated files (tests/CMakeFiles). The other files mentioned were not tracked in the repository.
Please let me know if anything else needs adjustment.

Comment thread tests/CMakeCache.txt Outdated
Comment thread CMakeLists.txt.save Outdated
Comment thread tmp_peering_forward.pdf Outdated
@aashirvad08
Copy link
Copy Markdown
Contributor Author

hey ,
Removed the accidentally committed CMakeCache.txt file.
The other mentioned files were not tracked in the repository.

@andresailer
Copy link
Copy Markdown
Member

The other mentioned files were not tracked in the repository.

Why do you think there are comments on the files in this PR to remove them, if they were not tracked in the repository?

Comment thread tests/CMakeLists.txt Outdated
Comment thread tests/CMakeLists.txt Outdated
@aashirvad08
Copy link
Copy Markdown
Contributor Author

I have removed the accidentally committed generated files.

Copy link
Copy Markdown
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks a lot for cleaning up. There are a few more minor things that could be improved (and that became more easily visible now that the "noise" has been cut down).

Comment thread tests/unittests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp
Comment thread tests/unittests/test_json_output.cpp Outdated
Comment thread tests/unittests/test_json_output.cpp Outdated
@aashirvad08
Copy link
Copy Markdown
Contributor Author

Thanks for the suggestions
I have updated tests/unittests/test_json_output.cpp accordingly.

@andresailer
Copy link
Copy Markdown
Member

Please make sure you squash the commits to remove those accidentally committed files from the history.

tests: address review comments for JSON serialization tests

tests: address review comments for JSON serialization tests

tests: remove accidentally committed CMake generated files

tests: remove accidentally committed CMake generated files

tests: simplify json dependency configuration

tests: simplify json dependency configuration

tests: address review comments for JSON serialization tests
@tmadlener tmadlener enabled auto-merge (squash) March 24, 2026 09:54
@jmcarcell
Copy link
Copy Markdown
Member

Fun fact: if you want to reduce compile times of podio or EDM4hep-related, the easiest and probably only practical thing you can do is to compile without json, everything else is the standard library and not much can be done.

@tmadlener tmadlener merged commit 38998a1 into AIDASoft:master Mar 24, 2026
33 of 35 checks passed
@aashirvad08 aashirvad08 deleted the tests/json-output branch March 24, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for JSON output

5 participants