Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GTest improvements #563

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from
Open

GTest improvements #563

wants to merge 2 commits into from

Conversation

andy9a9
Copy link

@andy9a9 andy9a9 commented Nov 28, 2024

  • make some gmock/gtest improvements
    • port 8915ab8 to ament_cmake_gmock-extras
    • add option to use system' gtest or gmock targets from GTest
      • don't change the old building behavior

Instead of always compiling own gmock/gtestG, try to use GTest targets
from FindGTest cmake config, but without changing the old behavior.

Signed-off-by: Andrej Valek <[email protected]>
@sloretz
Copy link
Contributor

sloretz commented Dec 6, 2024

Closes #255

I don't think this PR relates to this issue. Is there another one you meant to link?

Comment on lines +71 to +78
else()
# try to find and use gmock from GTest
find_package(GTest QUIET)
if(GTest_FOUND)
set(GMOCK_FOUND TRUE)
set(GMOCK_LIBRARIES GTest::gmock)
set(GMOCK_MAIN_LIBRARIES GTest::gmock_main)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little strange to me. Mind explaining when we can assume GTest will provide targets for gmock prefixed with GTest::?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy9a9
Copy link
Author

andy9a9 commented Dec 6, 2024

Closes #255

I don't think this PR relates to this issue. Is there another one you meant to link?

Ah, I see. I was thinking, that there was a missing option to add external gmock library.

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