Adds if check around calling catkin_add_gmock #43
Conversation
Adds if check around calling catkin_add_gmock in order to prevent the warning that is causing our builds to be "unstable".
Codecov Report
@@ Coverage Diff @@
## master #43 +/- ##
=======================================
Coverage 81.33% 81.33%
=======================================
Files 11 11
Lines 375 375
=======================================
Hits 305 305
Misses 70 70
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Unfortunately this would break downstream packages when CATKIN_ENABLE_TESTING is turned off.
For example here we add the test and call target_.... With this change, when CATKIN_ENABLE_TESTING=0 the target wouldn't exist and the build would break.
One option is to audit all our CEs and add guards to check that targets exist like here (or create a macro for it in the utils package). A bit ugly and repetitive though... there might be more elegant solutions.
|
I have a feeling we're not supposed to be calling |
Good callout.
I started to look at the option of creating a macro to see if testing is enabled that we could use in these ROS agnostic packages but ran into some issues. Because these builds aren't native Catkin/Ament, the CATKIN_ENABLE_TESTING and BUILD_TESTING variables are not being set by the build system, making it difficult to know if testing has been disabled or not. I'm trying to see if there is a work around for this. |
We might not need to call enable_testing directly anymore if we're actually using Catkin/Ament to initialize the testing, but I'm not actually sure that we are. I also don't think it hurts to call this as long as we actually do want testing enabled for our package (https://cmake.org/cmake/help/v3.0/command/enable_testing.html). |
|
Updated with a change that I think addresses @AAlon concern. I tested it by making the same change in the h264_encoder_core package that I made in aws_common package and it was able to build successfully when I set testing to be disabled. However, in order actually disable testing I had to run the build as: Even though this is kind of working right now, I would propose that we ditch using |
| 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) |
There was a problem hiding this comment.
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
| endif() | ||
|
|
||
| if(CATKIN_ENABLE_TESTING OR BUILD_TESTING) | ||
| set(aws_common_TESTING_ENABLED 1) |
There was a problem hiding this comment.
This DefineTestMacros.cmake file is used by many packages. It's not specific to aws_common.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
Adds if check around calling catkin_add_gmock in order to prevent the warning that is causing our builds to be "unstable".
Issue #, if available:
https://t.corp.amazon.com/P24962820
Description of changes:
Adds if check around calling catkin_add_gmock in order to prevent the warning that is causing our builds to be "unstable".
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.