From 5b95fa7b16023c1e1ab0b00f7ce73a2d46a95911 Mon Sep 17 00:00:00 2001 From: "zhanyong.wan" Date: Tue, 27 Jan 2009 22:28:45 +0000 Subject: [PATCH] Improves error messages for undefined return value (by Sverre Sundsdal); improves gmock_doctor. --- include/gmock/gmock-actions.h | 18 +++++++ include/gmock/gmock-spec-builders.h | 30 ++++++----- scripts/gmock_doctor.py | 19 +++++++ test/gmock-actions_test.cc | 77 +++++++++++++++++++++++++++++ test/gmock-spec-builders_test.cc | 12 +++++ 5 files changed, 144 insertions(+), 12 deletions(-) diff --git a/include/gmock/gmock-actions.h b/include/gmock/gmock-actions.h index a4327588..95e075d2 100644 --- a/include/gmock/gmock-actions.h +++ b/include/gmock/gmock-actions.h @@ -69,6 +69,8 @@ class ActionAdaptor; template class BuiltInDefaultValue { public: + // This function returns true iff type T has a built-in default value. + static bool Exists() { return false; } static T Get() { Assert(false, __FILE__, __LINE__, "Default action undefined for the function return type."); @@ -83,6 +85,7 @@ class BuiltInDefaultValue { template class BuiltInDefaultValue { public: + static bool Exists() { return BuiltInDefaultValue::Exists(); } static T Get() { return BuiltInDefaultValue::Get(); } }; @@ -91,6 +94,7 @@ class BuiltInDefaultValue { template class BuiltInDefaultValue { public: + static bool Exists() { return true; } static T* Get() { return NULL; } }; @@ -100,6 +104,7 @@ class BuiltInDefaultValue { template <> \ class BuiltInDefaultValue { \ public: \ + static bool Exists() { return true; } \ static type Get() { return value; } \ } @@ -191,6 +196,12 @@ class DefaultValue { // Returns true iff the user has set the default value for type T. static bool IsSet() { return value_ != NULL; } + // Returns true if T has a default return value set by the user or there + // exists a built-in default value. + static bool Exists() { + return IsSet() || internal::BuiltInDefaultValue::Exists(); + } + // Returns the default value for type T if the user has set one; // otherwise returns the built-in default value if there is one; // otherwise aborts the process. @@ -220,6 +231,12 @@ class DefaultValue { // Returns true iff the user has set the default value for type T&. static bool IsSet() { return address_ != NULL; } + // Returns true if T has a default return value set by the user or there + // exists a built-in default value. + static bool Exists() { + return IsSet() || internal::BuiltInDefaultValue::Exists(); + } + // Returns the default value for type T& if the user has set one; // otherwise returns the built-in default value if there is one; // otherwise aborts the process. @@ -236,6 +253,7 @@ class DefaultValue { template <> class DefaultValue { public: + static bool Exists() { return true; } static void Get() {} }; diff --git a/include/gmock/gmock-spec-builders.h b/include/gmock/gmock-spec-builders.h index 84e0b513..0364b570 100644 --- a/include/gmock/gmock-spec-builders.h +++ b/include/gmock/gmock-spec-builders.h @@ -1061,15 +1061,21 @@ class FunctionMockerBase : public UntypedFunctionMockerBase { return NULL; } - // Performs the default action of this mock function on the given - // arguments and returns the result. This method doesn't depend on - // the mutable state of this object, and thus can be called - // concurrently without locking. + // Performs the default action of this mock function on the given arguments + // and returns the result. Asserts with a helpful call descrption if there is + // no valid return value. This method doesn't depend on the mutable state of + // this object, and thus can be called concurrently without locking. // L = * - Result PerformDefaultAction(const ArgumentTuple& args) const { + Result PerformDefaultAction(const ArgumentTuple& args, + const string& call_description) const { const DefaultActionSpec* const spec = FindDefaultActionSpec(args); - return (spec != NULL) ? spec->GetAction().Perform(args) - : DefaultValue::Get(); + if (spec != NULL) { + return spec->GetAction().Perform(args); + } + Assert(DefaultValue::Exists(), "", -1, + call_description + "\n The mock function has no default action " + "set, and its return type has no default value set."); + return DefaultValue::Get(); } // Registers this function mocker and the mock object owning it; @@ -1407,7 +1413,7 @@ class InvokeWithHelper { Mock::GetReactionOnUninterestingCalls(mocker->MockObject()); // Calculates the function result. - Result result = mocker->PerformDefaultAction(args); + Result result = mocker->PerformDefaultAction(args, ss.str()); // Prints the function result. ss << "\n Returns: "; @@ -1429,8 +1435,8 @@ class InvokeWithHelper { args, &exp, &action, &is_excessive, &ss, &why); ss << " Function call: " << mocker->Name(); UniversalPrinter::Print(args, &ss); - Result result = - action.IsDoDefault() ? mocker->PerformDefaultAction(args) + Result result = action.IsDoDefault() ? + mocker->PerformDefaultAction(args, ss.str()) : action.Perform(args); ss << "\n Returns: "; UniversalPrinter::Print(result, &ss); @@ -1480,7 +1486,7 @@ class InvokeWithHelper { const CallReaction reaction = Mock::GetReactionOnUninterestingCalls(mocker->MockObject()); - mocker->PerformDefaultAction(args); + mocker->PerformDefaultAction(args, ss.str()); ReportUninterestingCall(reaction, ss.str()); return; } @@ -1499,7 +1505,7 @@ class InvokeWithHelper { UniversalPrinter::Print(args, &ss); ss << "\n" << why.str(); if (action.IsDoDefault()) { - mocker->PerformDefaultAction(args); + mocker->PerformDefaultAction(args, ss.str()); } else { action.Perform(args); } diff --git a/scripts/gmock_doctor.py b/scripts/gmock_doctor.py index ce8ec498..ca7935ca 100755 --- a/scripts/gmock_doctor.py +++ b/scripts/gmock_doctor.py @@ -55,6 +55,7 @@ _COMMON_GMOCK_SYMBOLS = [ 'Ge', 'Gt', 'HasSubstr', + 'IsInitializedProto', 'Le', 'Lt', 'MatcherCast', @@ -63,6 +64,7 @@ _COMMON_GMOCK_SYMBOLS = [ 'Not', 'NotNull', 'Pointee', + 'PointeeIsInitializedProto', 'Property', 'Ref', 'StartsWith', @@ -307,12 +309,29 @@ Did you forget to write yield ('NUS', 'Need to Use Symbol', diagnosis % m.groupdict()) +def _NeedToUseReturnNullDiagnoser(msg): + """Diagnoses the NRNULL disease, given the error messages by gcc.""" + + regex = (r'(?P.*):(?P\d+):\s+instantiated from here\n' + r'.*gmock-actions\.h.*error: invalid conversion from ' + r'\'long int\' to \'(?P.+\*)') + + diagnosis = """%(file)s:%(line)s: +You are probably calling Return(NULL) and the compiler isn't sure how to turn +NULL into a %(type)s*. Use ReturnNull() instead. +Note: the line number may be off; please fix all instances of Return(NULL).""" + return _GenericDiagnoser('NRNULL', 'Need to use ReturnNull', + regex, diagnosis, msg) + + + _DIAGNOSERS = [ _IncompleteByReferenceArgumentDiagnoser, _MockObjectPointerDiagnoser, _NeedToReturnNothingDiagnoser, _NeedToReturnReferenceDiagnoser, _NeedToReturnSomethingDiagnoser, + _NeedToUseReturnNullDiagnoser, _NeedToUseSymbolDiagnoser, _OverloadedFunctionActionDiagnoser, _OverloadedFunctionMatcherDiagnoser, diff --git a/test/gmock-actions_test.cc b/test/gmock-actions_test.cc index 1000e306..6fb47bca 100644 --- a/test/gmock-actions_test.cc +++ b/test/gmock-actions_test.cc @@ -82,6 +82,13 @@ TEST(BuiltInDefaultValueTest, IsNullForPointerTypes) { EXPECT_TRUE(BuiltInDefaultValue::Get() == NULL); } +// Tests that BuiltInDefaultValue::Exists() return true. +TEST(BuiltInDefaultValueTest, ExistsForPointerTypes) { + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); +} + // Tests that BuiltInDefaultValue::Get() returns 0 when T is a // built-in numeric type. TEST(BuiltInDefaultValueTest, IsZeroForNumericTypes) { @@ -108,11 +115,42 @@ TEST(BuiltInDefaultValueTest, IsZeroForNumericTypes) { EXPECT_EQ(0, BuiltInDefaultValue::Get()); } +// Tests that BuiltInDefaultValue::Exists() returns true when T is a +// built-in numeric type. +TEST(BuiltInDefaultValueTest, ExistsForNumericTypes) { + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); +#ifndef GTEST_OS_WINDOWS + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); +#endif // GTEST_OS_WINDOWS + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); // NOLINT + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); + EXPECT_TRUE(BuiltInDefaultValue::Exists()); +} + // Tests that BuiltInDefaultValue::Get() returns false. TEST(BuiltInDefaultValueTest, IsFalseForBool) { EXPECT_FALSE(BuiltInDefaultValue::Get()); } +// Tests that BuiltInDefaultValue::Exists() returns true. +TEST(BuiltInDefaultValueTest, BoolExists) { + EXPECT_TRUE(BuiltInDefaultValue::Exists()); +} + // Tests that BuiltInDefaultValue::Get() returns "" when T is a // string type. TEST(BuiltInDefaultValueTest, IsEmptyStringForString) { @@ -125,6 +163,18 @@ TEST(BuiltInDefaultValueTest, IsEmptyStringForString) { #endif // GTEST_HAS_STD_STRING } +// Tests that BuiltInDefaultValue::Exists() returns true when T is a +// string type. +TEST(BuiltInDefaultValueTest, ExistsForString) { +#if GTEST_HAS_GLOBAL_STRING + EXPECT_TRUE(BuiltInDefaultValue< ::string>::Exists()); +#endif // GTEST_HAS_GLOBAL_STRING + +#if GTEST_HAS_STD_STRING + EXPECT_TRUE(BuiltInDefaultValue< ::std::string>::Exists()); +#endif // GTEST_HAS_STD_STRING +} + // Tests that BuiltInDefaultValue::Get() returns the same // value as BuiltInDefaultValue::Get() does. TEST(BuiltInDefaultValueTest, WorksForConstTypes) { @@ -142,6 +192,10 @@ struct UserType { int value; }; +TEST(BuiltInDefaultValueTest, UserTypeHasNoDefault) { + EXPECT_FALSE(BuiltInDefaultValue::Exists()); +} + #ifdef GTEST_HAS_DEATH_TEST // Tests that BuiltInDefaultValue::Get() aborts the program. @@ -170,17 +224,26 @@ TEST(DefaultValueTest, IsInitiallyUnset) { // Tests that DefaultValue can be set and then unset. TEST(DefaultValueTest, CanBeSetAndUnset) { + EXPECT_TRUE(DefaultValue::Exists()); + EXPECT_FALSE(DefaultValue::Exists()); + DefaultValue::Set(1); DefaultValue::Set(UserType()); EXPECT_EQ(1, DefaultValue::Get()); EXPECT_EQ(0, DefaultValue::Get().value); + EXPECT_TRUE(DefaultValue::Exists()); + EXPECT_TRUE(DefaultValue::Exists()); + DefaultValue::Clear(); DefaultValue::Clear(); EXPECT_FALSE(DefaultValue::IsSet()); EXPECT_FALSE(DefaultValue::IsSet()); + + EXPECT_TRUE(DefaultValue::Exists()); + EXPECT_FALSE(DefaultValue::Exists()); } // Tests that DefaultValue::Get() returns the @@ -188,7 +251,9 @@ TEST(DefaultValueTest, CanBeSetAndUnset) { // false. TEST(DefaultValueDeathTest, GetReturnsBuiltInDefaultValueWhenUnset) { EXPECT_FALSE(DefaultValue::IsSet()); + EXPECT_TRUE(DefaultValue::Exists()); EXPECT_FALSE(DefaultValue::IsSet()); + EXPECT_FALSE(DefaultValue::Exists()); EXPECT_EQ(0, DefaultValue::Get()); @@ -212,6 +277,12 @@ TEST(DefaultValueOfReferenceTest, IsInitiallyUnset) { EXPECT_FALSE(DefaultValue::IsSet()); } +// Tests that DefaultValue::Exists is false initiallly. +TEST(DefaultValueOfReferenceTest, IsInitiallyNotExisting) { + EXPECT_FALSE(DefaultValue::Exists()); + EXPECT_FALSE(DefaultValue::Exists()); +} + // Tests that DefaultValue can be set and then unset. TEST(DefaultValueOfReferenceTest, CanBeSetAndUnset) { int n = 1; @@ -219,12 +290,18 @@ TEST(DefaultValueOfReferenceTest, CanBeSetAndUnset) { UserType u; DefaultValue::Set(u); + EXPECT_TRUE(DefaultValue::Exists()); + EXPECT_TRUE(DefaultValue::Exists()); + EXPECT_EQ(&n, &(DefaultValue::Get())); EXPECT_EQ(&u, &(DefaultValue::Get())); DefaultValue::Clear(); DefaultValue::Clear(); + EXPECT_FALSE(DefaultValue::Exists()); + EXPECT_FALSE(DefaultValue::Exists()); + EXPECT_FALSE(DefaultValue::IsSet()); EXPECT_FALSE(DefaultValue::IsSet()); } diff --git a/test/gmock-spec-builders_test.cc b/test/gmock-spec-builders_test.cc index 3e27aa84..3e944ea4 100644 --- a/test/gmock-spec-builders_test.cc +++ b/test/gmock-spec-builders_test.cc @@ -987,6 +987,18 @@ TEST(UnexpectedCallTest, UnsatisifiedPrerequisites) { #endif // GMOCK_HAS_REGEX +#ifdef GTEST_HAS_DEATH_TEST + +TEST(UndefinedReturnValueTest, ReturnValueIsMandatory) { + MockA a; + // TODO(wan@google.com): We should really verify the output message, + // but we cannot yet due to that EXPECT_DEATH only captures stderr + // while Google Mock logs to stdout. + EXPECT_DEATH(a.ReturnResult(1), ""); +} + +#endif // GTEST_HAS_DEATH_TEST + // Tests that an excessive call (one whose arguments match the // matchers but is called too many times) performs the default action. TEST(ExcessiveCallTest, DoesDefaultAction) {