feat: cmake option to skip building mock libraries #13673

dbolduc · 2024-02-27T19:06:08Z

Motivated by #13621

Consumers of google-cloud-cpp who want to test their code can use this flag to install mocks, without needing to build all of our unit tests, internal testing infrastructure.

Background

We create mocking libraries (which are all header only) and install them by default (even when BUILD_TESTING=OFF). For many months, we only did this in Pub/Sub and GCS+gRPC, but in the latest release, we started installing mocks for all GAPICs. (And in this release, for all libraries except compute).

These libraries link to GoogleTest, which can be hard to find. This can break some (atypical?) builds. (See the linked issue, which uses FetchContent().

Things to Scrutinize

The option controls whether or not we install the mock libraries. It could control whether or not we define mock targets at all. Or whether we install headers.
- naming follow up: it could be called GOOGLE_CLOUD_CPP_INSTALL_MOCKS.
We are not exercising the option set to OFF in our CI.

This change is

codecov · 2024-02-27T19:33:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.28%. Comparing base (b8df092) to head (db7582d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13673      +/-   ##
==========================================
- Coverage   93.28%   93.28%   -0.01%     
==========================================
  Files        2228     2228              
  Lines      193491   193491              
==========================================
- Hits       180501   180497       -4     
- Misses      12990    12994       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anthonyalayo · 2024-02-27T19:47:19Z

I'm not a maintainer, but this is looking reasonable to me.

coryan · 2024-02-27T19:54:29Z

cmake/GoogleCloudCppCommon.cmake

@@ -164,6 +164,17 @@ endfunction ()
 function (google_cloud_cpp_install_mocks library display_name)
    set(library_target "google_cloud_cpp_${library}")
    set(mocks_target "google_cloud_cpp_${library}_mocks")
+
+    # Always install the mock headers. These are harmless, in the sense that
+    # they will not cause build failures in environments without GoogleTest. The


If GMock is not installed, as soon as you #include one of them the build will stop working 🤷

~~I assumed they were stubs from the comment, but if they're not, that would be problematic. I'm assuming other build issues creeped up when this wasn't done?~~

Ignore the above, I believe @coryan's comment was from the library's perspective and not the user of the library's perspective?

coryan · 2024-02-29T14:04:47Z

cmake/GoogleCloudCppLibrary.cmake

+    if (GOOGLE_CLOUD_CPP_WITH_MOCKS)
+        # Create a header-only library for the mocks. We use a CMake `INTERFACE`
+        # library for these, a regular library would not work on macOS (where
+        # the library needs at least one .o file). Unfortunately INTERFACE


No action needed: I wonder if this is still true now that we use CMake >= 3.13 ..

Ack, I don't know the answer and I don't have easy access to a mac.

coryan · 2024-02-29T14:05:02Z

cmake/GoogleCloudCppCommon.cmake

@@ -164,6 +164,7 @@ endfunction ()
 function (google_cloud_cpp_install_mocks library display_name)
    set(library_target "google_cloud_cpp_${library}")
    set(mocks_target "google_cloud_cpp_${library}_mocks")
+


coryan · 2024-02-29T14:14:34Z

CMakeLists.txt

+cmake_dependent_option(
+    GOOGLE_CLOUD_CPP_WITH_MOCKS
+    [==[Build the google-cloud-cpp mocking libraries.
+
+google-cloud-cpp offers mocking libraries with mock classes, to facilitate unit
+testing of Cloud C++ clients. Consumers of this library that do not use the
+provided mocks to test code involving the Cloud C++ clients may wish to turn
+this flag off.]==]
+    ON
+    "NOT BUILD_TESTING"
+    ON)
+mark_as_advanced(GOOGLE_CLOUD_CPP_WITH_MOCKS)


TIL, you can use NOT condition in cmake_dependent_option. Using ON ... ON looks weird. I think you want something like this:

Suggested change

cmake_dependent_option(

GOOGLE_CLOUD_CPP_WITH_MOCKS

[==[Build the google-cloud-cpp mocking libraries.

google-cloud-cpp offers mocking libraries with mock classes, to facilitate unit

testing of Cloud C++ clients. Consumers of this library that do not use the

provided mocks to test code involving the Cloud C++ clients may wish to turn

this flag off.]==]

ON

"NOT BUILD_TESTING"

ON)

mark_as_advanced(GOOGLE_CLOUD_CPP_WITH_MOCKS)

option(

GOOGLE_CLOUD_CPP_WITH_MOCKS

[==[Build the google-cloud-cpp mocking libraries.

google-cloud-cpp offers mocking libraries with mock classes, to facilitate unit

testing of Cloud C++ clients. Consumers of this library that do not use the

provided mocks to test code involving the Cloud C++ clients may wish to turn

this flag off.]==]

$<IF:$<BOOL:${BUILD_TESTING}>:OFF:ON>)

mark_as_advanced(GOOGLE_CLOUD_CPP_WITH_MOCKS)

That is, the option always exists, but its default value is based on a generator expression.

I don't think I want that.

The default value should always be ON, regardless of BUILD_TESTING. But if BUILD_TESTING is ON, then GOOGLE_CLOUD_CPP_WITH_MOCKS must be ON (because our unit tests depend on the mock libraries). That is what I am enforcing with the ON ... ON.

I think this is doing what I want. Adding a debug print statement after the option is defined:

$ cmake -DBUILD_TESTING=ON -DGOOGLE_CLOUD_CPP_WITH_MOCKS=OFF GOOGLE_CLOUD_CPP_WITH_MOCKS=ON $ cmake -DBUILD_TESTING=OFF -DGOOGLE_CLOUD_CPP_WITH_MOCKS=OFF GOOGLE_CLOUD_CPP_WITH_MOCKS=OFF

$ cmake -LAH ... // Build the google-cloud-cpp mocking libraries. google-cloud-cpp offers mocking libraries with mock classes, to facilitate unit testing of Cloud C++ clients. Consumers of this library that do not use the provided mocks to test code involving the Cloud C++ clients may wish to turn this flag off. GOOGLE_CLOUD_CPP_WITH_MOCKS:BOOL=ON

anthonyalayo · 2024-02-29T17:53:52Z

Thanks for the merge!

dbolduc added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 27, 2024

dbolduc temporarily deployed to internal February 27, 2024 19:06 — with GitHub Actions Inactive

dbolduc force-pushed the cmake-install-mocks-flag branch from e6d4247 to c5b414f Compare February 27, 2024 19:06

dbolduc temporarily deployed to internal February 27, 2024 19:07 — with GitHub Actions Inactive

dbolduc mentioned this pull request Feb 27, 2024

gmock and gtest targets are required even with BUILD_TESTING OFF #13621

Closed

coryan approved these changes Feb 27, 2024

View reviewed changes

dbolduc temporarily deployed to internal February 28, 2024 22:02 — with GitHub Actions Inactive

dbolduc removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 29, 2024

dbolduc marked this pull request as ready for review February 29, 2024 13:56

dbolduc requested a review from a team as a code owner February 29, 2024 13:56

coryan approved these changes Feb 29, 2024

View reviewed changes

dbolduc added 3 commits February 29, 2024 10:07

feat: ability to skip installing mocks

575b5fb

only define mock library targets if GOOGLE_CLOUD_CPP_WITH_MOCKS

6eae00b

undiff this file

db7582d

dbolduc force-pushed the cmake-install-mocks-flag branch from 32d7a6c to db7582d Compare February 29, 2024 15:11

dbolduc temporarily deployed to internal February 29, 2024 15:11 — with GitHub Actions Inactive

dbolduc changed the title ~~feat: ability to skip installing mock libraries~~ feat: cmake option to skip building mock libraries Feb 29, 2024

dbolduc merged commit 7a8daed into googleapis:main Feb 29, 2024
62 checks passed

dbolduc deleted the cmake-install-mocks-flag branch February 29, 2024 16:24

teo-tsirpanis mentioned this pull request Jul 9, 2024

[google-cloud-cpp] Skip building mock libraries. microsoft/vcpkg#39802

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cmake option to skip building mock libraries #13673

feat: cmake option to skip building mock libraries #13673

dbolduc commented Feb 27, 2024 •

edited by coryan

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

anthonyalayo commented Feb 27, 2024

coryan Feb 27, 2024

anthonyalayo Feb 27, 2024 •

edited

Loading

coryan Feb 29, 2024

dbolduc Feb 29, 2024

coryan Feb 29, 2024

dbolduc Feb 29, 2024

coryan Feb 29, 2024

dbolduc Feb 29, 2024

anthonyalayo commented Feb 29, 2024

feat: cmake option to skip building mock libraries #13673

feat: cmake option to skip building mock libraries #13673

Conversation

dbolduc commented Feb 27, 2024 • edited by coryan Loading

Background

Things to Scrutinize

codecov bot commented Feb 27, 2024 • edited Loading

Codecov Report

anthonyalayo commented Feb 27, 2024

coryan Feb 27, 2024

Choose a reason for hiding this comment

anthonyalayo Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

coryan Feb 29, 2024

Choose a reason for hiding this comment

dbolduc Feb 29, 2024

Choose a reason for hiding this comment

coryan Feb 29, 2024

Choose a reason for hiding this comment

dbolduc Feb 29, 2024

Choose a reason for hiding this comment

coryan Feb 29, 2024

Choose a reason for hiding this comment

dbolduc Feb 29, 2024

Choose a reason for hiding this comment

anthonyalayo commented Feb 29, 2024

dbolduc commented Feb 27, 2024 •

edited by coryan

Loading

codecov bot commented Feb 27, 2024 •

edited

Loading

anthonyalayo Feb 27, 2024 •

edited

Loading