Fix test build: stale TestFile.h includes after #550 + hardcoded g++ in test CMakeLists#551
Conversation
…in test CMakeLists PR TactilityProject#550 moved TestFile.h from Tests/TactilityCore to Tests/Tactility, but AppManifestParsingTest.cpp and PropertiesFileTest.cpp still include the old path, so the test build fails on main. The per-suite 'set(CMAKE_CXX_COMPILER g++)' lines override whatever compiler the build was configured with. On hosts where /usr/bin/g++ is older than the project's C++23 baseline (e.g. g++ 11), the tests then fail to compile (static operator() in MessageQueue.h/Semaphore.h) even though the firmware itself builds fine with the configured toolchain. Removing the override lets the tests inherit the top-level compiler as usual. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughThis change removes explicit Changes
Sequence Diagram(s)Not applicable; changes are limited to build configuration and include path adjustments with no observable runtime flow changes. Estimated code review effort: 2/5 (build config and include path changes, low complexity) Related issues: None specified Related PRs: None specified Suggested labels: build, chore, tests Suggested reviewers: None specified Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Thanks! |
What this fixes
Two small, independent issues in the test tree, bundled because both only affect
Tests/:1. Test build is broken on
mainafter #550 (TactilityCore removal)#550 moved
TestFile.hfromTests/TactilityCore/Source/toTests/Tactility/Source/, but two test sources still include it from the old path:Tests/Tactility/Source/AppManifestParsingTest.cppTests/Tactility/Source/PropertiesFileTest.cppBoth files sit in the same directory as the moved header, so the include is now just
"TestFile.h".2. Per-suite
set(CMAKE_CXX_COMPILER g++)overrides the configured toolchainFour test CMakeLists (
Tests/Tactility,Tests/TactilityKernel,Tests/TactilityFreeRtos,Tests/crypt-module) hardcodeset(CMAKE_CXX_COMPILER g++), silently discarding whatever-DCMAKE_CXX_COMPILER=...the build was configured with. (SettingCMAKE_CXX_COMPILERafterproject()is also unsupported per CMake docs.)This breaks test builds on any host where
/usr/bin/g++predates the project's C++23 baseline. Concrete example: on Ubuntu with system g++ 11, the firmware builds fine with a configured GCC 13 toolchain, but the tests are forced onto g++ 11 and fail on C++23static operator():Removing the override lets the test targets inherit the top-level compiler like every other target. CI (
ubuntu-latest) is unaffected either way since its default g++ is new enough.Verification
On this branch, configured with GCC 13 on a Ubuntu host whose system g++ is 11:
ninja build-testscompiles clean (previously failed on both issues above)ctest: all 4 suites pass (TactilityFreeRtosTests,TactilityKernelTests,TactilityTests,CryptModuleTests), re-run 3× to confirm stabilityNotes for review
set()lines, nothing else.Tests/crypt-module/CMakeLists.txtis new from Crypt module #549 and appears to have inherited the hardcoded compiler line from the older suites — worth avoiding in future test suites.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests