Fix: let the C++ unit test tree's root CMakeLists describe the tree it builds - #2421
Conversation
…t builds tests/ut/cpp/CMakeLists.txt opened by naming src/common/hierarchical as its subject, a target to build the tree with, and a directory to run ctest in. All three were wrong. That tree holds 12 of the 155 cases here. `run_ut_cpp` names no target and never did: the 1979-line version this file replaced carried the same line and no add_custom_target either, so it has pointed at nothing since it was written, and reading it as something the refactor dropped is the wrong conclusion to leave available. The build directory is tests/ut/cpp/build, which is what CI, docs/testing.md and docs/ci.md all use. The opening now states what the tree is and what this file in particular holds, and points at docs/testing/adding-a-cpp-unit-test.md for the commands rather than carrying a fourth copy of them. Writing the correct commands here would reset the drift once, and the reason these lines were wrong is that they are a copy nobody consults when the commands change. project() takes the tree's own name. Nothing consumes the old one: no PROJECT_NAME or hierarchical_ut_SOURCE_DIR reference exists anywhere, CI configures by -S/-B path and runs by --test-dir, and the tree calls enable_testing() without include(CTest), so not even a DartConfiguration.tcl is written. The name's one landing place is CMAKE_PROJECT_NAME in the cache. 229 targets built from a clean configure, 229 ctest passes, and check_ut_cpp_axis.py, check_ut_cpp_stub_linkage.py and check_ut_cpp_case_naming.py all clean.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe C++ unit-test CMake project now covers multiple test directories. It also checks for orphan cases and writes the PYUT manifest after test registration. ChangesC++ unit-test tree
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to The change expands C++ unit-test coverage and validation metadata generation, with reported successful build and test results. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each test-tree lane Comment |
2 participants
What
tests/ut/cpp/CMakeLists.txt's opening three lines and itsproject()name described the tree as it stood years ago. Four lines, raised in review on #2418 after it merged.src/common/hierarchical"cmake --build . --target run_ut_cppctest --test-dir build/ut_cpptests/ut/cpp/buildproject(hierarchical_ut CXX)The defect predates #2418, but that PR rewrote this file from 1979 lines to 191 on the theme of letting the structure say what the CMake used to, so leaving its own opening lines saying something else is a
doc-consistency.md§1 problem inside it.The commands are not re-written here
The new opening points at
docs/testing/adding-a-cpp-unit-test.mdrather than carrying a corrected recipe.docs/testing.md:539, that page's §8, anddocs/ci.md:426already hold one each. These three lines were wrong because they were a fourth copy that nobody consults when the commands change — writing correct ones in would reset the drift for one cycle and restore the mechanism.project()rename has no consumerVerified three ways, not one:
git grepfinds noPROJECT_NAME,CMAKE_PROJECT_NAMEorhierarchical_ut_SOURCE_DIRreference in the tree — every path is spelledCMAKE_SOURCE_DIR/CMAKE_CURRENT_SOURCE_DIR.-S/-Bpath and runs by--test-dir(_ut-no-hardware.yml:106-108,_ut-npu-a2a3.yml:53), never by project name.enable_testing()withoutinclude(CTest), so noDartConfiguration.tclis generated at all. The name's one landing place isCMAKE_PROJECT_NAMEin the cache.simpler_ut_cppis unused repo-wide and matches both thesimpler_ut_*helper prefix and theut-cppjob name indocs/ci.md.Test plan
Clean configure, since the globs are
CONFIGURE_DEPENDSbut an object library's source list is not re-read per build:pre-commit run --files tests/ut/cpp/CMakeLists.txtrun_ut_cpp/build/ut_cpp/hierarchical_utrepo-wideHardware-labelled cases are unaffected — the diff touches no target, source, or property.