Conversation
gemmeren
left a comment
There was a problem hiding this comment.
Thanks @greenc-FNAL , this looks good to me.
There was a problem hiding this comment.
Pull request overview
This PR improves FORM library test coverage by adding a new Catch2 unit test covering core FORM storage/config/persistence primitives, and it fixes potential ODR violations by making header-defined factory functions inline.
Changes:
- Added
test/form/form_basics_test.cppwith unit tests coveringToken, storage containers/files, factories, storage, persistence, and config helpers. - Registered the new test target in
test/form/CMakeLists.txt. - Marked factory functions in
form/util/factories.hppasinlineto prevent ODR issues across translation units.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/form/form_basics_test.cpp | Adds baseline unit coverage for FORM primitives and basic storage/persistence flows. |
| test/form/CMakeLists.txt | Adds a new Catch2-based test target and include path needed to compile it. |
| form/util/factories.hpp | Adds inline to header-defined factory functions to avoid ODR violations. |
| Token t("file.root", "container", 1, 42); | ||
| CHECK(t.fileName() == "file.root"); | ||
| CHECK(t.containerName() == "container"); | ||
| CHECK(t.technology() == 1); |
There was a problem hiding this comment.
In this codebase, technology values appear to be encoded using form::technology constants (e.g. ROOT_TTREE = major*256+minor). Using the literal 1 here is not a valid technology constant (it would yield GetMajor(1)==0) and may confuse future readers of the test. Consider using a real constant like form::technology::ROOT_TTREE (or another explicitly-valid tech value) for the Token construction.
| Token t("file.root", "container", 1, 42); | |
| CHECK(t.fileName() == "file.root"); | |
| CHECK(t.containerName() == "container"); | |
| CHECK(t.technology() == 1); | |
| Token t("file.root", "container", form::technology::ROOT_TTREE, 42); | |
| CHECK(t.fileName() == "file.root"); | |
| CHECK(t.containerName() == "container"); | |
| CHECK(t.technology() == form::technology::ROOT_TTREE); |
| c.setFile(f); | ||
|
|
||
| c.setupWrite(typeid(int)); | ||
| c.fill(nullptr); |
There was a problem hiding this comment.
Storage_Container::fill is exercised with nullptr here. Even though the current stub implementation ignores the pointer, this test can unintentionally codify that passing null is acceptable, and could become a crash if fill() later dereferences the input. Prefer passing a valid object pointer (e.g. an int on the stack) so the test remains correct if fill() gains real behavior.
| c.fill(nullptr); | |
| int value = 0; | |
| c.fill(&value); |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 80.39% 81.23% +0.83%
==========================================
Files 127 127
Lines 3102 3102
Branches 547 547
==========================================
+ Hits 2494 2520 +26
+ Misses 381 360 -21
+ Partials 227 222 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
- Added new unit tests for 'form' - Added `test/form_basics_test.cpp` to cover previously untested code in form libraries. - Fixed possible ODR violations in `form/util/factories.hpp` by adding `inline`. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
33fefb4 to
f54d1a4
Compare
* Improve test coverage of FORM code - Added new unit tests for 'form' - Added `test/form_basics_test.cpp` to cover previously untested code in form libraries. - Fixed possible ODR violations in `form/util/factories.hpp` by adding `inline`. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Address #333 (review) --------- Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Factored out of #329.
Added new unit tests for 'form'
Added
test/form_basics_test.cppto cover previously untested code inform libraries.
Fixed possible ODR violations in
form/util/factories.hppby addinginline.Co-authored-by: greenc-FNAL 2372949+greenc-FNAL@users.noreply.github.com