Add support for GTest based unit tests. #485

EricWF · 2017-11-25T21:38:51Z

As Dominic and I have previously discussed, there is some
need/desire to improve the testing situation in Google Benchmark.

One step to fixing this problem is to make it easier to write
unit tests by adding support for GTest, which is what this patch does.

By default it looks for an installed version of GTest. However the
user can specify -DBENCHMARK_BUILD_EXTERNAL_GTEST=ON to instead
download, build, and use copy of gtest from source. This is
quite useful when Benchmark is being built in non-standard configurations,
such as against libc++ or in 32 bit mode.

As Dominic and I have previously discussed, there is some need/desire to improve the testing situation in Google Benchmark. One step to fixing this problem is to make it easier to write unit tests by adding support for GTest, which is what this patch does. By default it looks for an installed version of GTest. However the user can specify -DBENCHMARK_BUILD_EXTERNAL_GTEST=ON to instead download, build, and use copy of gtest from source. This is quite useful when Benchmark is being built in non-standard configurations, such as against libc++ or in 32 bit mode.

AppVeyorBot · 2017-11-25T21:40:45Z

❌ Build benchmark 848 failed (commit 6cd3b8eda5 by @EricWF)

LebedevRI · 2017-11-25T21:45:31Z

cmake/HandleGTest.cmake

+  add_library(gtest UNKNOWN IMPORTED)
+  add_library(gtest_main UNKNOWN IMPORTED)
+  set_target_properties(gtest PROPERTIES
+    IMPORTED_LOCATION ${install_dir}/lib/libgtest.a


+ INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ${CMAKE_BINARY_DIR}/googletest/include

LebedevRI · 2017-11-25T21:45:37Z

cmake/HandleGTest.cmake

+    IMPORTED_LOCATION ${install_dir}/lib/libgtest.a
+  )
+  set_target_properties(gtest_main PROPERTIES
+    IMPORTED_LOCATION ${install_dir}/lib/libgtest_main.a


+ INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ${CMAKE_BINARY_DIR}/googletest/include

LebedevRI · 2017-11-25T21:46:20Z

cmake/HandleGTest.cmake

+  )
+  add_dependencies(gtest googletest)
+  add_dependencies(gtest_main googletest)
+  set(GTEST_INCLUDE_DIRS ${CMAKE_BINARY_DIR}/googletest/include)


- set(GTEST_INCLUDE_DIRS ${CMAKE_BINARY_DIR}/googletest/include)

LebedevRI · 2017-11-25T21:46:54Z

test/CMakeLists.txt

+
+if (BENCHMARK_ENABLE_GTEST_TESTS)
+  include(HandleGTest)
+  include_directories(SYSTEM ${GTEST_INCLUDE_DIRS})


- include_directories(SYSTEM ${GTEST_INCLUDE_DIRS})

This line still needs to stay for when out-of-tree GTest installations are used. However the SYSTEM can be removed, since it seems to be implicitly specified after adding the INTERFACE_SYSTEM_INCLUDE_DIRECTORIES line.

Nevermind... You're right.

LebedevRI · 2017-11-25T21:47:35Z

test/CMakeLists.txt

+      add_dependencies(${name} googletest)
+    endif()
+    target_link_libraries(${name} benchmark
+        ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT})


so now ${GTEST_BOTH_LIBRARIES} will properly add gtest include dirs.

LebedevRI

${} when not wrapped into "" will likely break when spaces are present.

AppVeyorBot · 2017-11-25T21:50:30Z

❌ Build benchmark 849 failed (commit 9d0bacadd0 by @EricWF)

AppVeyorBot · 2017-11-25T22:22:13Z

❌ Build benchmark 850 failed (commit 497ff83244 by @EricWF)

…libraries

AppVeyorBot · 2017-11-25T22:34:45Z

❌ Build benchmark 851 failed (commit dbdcb0b7c4 by @EricWF)

…build.

coveralls · 2017-11-25T22:37:39Z

Coverage decreased (-0.06%) to 86.799% when pulling f50c919 on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-25T22:40:57Z

❌ Build benchmark 852 failed (commit f8f8985666 by @EricWF)

coveralls · 2017-11-25T22:45:07Z

Coverage decreased (-0.06%) to 86.799% when pulling 0d971fd on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-25T22:56:13Z

❌ Build benchmark 853 failed (commit 647665d819 by @EricWF)

coveralls · 2017-11-25T23:16:30Z

Coverage decreased (-0.06%) to 86.799% when pulling 1cafd58 on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-25T23:17:34Z

❌ Build benchmark 854 failed (commit 0c90eb3b8b by @EricWF)

AppVeyorBot · 2017-11-25T23:22:02Z

❌ Build benchmark 855 failed (commit 7813239f2e by @EricWF)

coveralls · 2017-11-25T23:26:01Z

Coverage decreased (-0.06%) to 86.799% when pulling 1cafd58 on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-25T23:50:23Z

❌ Build benchmark 857 failed (commit decddf1fe5 by @EricWF)

coveralls · 2017-11-25T23:50:37Z

Coverage decreased (-0.06%) to 86.799% when pulling cc5ce7e on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-26T01:01:26Z

✅ Build benchmark 858 completed (commit c32affa913 by @EricWF)

coveralls · 2017-11-26T01:01:56Z

Coverage decreased (-0.06%) to 86.799% when pulling 18bf4e1 on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-26T01:13:15Z

❌ Build benchmark 859 failed (commit 8b1aed6a5b by @EricWF)

LebedevRI · 2017-11-26T07:23:57Z

CMakeLists.txt


+# This option is useful when building Benchmark in odd configurations, such as 32bit mode
+# or against libc++.
+option(BENCHMARK_BUILD_GTEST_INTREE "Download and build a custom copy of GTest to use" ON)


This should be two options:

build gtest

if build gtest, is downloading allowed? this really really should default to false.
build systems downloading stuff from internet is such a disgrace :/

SGTM. Thanks for the feedback.

LebedevRI · 2017-11-26T07:25:28Z

README.md

+```bash
+$ git clone https://github.com/google/benchmark.git
+# Benchmark requires GTest as a dependency. Add the source tree as a subdirectory.
+$ git clone https://github.com/google/googletest.git benchmark/googletest


I'd highly recommend

providing an option specifying the path to the gtest sources

defaulting to /usr/src/googletest/, since that is the natural location, and is where debian puts it.

First, I don't really care to allow users to specify random/arbirtrary out-of-tree GTest source directories. We already have a mechanism to allow users to specify arbitrary GTest installation using CMAKE_MODULE_PATH to influence find_package(GTest).

I've changed the build, as you've suggested, to only download dependencies if -DBENCHMARK_DOWNLOAD_DEPENDENCIES=ON is specified. So GTest installations among /usr/ and /usr/local/ should work by default.

why not git submodules?

LebedevRI · 2017-11-26T07:30:59Z

cmake/HandleGTest.cmake

+      PREFIX "${CMAKE_BINARY_DIR}/googletest"
+      INSTALL_DIR "${CMAKE_BINARY_DIR}/googletest"
+      CMAKE_CACHE_ARGS
+        -DCMAKE_BUILD_TYPE:STRING=${GTEST_BUILD_TYPE}


Since all this is just to propagate the current params, maybe it would be simpler to just use 2-step approach?
Here is how it looks in practice: [0] [1]

I've looked into 2 step approaches, but I think that they're less idiomatic CMake.
If CMake meant for you to download and load subdirectories during configuration time, they would have provided an actually supported way to do so.

LebedevRI

Some more thoughts since the small project i maintain has already had gone through the similar gtest integration.

AppVeyorBot · 2017-11-26T08:52:50Z

❌ Build benchmark 860 failed (commit eb4cebbe3a by @EricWF)

coveralls · 2017-11-26T09:11:44Z

Coverage decreased (-0.06%) to 86.799% when pulling d4ba15d on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-26T10:12:02Z

✅ Build benchmark 861 completed (commit 670f5304d8 by @EricWF)

coveralls · 2017-11-26T10:31:41Z

Coverage decreased (-0.04%) to 86.815% when pulling 6a209b9 on efcs:add-googletest into 27e0b43 on google:master.

This reverts commit 0806e80.

coveralls · 2017-11-26T10:59:48Z

Coverage decreased (-0.06%) to 86.799% when pulling 969426f on efcs:add-googletest into 27e0b43 on google:master.

AppVeyorBot · 2017-11-26T11:32:47Z

✅ Build benchmark 862 completed (commit ec5d73f757 by @EricWF)

AppVeyorBot · 2017-11-26T13:38:09Z

✅ Build benchmark 865 completed (commit e1d1cd3986 by @EricWF)

LebedevRI

As the first step looks great!

dmah42 · 2017-11-27T17:25:30Z

README.md

+* Otherwise, if `-DBENCHMARK_DOWNLOAD_DEPENDENCIES=ON` is specified during
+  configuration, the library will automatically download and build any required
+  dependencies.
+* Otherwise, if nothing is done, CMake will use `find_package(GTest REQUIRED)`


this should be the default, no? ie, why do we have it required to have it in-tree instead of just letting cmake do the right thing?

/usr/share/doc/googletest/README.Debian:

Use of precompiled libgtest Not Recommended ------------------------------------------- The Google C++ Testing Framework uses conditional compilation for some things. Because of the C++ "One Definition Rule", gtest and gmock must be compiled with exactly the same flags as your C++ code under test. Because this is hard to manage, upstream no longer recommends using precompiled libraries [1]. Using GTest with your project ----------------------------- See the upstream README for instructions on using gtest with your project. The sources for libgtest are installed into /usr/src/googletest/googletest along with CMakeLists.txt for use with cmake. If your build system uses CMake, the ExternalProject command can be used to build gtest, then FindGTest can be used to find the built library. Using gmock with your project ----------------------------- See the upstream README for instructions on using gmock with your project. The sources for libgmock are installed into /usr/src/googletest/googlemock along with CMakeLists.txt for use with cmake. With this Debian package something like the following should be enough to build a static library (which also includes gtest): g++ -I/usr/src/googletest/googlemock -c /usr/src/googletest/googlemock/src/gmock-all.cc g++ -I/usr/src/googletest/googletest -c /usr/src/googletest/googletest/src/gtest-all.cc ar -rv libgmock.a gmock-all.o gtest-all.o [1] http://groups.google.com/group/googletestframework/browse_thread/thread/668eff1cebf5309d -- Steve M. Robbins <[email protected]>, Sat, 19 Nov 2016 21:58:04 -0600

coveralls · 2017-12-14T00:25:40Z

Coverage remained the same at 86.926% when pulling fb279be on efcs:add-googletest into de725e5 on google:master.

* Add support for GTest based unit tests. As Dominic and I have previously discussed, there is some need/desire to improve the testing situation in Google Benchmark. One step to fixing this problem is to make it easier to write unit tests by adding support for GTest, which is what this patch does. By default it looks for an installed version of GTest. However the user can specify -DBENCHMARK_BUILD_EXTERNAL_GTEST=ON to instead download, build, and use copy of gtest from source. This is quite useful when Benchmark is being built in non-standard configurations, such as against libc++ or in 32 bit mode.

googlebot added the cla: yes label Nov 25, 2017

Change Appveyor to build custom GTest

37d83a9

LebedevRI reviewed Nov 25, 2017

View reviewed changes

EricWF added 2 commits November 25, 2017 15:04

Allow the GTest targets to implicitly specify their include directory

335d67b

Fix Travis setup to build with external GTest

289889d

Add explicit include_directories. It's not being propagated from the …

f50c919

…libraries

Propagate CMAKE_C_COMPILER and CMAKE_CXX_COMPILER to the Google Test …

0d971fd

…build.

Attempt to correctly name gtest library on Windows

aad55e9

EricWF added 2 commits November 25, 2017 16:12

Fix static library suffix in Debug mode

3de932e

Add debug print outs to see why cxxabi.h isn't being installed

1cafd58

Force use of shared CRT on Windows

7ed4a95

Remove debug print-outs from .travis-libcxx-setup.sh

cc5ce7e

Correct type of CMake option gtest_force_shared_crt

914bfb1

Document the GTest dependency and how to solve it

18bf4e1

LebedevRI reviewed Nov 26, 2017

View reviewed changes

Address review comments

4660646

Fix CI builders after recent change

d4ba15d

EricWF added 2 commits November 26, 2017 03:21

Clarify documentation as suggested by LebedevRI

6a209b9

Only do shallow clone of GTest

0806e80

Revert "Only do shallow clone of GTest"

969426f

This reverts commit 0806e80.

LebedevRI approved these changes Nov 27, 2017

View reviewed changes

dmah42 reviewed Nov 27, 2017

View reviewed changes

Merge with upstream

fb279be

EricWF merged commit 7db02be into google:master Dec 13, 2017

EricWF deleted the add-googletest branch December 13, 2017 23:27

Add support for GTest based unit tests. #485

Add support for GTest based unit tests. #485

Uh oh!

Conversation

EricWF commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

LebedevRI Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LebedevRI Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

LebedevRI left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

coveralls commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

coveralls commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

coveralls commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

coveralls commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 25, 2017

Uh oh!

coveralls commented Nov 25, 2017

Uh oh!

AppVeyorBot commented Nov 26, 2017

Uh oh!

coveralls commented Nov 26, 2017

Uh oh!

AppVeyorBot commented Nov 26, 2017

Uh oh!

LebedevRI Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

LebedevRI Nov 25, 2017 •

edited

Loading

LebedevRI Nov 25, 2017 •

edited

Loading

LebedevRI Nov 26, 2017 •

edited

Loading