Remove the googletest include dir from every module instead of scoping it - #268
luapmartin wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team 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 CMake module setup no longer retrieves or adds the GoogleTest source include path to module targets. Merge Risk: ⚪ Minimal · up to GoogleTest headers are no longer injected into every module, while LogRemover date parsing remains directly testable through its public interface. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes remove the unconditional GoogleTest include directory, but they do not meet the linked issue's stated requirement to retain that include directory for muse_global under MUSE_ENABLE_UNIT_TESTS. Instead, they make LogRemover::parseDate public and remove FRIEND_TEST and gtest_prod.h. Resolution Either add the GoogleTest include directory to muse_global under MUSE_ENABLE_UNIT_TESTS while preserving the existing FRIEND_TEST arrangement, or update issue [
✨ 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 |
|
/build |
|
Build dispatched: https://github.com/musescore/muse_framework/actions/runs/33692751686 |
…g it The only reason a production header needed it was logremover.h's FRIEND_TEST(Global_LogRemoverTests, ParseDate). parseDate is a pure, stateless parser, so making it public removes the need for FRIEND_TEST / gtest_prod.h entirely, which removes the last reason any module needs the gtest include dir at build time. Test targets are unaffected: gtest.cmake already adds the include dir to test executables.
a40d908 to
b572fde
Compare
Resolves: audacity/audacity#11897
muse_create_module/declare_moduleadd the googletest include directory to every module unconditionally, so any production TU can include gtest headers — in the Audacity build that was 407 non-test TUs across all modules.The only production code that needed it was
logremover.h'sFRIEND_TEST(Global_LogRemoverTests, ParseDate).parseDateis a pure, stateless parser, so making itpublicremoves the need forFRIEND_TEST/gtest_prod.hentirely — no test-only friend declaration, no conditional include. That removes the last reason any module needs the gtest include dir at build time, so the blanket injection in module setup is simply deleted, with nothing added back. Test targets are unaffected:gtest.cmakealready adds the include dir to test executables.Verified against both the Audacity and MuseScore builds: after the fix, no non-test TU in either sees the gtest include dir (only googletest's own targets do);
muse_global_testsbuilds and passes, includingGlobal_LogRemoverTests.ParseDate.Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64