Skip to content
This repository was archived by the owner on Feb 12, 2022. It is now read-only.

Adds if check around calling catkin_add_gmock #43

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

Adds if check around calling catkin_add_gmock #43

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be34fa5
Adds if check around calling catkin_add_gmock
nburek Aug 22, 2019
9d3c222
fixup
nburek Aug 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion aws_common/cmake/DefineTestMacros.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ macro(add_common_gtest target)
if(catkin_FOUND)
message(STATUS "Building tests using catkin")
set(GTEST_LIBRARIES "") # hack so that linking against libgmock doesn't also link against libgtest
catkin_add_gmock("${target}" ${ARGN})
if(CATKIN_ENABLE_TESTING)
Copy link
Contributor

Choose a reason for hiding this comment

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

So @AAlon was concerned about adding this because #39 loosened the condition to fix some build break.

Looking back at our previous discussions, I do believe we should be using CATKIN_ENABLE_TESTING somewhere. catkin_FOUND is too loose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be using CATKIN_ENABLE_TESTING only when DefineTestMacros is being called from a package being built with catkin. Right now the way that we're trying to adhoc import catkin/ament seems kind of hacky. We may need to rehtink how we're building these packages

catkin_add_gmock("${target}" ${ARGN})
endif()
elseif(ament_cmake_gtest_FOUND)
message(STATUS "Building tests using ament")
ament_add_gmock("${target}" ${ARGN})
Expand Down