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

Show file tree
Hide file tree
Changes from all commits
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
71 changes: 37 additions & 34 deletions aws_common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
cmake_minimum_required(VERSION 3.0.2)
project(aws_common)

include(${CMAKE_SOURCE_DIR}/cmake/DefineTestMacros.cmake)

SET(AWS_COMMON_VERSION 1.0.0)

add_compile_options(-std=c++11)
Expand Down Expand Up @@ -61,41 +63,42 @@ target_include_directories(${PROJECT_NAME} PUBLIC
#############
### Tests ###
#############
enable_testing()
include(${CMAKE_SOURCE_DIR}/cmake/DefineTestMacros.cmake)
find_common_test_packages()
add_common_gtest(test_aws_log_system test/sdk_utils/logging/aws_log_system_test.cpp)
add_common_gtest(test_throttling_manager test/sdk_utils/throttling_manager_test.cpp)
add_common_gtest(test_client_configuration_provider test/sdk_utils/client_configuration_provider_test.cpp)
add_common_gtest(test_service_credentials_provider test/sdk_utils/auth/service_credentials_provider_test.cpp)

add_library(test_utils test/sdk_utils/parameter_reader_mock.cpp)
target_include_directories(test_utils PUBLIC ${PROJECT_SOURCE_DIR}/test/include)

set(LIBS_FOR_TESTS
${PROJECT_NAME}
test_utils
${aws_common_LIBRARIES}
${GMOCK_LIBRARY}
pthread
)

set(HEADERS_FOR_TESTS
${aws_common_INCLUDE_DIRS}
${AWSSDK_INCLUDE_DIR}
)

macro(link_test_target target_name)
if(TARGET ${target_name})
target_include_directories("${target_name}" PRIVATE ${HEADERS_FOR_TESTS})
target_link_libraries("${target_name}" ${LIBS_FOR_TESTS})
endif()
endmacro()

link_test_target(test_aws_log_system)
link_test_target(test_throttling_manager)
link_test_target(test_client_configuration_provider)
link_test_target(test_service_credentials_provider)
if (aws_common_TESTING_ENABLED)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good idea.

In the CMakeLists.txt of our ROS packages, they are guarded CATKIN_ENABLE_TESTING.

It makes sense for our ROS-less packages to be guarded by a similar variable (that's derived from catkin and ament testing variables).

In the same vein, our ROS2 packages should be made to do something similar as well.

enable_testing()
add_common_gtest(test_aws_log_system test/sdk_utils/logging/aws_log_system_test.cpp)
add_common_gtest(test_throttling_manager test/sdk_utils/throttling_manager_test.cpp)
add_common_gtest(test_client_configuration_provider test/sdk_utils/client_configuration_provider_test.cpp)
add_common_gtest(test_service_credentials_provider test/sdk_utils/auth/service_credentials_provider_test.cpp)

add_library(test_utils test/sdk_utils/parameter_reader_mock.cpp)
target_include_directories(test_utils PUBLIC ${PROJECT_SOURCE_DIR}/test/include)

set(LIBS_FOR_TESTS
${PROJECT_NAME}
test_utils
${aws_common_LIBRARIES}
${GMOCK_LIBRARY}
pthread
)

set(HEADERS_FOR_TESTS
${aws_common_INCLUDE_DIRS}
${AWSSDK_INCLUDE_DIR}
)

macro(link_test_target target_name)
if(TARGET ${target_name})
target_include_directories("${target_name}" PRIVATE ${HEADERS_FOR_TESTS})
target_link_libraries("${target_name}" ${LIBS_FOR_TESTS})
endif()
endmacro()

link_test_target(test_aws_log_system)
link_test_target(test_throttling_manager)
link_test_target(test_client_configuration_provider)
link_test_target(test_service_credentials_provider)
endif()

#############
## Install ##
Expand Down
12 changes: 11 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 @@ -27,6 +27,8 @@ macro(find_catkin)
endmacro()

macro(find_common_test_packages)
# Loads the proper testing package (either catkin or ament) depending on what's available during
# the build
find_catkin()
find_package(ament_cmake_gtest QUIET)
if(catkin_FOUND)
Expand All @@ -37,6 +39,12 @@ macro(find_common_test_packages)
message(WARNING "Could not find catkin or ament!")
endif()

if(CATKIN_ENABLE_TESTING OR BUILD_TESTING)
set(aws_common_TESTING_ENABLED 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This DefineTestMacros.cmake file is used by many packages. It's not specific to aws_common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I was trying to give the variable some sort of namespace to indicate where it comes from. Multiple packages use DefineTestMacros.cmake, but they all source it from aws_common. Open to other naming suggestions as well.

else()
set(aws_common_TESTING_ENABLED 0)
endif()

if(DEFINED GMOCK_LIBRARIES)
set(GMOCK_LIBRARY ${GMOCK_LIBRARIES})
else()
Expand All @@ -48,7 +56,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