Skip to content

Fix test build: stale TestFile.h includes after #550 + hardcoded g++ in test CMakeLists#551

Merged
KenVanHoeylandt merged 1 commit into
TactilityProject:mainfrom
Crazypedia:fix-test-build-after-tactilitycore-removal
Jul 5, 2026
Merged

Fix test build: stale TestFile.h includes after #550 + hardcoded g++ in test CMakeLists#551
KenVanHoeylandt merged 1 commit into
TactilityProject:mainfrom
Crazypedia:fix-test-build-after-tactilitycore-removal

Conversation

@Crazypedia

@Crazypedia Crazypedia commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

What this fixes

Two small, independent issues in the test tree, bundled because both only affect Tests/:

1. Test build is broken on main after #550 (TactilityCore removal)

#550 moved TestFile.h from Tests/TactilityCore/Source/ to Tests/Tactility/Source/, but two test sources still include it from the old path:

  • Tests/Tactility/Source/AppManifestParsingTest.cpp
  • Tests/Tactility/Source/PropertiesFileTest.cpp
fatal error: ../../TactilityCore/Source/TestFile.h: No such file or directory

Both 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 toolchain

Four test CMakeLists (Tests/Tactility, Tests/TactilityKernel, Tests/TactilityFreeRtos, Tests/crypt-module) hardcode set(CMAKE_CXX_COMPILER g++), silently discarding whatever -DCMAKE_CXX_COMPILER=... the build was configured with. (Setting CMAKE_CXX_COMPILER after project() 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++23 static operator():

Tactility/MessageQueue.h:30:21: error: 'static void ... operator()(QueueHandle_t)'
    must be a non-static member function

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-tests compiles clean (previously failed on both issues above)
  • ctest: all 4 suites pass (TactilityFreeRtosTests, TactilityKernelTests, TactilityTests, CryptModuleTests), re-run 3× to confirm stability

Notes for review

  • Diff is intentionally minimal: 2 include-path fixes + 4 deleted set() lines, nothing else.
  • Tests/crypt-module/CMakeLists.txt is 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

    • Removed hard-coded compiler settings from multiple test projects so builds now follow the active toolchain configuration.
    • Updated test includes to use a cleaner, direct header path.
  • Tests

    • Kept test behavior unchanged while improving build portability and include handling across test suites.

…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>
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1caef59d-97dd-439b-807e-5a8395ffada2

📥 Commits

Reviewing files that changed from the base of the PR and between 66a4eb6 and 6b5698a.

📒 Files selected for processing (6)
  • Tests/Tactility/CMakeLists.txt
  • Tests/Tactility/Source/AppManifestParsingTest.cpp
  • Tests/Tactility/Source/PropertiesFileTest.cpp
  • Tests/TactilityFreeRtos/CMakeLists.txt
  • Tests/TactilityKernel/CMakeLists.txt
  • Tests/crypt-module/CMakeLists.txt
💤 Files with no reviewable changes (4)
  • Tests/TactilityKernel/CMakeLists.txt
  • Tests/TactilityFreeRtos/CMakeLists.txt
  • Tests/crypt-module/CMakeLists.txt
  • Tests/Tactility/CMakeLists.txt

📝 Walkthrough

Walkthrough

This change removes explicit set(CMAKE_CXX_COMPILER g++) directives from four CMakeLists.txt files under Tests/Tactility, Tests/TactilityFreeRtos, Tests/TactilityKernel, and Tests/crypt-module, allowing compiler selection to defer to the broader CMake/toolchain configuration. Additionally, two test source files (AppManifestParsingTest.cpp and PropertiesFileTest.cpp) update their include of TestFile.h from a relative TactilityCore-based path to a local include path. No test logic, assertions, or public entity declarations were altered.

Changes

Cohort / File(s) Summary
CMake compiler override removal (Tests/Tactility, Tests/TactilityFreeRtos, Tests/TactilityKernel, Tests/crypt-module) Removed set(CMAKE_CXX_COMPILER g++) directives
Test include path fixes (AppManifestParsingTest.cpp, PropertiesFileTest.cpp) Changed TestFile.h include path to local include

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:
A rabbit hopped through CMake's maze,
Removed the g++ from its ways,
Then fixed a path for TestFile's call,
No logic changed, no tests to fall,
A tidy hop, a clean rebase.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main fixes: stale TestFile.h includes and hardcoded g++ overrides in test CMakeLists.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@KenVanHoeylandt

Copy link
Copy Markdown
Contributor

Thanks!

@KenVanHoeylandt KenVanHoeylandt merged commit cca7224 into TactilityProject:main Jul 5, 2026
1 check passed
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.

2 participants