From ca4b7c9ff4d8a5c37ac51795b03ffe934958aeff Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Mon, 21 Dec 2020 15:41:17 -0500 Subject: Googletest export Give each of Naggy/Nice/StrictMock a base class whose constructor runs before the mocked class's constructor, and a destructor that runs after the mocked class's destructor, so that any mock methods run in either the constructor or destructor use the same strictness as other calls. PiperOrigin-RevId: 348511612 --- googlemock/docs/cook_book.md | 10 --- googlemock/include/gmock/gmock-nice-strict.h | 104 ++++++++++++++++--------- googlemock/include/gmock/gmock-spec-builders.h | 22 ++++-- googlemock/test/gmock-nice-strict_test.cc | 39 ++++++++++ 4 files changed, 121 insertions(+), 54 deletions(-) diff --git a/googlemock/docs/cook_book.md b/googlemock/docs/cook_book.md index cfe5858..cd64150 100644 --- a/googlemock/docs/cook_book.md +++ b/googlemock/docs/cook_book.md @@ -429,16 +429,6 @@ limitations): 2. `NiceMock` and `StrictMock` may not work correctly if the destructor of `MockFoo` is not virtual. We would like to fix this, but it requires cleaning up existing tests. -3. During the constructor or destructor of `MockFoo`, the mock object is *not* - nice or strict. This may cause surprises if the constructor or destructor - calls a mock method on `this` object. (This behavior, however, is consistent - with C++'s general rule: if a constructor or destructor calls a virtual - method of `this` object, that method is treated as non-virtual. In other - words, to the base class's constructor or destructor, `this` object behaves - like an instance of the base class, not the derived class. This rule is - required for safety. Otherwise a base constructor may use members of a - derived class before they are initialized, or a base destructor may use - members of a derived class after they have been destroyed.) Finally, you should be **very cautious** about when to use naggy or strict mocks, as they tend to make tests more brittle and harder to maintain. When you diff --git a/googlemock/include/gmock/gmock-nice-strict.h b/googlemock/include/gmock/gmock-nice-strict.h index 69afa1b..8230058 100644 --- a/googlemock/include/gmock/gmock-nice-strict.h +++ b/googlemock/include/gmock/gmock-nice-strict.h @@ -90,10 +90,51 @@ constexpr bool HasStrictnessModifier() { return decltype(StrictnessModifierProbe(std::declval()))::value; } +// Base classes that register and deregister with testing::Mock to alter the +// default behavior around uninteresting calls. Inheriting from one of these +// classes first and then MockClass ensures the MockClass constructor is run +// after registration, and that the MockClass destructor runs before +// deregistration. This guarantees that MockClass's constructor and destructor +// run with the same level of strictness as its instance methods. + +#if GTEST_OS_WINDOWS && (defined(_MSC_VER) || defined(__clang__)) +// We need to mark these classes with this declspec to ensure that +// the empty base class optimization is performed. +#define GTEST_INTERNAL_EMPTY_BASE_CLASS __declspec(empty_bases) +#else +#define GTEST_INTERNAL_EMPTY_BASE_CLASS +#endif + +template +class NiceMockImpl { + public: + NiceMockImpl() { ::testing::Mock::AllowUninterestingCalls(this); } + + ~NiceMockImpl() { ::testing::Mock::UnregisterCallReaction(this); } +}; + +template +class NaggyMockImpl { + public: + NaggyMockImpl() { ::testing::Mock::WarnUninterestingCalls(this); } + + ~NaggyMockImpl() { ::testing::Mock::UnregisterCallReaction(this); } +}; + +template +class StrictMockImpl { + public: + StrictMockImpl() { ::testing::Mock::FailUninterestingCalls(this); } + + ~StrictMockImpl() { ::testing::Mock::UnregisterCallReaction(this); } +}; + } // namespace internal template -class NiceMock : public MockClass { +class GTEST_INTERNAL_EMPTY_BASE_CLASS NiceMock + : private internal::NiceMockImpl, + public MockClass { public: static_assert( !internal::HasStrictnessModifier(), @@ -102,8 +143,8 @@ class NiceMock : public MockClass { "https://github.com/google/googletest/blob/master/googlemock/docs/" "cook_book.md#the-nice-the-strict-and-the-naggy-nicestrictnaggy"); NiceMock() : MockClass() { - ::testing::Mock::AllowUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } // Ideally, we would inherit base class's constructors through a using @@ -115,21 +156,16 @@ class NiceMock : public MockClass { // made explicit. template explicit NiceMock(A&& arg) : MockClass(std::forward(arg)) { - ::testing::Mock::AllowUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } template NiceMock(TArg1&& arg1, TArg2&& arg2, An&&... args) : MockClass(std::forward(arg1), std::forward(arg2), std::forward(args)...) { - ::testing::Mock::AllowUninterestingCalls( - internal::ImplicitCast_(this)); - } - - ~NiceMock() { // NOLINT - ::testing::Mock::UnregisterCallReaction( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } private: @@ -137,7 +173,9 @@ class NiceMock : public MockClass { }; template -class NaggyMock : public MockClass { +class GTEST_INTERNAL_EMPTY_BASE_CLASS NaggyMock + : private internal::NaggyMockImpl, + public MockClass { static_assert( !internal::HasStrictnessModifier(), "Can't apply NaggyMock to a class hierarchy that already has a " @@ -147,8 +185,8 @@ class NaggyMock : public MockClass { public: NaggyMock() : MockClass() { - ::testing::Mock::WarnUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } // Ideally, we would inherit base class's constructors through a using @@ -160,21 +198,16 @@ class NaggyMock : public MockClass { // made explicit. template explicit NaggyMock(A&& arg) : MockClass(std::forward(arg)) { - ::testing::Mock::WarnUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } template NaggyMock(TArg1&& arg1, TArg2&& arg2, An&&... args) : MockClass(std::forward(arg1), std::forward(arg2), std::forward(args)...) { - ::testing::Mock::WarnUninterestingCalls( - internal::ImplicitCast_(this)); - } - - ~NaggyMock() { // NOLINT - ::testing::Mock::UnregisterCallReaction( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } private: @@ -182,7 +215,9 @@ class NaggyMock : public MockClass { }; template -class StrictMock : public MockClass { +class GTEST_INTERNAL_EMPTY_BASE_CLASS StrictMock + : private internal::StrictMockImpl, + public MockClass { public: static_assert( !internal::HasStrictnessModifier(), @@ -191,8 +226,8 @@ class StrictMock : public MockClass { "https://github.com/google/googletest/blob/master/googlemock/docs/" "cook_book.md#the-nice-the-strict-and-the-naggy-nicestrictnaggy"); StrictMock() : MockClass() { - ::testing::Mock::FailUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } // Ideally, we would inherit base class's constructors through a using @@ -204,27 +239,24 @@ class StrictMock : public MockClass { // made explicit. template explicit StrictMock(A&& arg) : MockClass(std::forward(arg)) { - ::testing::Mock::FailUninterestingCalls( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } template StrictMock(TArg1&& arg1, TArg2&& arg2, An&&... args) : MockClass(std::forward(arg1), std::forward(arg2), std::forward(args)...) { - ::testing::Mock::FailUninterestingCalls( - internal::ImplicitCast_(this)); - } - - ~StrictMock() { // NOLINT - ::testing::Mock::UnregisterCallReaction( - internal::ImplicitCast_(this)); + static_assert(sizeof(*this) == sizeof(MockClass), + "The impl subclass shouldn't introduce any padding"); } private: GTEST_DISALLOW_COPY_AND_ASSIGN_(StrictMock); }; +#undef GTEST_INTERNAL_EMPTY_BASE_CLASS + } // namespace testing #endif // GMOCK_INCLUDE_GMOCK_GMOCK_NICE_STRICT_H_ diff --git a/googlemock/include/gmock/gmock-spec-builders.h b/googlemock/include/gmock/gmock-spec-builders.h index ac21550..d42d4cb 100644 --- a/googlemock/include/gmock/gmock-spec-builders.h +++ b/googlemock/include/gmock/gmock-spec-builders.h @@ -108,6 +108,14 @@ template class TypedExpectation; // Helper class for testing the Expectation class template. class ExpectationTester; +// Helper classes for implementing NiceMock, StrictMock, and NaggyMock. +template +class NiceMockImpl; +template +class StrictMockImpl; +template +class NaggyMockImpl; + // Protects the mock object registry (in class Mock), all function // mockers, and all expectations. // @@ -413,14 +421,12 @@ class GTEST_API_ Mock { template friend class internal::FunctionMocker; - template - friend class NiceMock; - - template - friend class NaggyMock; - - template - friend class StrictMock; + template + friend class internal::NiceMockImpl; + template + friend class internal::NaggyMockImpl; + template + friend class internal::StrictMockImpl; // Tells Google Mock to allow uninteresting calls on the given mock // object. diff --git a/googlemock/test/gmock-nice-strict_test.cc b/googlemock/test/gmock-nice-strict_test.cc index 0a201ed..25558eb 100644 --- a/googlemock/test/gmock-nice-strict_test.cc +++ b/googlemock/test/gmock-nice-strict_test.cc @@ -67,6 +67,12 @@ class NotDefaultConstructible { explicit NotDefaultConstructible(int) {} }; +class CallsMockMethodInDestructor { + public: + ~CallsMockMethodInDestructor() { OnDestroy(); } + MOCK_METHOD(void, OnDestroy, ()); +}; + // Defines some mock classes needed by the tests. class Foo { @@ -302,6 +308,13 @@ TEST(NiceMockTest, AcceptsClassNamedMock) { nice.DoThis(); } +TEST(NiceMockTest, IsNiceInDestructor) { + { + NiceMock nice_on_destroy; + // Don't add an expectation for the call before the mock goes out of scope. + } +} + TEST(NiceMockTest, IsNaggy_IsNice_IsStrict) { NiceMock nice_foo; EXPECT_FALSE(Mock::IsNaggy(&nice_foo)); @@ -405,6 +418,22 @@ TEST(NaggyMockTest, AcceptsClassNamedMock) { naggy.DoThis(); } +TEST(NaggyMockTest, IsNaggyInDestructor) { + const std::string saved_flag = GMOCK_FLAG(verbose); + GMOCK_FLAG(verbose) = "warning"; + CaptureStdout(); + + { + NaggyMock naggy_on_destroy; + // Don't add an expectation for the call before the mock goes out of scope. + } + + EXPECT_THAT(GetCapturedStdout(), + HasSubstr("Uninteresting mock function call")); + + GMOCK_FLAG(verbose) = saved_flag; +} + TEST(NaggyMockTest, IsNaggy_IsNice_IsStrict) { NaggyMock naggy_foo; EXPECT_TRUE(Mock::IsNaggy(&naggy_foo)); @@ -489,6 +518,16 @@ TEST(StrictMockTest, AcceptsClassNamedMock) { strict.DoThis(); } +TEST(StrictMockTest, IsStrictInDestructor) { + EXPECT_NONFATAL_FAILURE( + { + StrictMock strict_on_destroy; + // Don't add an expectation for the call before the mock goes out of + // scope. + }, + "Uninteresting mock function call"); +} + TEST(StrictMockTest, IsNaggy_IsNice_IsStrict) { StrictMock strict_foo; EXPECT_FALSE(Mock::IsNaggy(&strict_foo)); -- cgit v0.12