Skip to content

Let the tests configure alongside Highway's - #1042

Open
yupengtang wants to merge 1 commit into
google:devfrom
yupengtang:cmake-tests-highway-collision
Open

yupengtang wants to merge 1 commit into
google:devfrom
yupengtang:cmake-tests-highway-collision

Conversation

@yupengtang

Copy link
Copy Markdown

cmake -B build -DGEMMA_ENABLE_TESTS=ON does not configure on dev. 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:

CMake Error at CMakeLists.txt:410 (add_executable):
  add_executable cannot create target "dot_test" because another target
  with the same name already exists.

The collisions are compress_test, dot_test and image_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_test includes gmock/gmock.h, whose headers come with the gmock target rather than with GTest::Main:

util/threading_test.cc:21:10: fatal error: gmock/gmock.h: No such file or directory

Verified on dev

Ubuntu 22.04, GCC 11.4, CMake 3.22, Intel i7-1355U, at the current dev plus this change:

test result
gemma_compress_test 6 tests pass
gemma_image_test 2 tests pass
threading_test 6 tests pass
ops_test 40 tests pass

gemma_dot_test builds 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:

!!line 751, comp2 actual 2.586005E+00 not in [1.001000E+00, 1.400000E+00]
Abort at dot_test.cc:100: Assert false:

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 a GTest::Main alias, a C++17 fix in ops/dot_test.cc, and hit a link error for hwy::Stats because the pinned Highway did not compile hwy/stats.cc. None of those apply here: dev already uses find_package(GTest REQUIRED), sets C++20, and pins a newer Highway. So this is down to the two hunks above. The WORKING_DIRECTORY you already added to gtest_discover_tests also covers the relative-path issue I would have raised for image_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 current dev.

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.
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.

1 participant