Let the tests configure alongside Highway's - #1042
Open
yupengtang wants to merge 1 commit into
Open
yupengtang wants to merge 1 commit into
yupengtang wants to merge 1 commit into
Conversation
Highway derives its test target names from the file basename exactly as we do, and it is fetched before this block, so compress_test, dot_test and image_test were already taken and the configure step failed. Prefix ours when the name is taken; those three of our tests were never built through CMake as a result. threading_test includes gmock/gmock.h, whose headers come with the gmock target rather than with GTest::Main, so link it where it exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cmake -B build -DGEMMA_ENABLE_TESTS=ONdoes not configure ondev. Two reasons, both in the test block.1. Test target names collide with Highway's. Highway derives its test target names from the file basename exactly as we do, and it is fetched before this block runs, so three names are already taken:
The collisions are
compress_test,dot_testandimage_test. A side effect worth noting: those three of our tests were never built through CMake, since the name resolved to Highway's target.2.
threading_testincludesgmock/gmock.h, whose headers come with thegmocktarget rather than withGTest::Main:Verified on dev
Ubuntu 22.04, GCC 11.4, CMake 3.22, Intel i7-1355U, at the current
devplus this change:gemma_dot_testbuilds and links, then aborts on an accuracy expectation. I left it alone, since I cannot tell whether the bound is stale or whether this machine is genuinely outside it:Happy to open a separate issue for that if it is worth tracking.
Note on the earlier version of this PR
This started against
main, where the same run also needed aGTest::Mainalias, a C++17 fix inops/dot_test.cc, and hit a link error forhwy::Statsbecause the pinned Highway did not compilehwy/stats.cc. None of those apply here:devalready usesfind_package(GTest REQUIRED), sets C++20, and pins a newer Highway. So this is down to the two hunks above. TheWORKING_DIRECTORYyou already added togtest_discover_testsalso covers the relative-path issue I would have raised forimage_test.I used AI assistance (Claude Code) while working through this, mainly to bisect the configure and link failures and to draft this description. I ran every build and test listed above myself and reviewed each line of the change.
This replaces #1038, which had picked up too many commits for the CLA scanner after being retargeted from
main. Same change, one commit on top of the currentdev.