Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves test coverage for Phlex core components and modernizes the build system by using cet_make_exec() for the main executable. The changes enhance test coverage for identifier operations, data cell index methods, algorithm name handling, consumer functionality, and internal validation functions, while ensuring the build system follows cetmodules conventions.
Changes:
- Switch from
add_executable()tocet_make_exec()for the phlex executable and update all test references to use the namespacedphlex::phlexalias - Add comprehensive test coverage for
data_cell_indexmethods including comparisons, string formatting, parent lookup, and layer path - Add new test file
core_misc_test.cppcoveringalgorithm_name,consumer,verify_name, andadd_to_error_messagesfunctionality - Add test coverage for identifier comparison operators
- Add
fmt::formatterspecialization foridentifiertype to support formatting
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| phlex/app/CMakeLists.txt | Modernizes build by using cet_make_exec() instead of add_executable(), which creates the phlex::phlex alias and handles installation automatically |
| test/python/CMakeLists.txt | Updates test commands to use phlex::phlex alias instead of bare phlex |
| test/plugins/CMakeLists.txt | Updates TEST_EXEC to use phlex::phlex alias |
| test/mock-workflow/CMakeLists.txt | Updates TEST_EXEC to use phlex::phlex alias |
| test/max-parallelism/CMakeLists.txt | Updates TEST_EXEC to use phlex::phlex alias for all parallelism tests |
| test/form/CMakeLists.txt | Updates TEST_EXEC to use phlex::phlex alias |
| test/benchmarks/CMakeLists.txt | Updates TEST_EXEC to use phlex::phlex alias |
| test/CMakeLists.txt | Adds new core_misc_test executable with dependencies on phlex::core and Boost::json |
| test/identifier.cpp | Adds test coverage for identifier comparison operators (==, !=, <, <=, >=) |
| test/data_cell_index.cpp | Adds comprehensive test cases for comparisons, to_string, parent lookup, layer_path, and base access |
| test/core_misc_test.cpp | New test file covering algorithm_name creation/matching, consumer construction, verify_name validation, and add_to_error_messages functionality |
| phlex/model/identifier.hpp | Adds fmt::formatter specialization for identifier type to support use with fmt::format and fmt::print |
| identifier id2("def"); | ||
| assert(id1 == id1); | ||
| assert(id1 != id2); | ||
| assert(id1 < id2 || id2 < id1); |
There was a problem hiding this comment.
The assertion id1 < id2 || id2 < id1 is a tautology that will always be true for any two distinct identifiers, since we already established they're not equal on line 73. This doesn't meaningfully test the less-than operator. Consider replacing with assert(id1 < id2); to specifically verify the comparison relationship between "abc" and "def", or split it into two separate assertions to test both directions with known values.
| assert(id1 < id2 || id2 < id1); | |
| assert(id1 < id2); |
There was a problem hiding this comment.
Not if the operators are defined incorrectly.
- Added `test/core_misc_test.cpp` to cover previously untested code in core library. - Improved `test/data_cell_index.cpp` and `test/identifier.cpp` for edge case coverage. - Added `fmt::formatter` specialization for `phlex::experimental::identifier`. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
a2248f7 to
6b2fae7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 80.39% 81.43% +1.03%
==========================================
Files 127 127
Lines 3102 3103 +1
Branches 547 547
==========================================
+ Hits 2494 2527 +33
+ Misses 381 355 -26
+ Partials 227 221 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
* Improve test coverage of Phlex code - Added `test/core_misc_test.cpp` to cover previously untested code in core library. - Improved `test/data_cell_index.cpp` and `test/identifier.cpp` for edge case coverage. - Added `fmt::formatter` specialization for `phlex::experimental::identifier`. Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Address #335 (comment) * Address @beojan review comments - Per https://github.com/Framework-R-D/phlex/pull/339/files/0e216d76897394a4726972f5b1c479016f5b061b --------- Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
Factored out of #329.