Add support for GMock by v-lopez · Pull Request #896 · ros/catkin

v-lopez · 2017-10-16T13:20:55Z

I took the changes from @kyrofa, rebased them on kinetic and updated them for GMock 1.7 which no longer has an embedded gtest version.

Search for GMock (and its bundled GTest) before searching for standalone GTest. This resolves ros#699. Signed-off-by: Kyle Fazzari <kyle.fazzari@navy.mil>

dirk-thomas · 2017-10-16T16:20:56Z

In Ubuntu Xenial gtest 1.7 is being shipped without gmock (https://packages.ubuntu.com/xenial/libgtest-dev). Only as of Zesty googletest 1.8 contain both gtest and gmock (https://packages.ubuntu.com/zesty/googletest). So this can be targeted for the next Ubuntu LTS and ROS Melodic but not for ROS Kinetic and Lunar.

v-lopez · 2017-10-16T16:38:35Z

Actually the modifications I made are for the case where gtest and gmock are not bundled together, as is the case in Xenial, and should also work in the case only gtest is installed.

In Trusty they were separated as well, but gmock contained it's own version of gtest.

dirk-thomas · 2017-10-16T17:09:30Z

I see. In Xenial they are not bundled in the same Debian package anymore but as separate packages where google-mock depends on libgtest-dev. I will add some comments / questions inline.

kyrofa · 2017-10-16T17:07:40Z

cmake/test/gtest.cmake

+  # Find the gtest sources
+  find_file(_GTEST_SOURCES "gtest.cc"
+           PATHS ${src_paths}
+           NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH


The indentation here seems off by a space.

Nice catch, fixed both.

kyrofa · 2017-10-16T17:07:57Z

cmake/test/gtest.cmake

+  # Find the gmock sources
+  find_file(_GMOCK_SOURCES "gmock.cc"
+           PATHS ${src_paths}
+           NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH


Off by a space here as well.

dirk-thomas

The overall logic seems weird to me:

If gmock is found (https://github.com/ros/catkin/pull/896/files#diff-84d1585b7758a3723f688bcff3f8602cR212) it is being built. But gtest is neither being considered nor being built in this case.

dirk-thomas · 2017-10-16T17:19:58Z

cmake/test/gtest.cmake

+                               gmock_libs gmock_main_libs)
+
+      # If we found gmock, set it up to be built (which will also build gtest,
+      # since it's bundled)


The comment still mentions that gtest is bundled.

@dirk-thomas Fixed in 2ccb83b as well

dirk-thomas · 2017-10-16T17:21:01Z

cmake/test/gtest.cmake

+        set_target_properties(${gmock_libs} ${gmock_main_libs}
+                              PROPERTIES EXCLUDE_FROM_ALL 1)
+
+        message(STATUS "Found gmock sources under '${gmock_base_dir}': gtests will be built")


The message seems inconsistent. The first part mentions gmock - the second part gtest.

Fixed in 1d5e21c, when gmock is found and built, so is gtests.

dirk-thomas · 2017-10-16T17:21:41Z

cmake/test/gtest.cmake

+          message(STATUS "Found gtest sources under '${gtest_base_dir}': gtests will be built")
+        else()
+          if(CATKIN_TOPLEVEL)
+            message(STATUS "gtest not found, C++ tests can not be built. Please install the gtest headers globally in your system or checkout gtest (by running 'svn checkout http://googletest.googlecode.com/svn/tags/release-1.6.0 gtest' in the source space '${CMAKE_SOURCE_DIR}' of your workspace) to enable gtests")


Should this reference at least version 1.7 or even newer and the GitHub repo instead?

Fixed, it references 1.8 in the github repo.

dirk-thomas · 2017-10-16T17:23:06Z

cmake/test/gtest.cmake

+            return()
+          endif()
+          _install(${ARGN})
+        endfunction()


Please keep the comment why this is necessary.

Same below.

Done in 4d69841

dirk-thomas · 2017-10-16T17:24:09Z

cmake/test/gtest.cmake

-  set(GTEST_LIBRARIES ${GTEST_LIBRARIES} CACHE INTERNAL "")
-  set(GTEST_MAIN_LIBRARIES ${GTEST_MAIN_LIBRARIES} CACHE INTERNAL "")
-  set(GTEST_BOTH_LIBRARIES ${GTEST_BOTH_LIBRARIES} CACHE INTERNAL "")
+  message(STATUS "Found gmock: gtests will be built")


Inconsistent message.

Like above, fixed in 1d5e21c

dirk-thomas · 2017-10-16T17:27:34Z

cmake/test/gtest.cmake

+      set(GTEST_LIBRARY_DIRS ${GMOCK_LIBRARY_DIRS})
+      set(GTEST_LIBRARIES ${GMOCK_LIBRARIES})
+      set(GTEST_MAIN_LIBRARIES ${GMOCK_MAIN_LIBRARIES})
+      set(GTEST_BOTH_LIBRARIES ${GMOCK_BOTH_LIBRARIES})


Why are the gtest variables set to the same values as gmock here?

Same below.

GMOCK_ variables include the GTEST headers and libraries as well.

The purpose of most of the code here, is to make sure that both gmock and gtest use the same version and are compiled together if they, that's why catkin_find_gmock_source calls for catkin_find_gtest_source.

Currently catkin_find_gmock_source does not export the GTEST_ variables to the parent scope, if this is needed I can add it.

I guess the question (not yet asked / answered) is: how do we expect a user to use this?

Based on the current patch I assume that users should continue to call catkin_add_gtest / catkin_add_executable_with_gtest and are then able to use gtest as well as gmock (when available. Is that assumption correct?

I am not sure that is a good way. I would have expected that the existing functions just link against gtest (as they did before, without changing). And a new set of functions catkin_add_gmock / catkin_add_executable_with_gmock would be introduced which links against both gmock as well as gtest. A user then needs to opt-in explicitly by updating the called CMake function name to use gmock.

Yes your understanding of this patch's behavior is correct.

I understand your point, for any pair of libraries that were unrelated I would completely agree.
In this case gmock is an extension of gtest, that in theory should be harmless. So I would say that if keeping them together keeps things simple and doesn't introduce any side effects (that I've seen) it might be worth it.

I am not use if that might not confuse users that catkin_add_gtest "sometimes" works with gmock and sometimes doesn't (e.g. when they use different versions of the ROS). ROS packages might start using gmock in their tests without realizing that the same API won't work in older ROS distros.

@clalancette @dhood @mikaelarguedas @tfoote @wjwwood Maybe any of you has feedback on this?

IMO there is value in keeping behavior consistent across distros and to let users opt-in to use new features. So I'd be in favor of having new cmake functions for users that want to use gmock. Then I don't have enough knowledge to estimate the risk and could be convinced that the risk is minimal enough for allowing a documented change of behavior

Having new function/macro names sounds fine to me, I didn't realize that was an option. I thought this was a case where the new version of googletesting is just gmock and gtest isn't separate anymore. My confusion.

Ok, I will add catkin_add_gmock and catkin_add_executable_with_gmock.

Before I continue with it, there will be a lot of similarities between the gtest and gmock functions.
My idea was to make the current 2 functions' code generic in tow new "private" functions, let's say _catkin_add_google_test and _catkin_add_executable_with_google_test, and have the gtest and gmock functions call this with minimum code duplication.

Would this be your preferred approach? It would make the code a little bit more complex, the other option is to duplicate and modify the code for the gmock versions, and leave the originals as they were.

... with minimum code duplication.

@v-lopez That sounds good. Whatever avoids code duplication. Either some "private" functions which are used from the "public" ones. Or the gmock ones calling the gtest ones internally (if that is feasible).

I've made the discussed changes in 3c4c7b2

There's one thing that I don't know how to test. All my changes are focused towards the gtest & gmock from sources approach, which as far as I know, is the recommended way of using google test.

But there's a find_package(GMock QUIET) and find_package(GTest QUIET) that I don't know how should behave, because there's not a unique FindGMock.cmake file, there are many implementations with different behaviours.

dirk-thomas · 2017-10-16T17:29:30Z

Can you please describe the different use cases you have tried this patch with as well as steps to reproduce these tests.

v-lopez · 2017-10-17T10:06:34Z

For testing this I have created 2 packages that are in this repository: https://github.com/pal-robotics/catkin_gtest_tests.git
I tested them on a Xenial Kinetic docker.

Install libgtest-dev and google-mock and build everything:

mkdir -p my_ws/src
cd my_ws/src/
git clone https://github.com/pal-robotics/catkin -b gmock-kinetic
git clone https://github.com/pal-robotics/catkin_gtest_tests.git
cd ..
catkin_make tests

It should all work fine, you should be able to run the dummy tests as wells after sourcing your workspace with:

rosrun gtest_test gtest_test-test 
rosrun gmock_test gmock_test-test

Now remove google-mock, clean workspace and build again

sudo apt-get -y remove google-mock
rm -rf build devel
catkin_make tests

It should build the gtest package but fail when compiling the package that requires gmock.

Now remove gtest, I force a removal without removing it's dependencies for convenience

sudo dpkg -r --force-depends libgtest-dev
rm -rf build devel
catkin_make tests

It should now fail on a call to catkin_gtest_tests

Now add googletest from sources and build again:

git clone  https://github.com/google/googletest.git -b release-1.8.0 src/googletest
rm -rf build devel
catkin_make tests

Now everything should work again.

dirk-thomas · 2017-10-27T17:55:43Z

I started adding some changes here (using the GitHub web interface) but quickly reached a limit. Therefore I created #897 from a branch in this repo.

dirk-thomas · 2017-10-27T17:56:23Z

cmake/test/gtest.cmake

@@ -1,4 +1,5 @@
 _generate_function_if_testing_is_disabled("catkin_add_gtest")
+_generate_function_if_testing_is_disabled("catkin_add_gmock")


This doesn't work since the first invocation already calls return.

I addressed this in aa28d0c.

dirk-thomas · 2017-10-30T18:57:15Z

Closing in favor of #897.

Kyle Fazzari and others added 2 commits October 16, 2017 12:56

Add support for GMock.

8d546e0

Search for GMock (and its bundled GTest) before searching for standalone GTest. This resolves ros#699. Signed-off-by: Kyle Fazzari <kyle.fazzari@navy.mil>

GMock 1.7 no longer includes gtest

971a86d

dirk-thomas added the consider-future-release label Oct 16, 2017

dirk-thomas removed the consider-future-release label Oct 16, 2017

kyrofa reviewed Oct 16, 2017

View reviewed changes

Cosmetic changes

8b8b17d

dirk-thomas reviewed Oct 16, 2017

View reviewed changes

Victor Lopez added 4 commits October 17, 2017 07:08

Update comments to reflect gtest is no longer bundled in gmock

2ccb83b

Recover comment documenting why we skip install rules for gtest/gmock

4d69841

Fix using gtest and gmock from source space

0162766

Clarify that when gmock is built, so is gtests

1d5e21c

Victor Lopez and others added 2 commits October 27, 2017 13:47

Don't include gmock when adding a gtest only target

3c4c7b2

document public functions instead

a206db1

dirk-thomas added the enhancement label Oct 27, 2017

dirk-thomas mentioned this pull request Oct 27, 2017

add support for GMock #897

Merged

dirk-thomas reviewed Oct 27, 2017

View reviewed changes

dirk-thomas closed this Oct 30, 2017

		@@ -1,4 +1,5 @@
		_generate_function_if_testing_is_disabled("catkin_add_gtest")
		_generate_function_if_testing_is_disabled("catkin_add_gmock")

Conversation

v-lopez commented Oct 16, 2017

Uh oh!

dirk-thomas commented Oct 16, 2017

Uh oh!

v-lopez commented Oct 16, 2017

Uh oh!

dirk-thomas commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 16, 2017

Uh oh!

v-lopez commented Oct 17, 2017

Uh oh!

dirk-thomas commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants