IMPORTANT proposal: catch leaked mock objects

[Zhanyong Wan (λx.x x)]

Greetings,

This is about http://code.google.com/p/googlemock/issues/detail?id=24.
Please read this entire message, as what we'll do could affect the
result of your *existing* tests.

** Problem **

In Google Mock, expectations on a mock object is verified when the
mock object is *destroyed*. If you create a mock on the heap but
forget to delete it, the EXPECT_CALL()s on it won't be verified, and
your test could pass when it should fail. This has bitten several
users before.

** Plan **

I plan to change Google Mock to catch leaked mock objects.
Specifically, when your test program exits, Google Mock will see if
there are still mock objects alive. If yes, it will print an error
message and change the program's exit code to 1, indicating a test
failure.

Sometimes, however, a user may not care about verifying a mock object
and may intentionally "leak" it. (For example, he may be using it as
a singleton fake object, which has only default behaviors
(i.e. ON_CALL) but no expectations. Such a singleton object is
intended to be never deleted, such that unjoined threads can keep
using it at program exit time.) Therefore we need to provide a means
for the user to opt out this leak checking. My plan is to let the
user write:

testing::Mock::AllowLeak(mock_object);

to notify Google Mock that it's fine to leak mock_object, which points
to a mock object.

We will also add a command-line flag --gmock_catch_leaked_mock=0 for
disabling this leak check entirely.

** Impact on You **

If you have a test that has been leaking mock objects (you may not
know it yet), it will *fail* after this change is made. Usually, the
failure indicates a *real* problem in your test, and you should fix
it. In the rare case where the leak is intended, you can disable the
leak check either locally (via testing::Mock::AllowLeak()) or globally
(via --gmock_catch_leaked_mock).

I wish the change will only reveal real bugs. Unfortunately, it might
break some good tests too. Based on my experience on how mocks are
used, the odds of false alarm should be quite small. Still, if the
proposed change breaks your tests wrongfully, please accept my apology
and I can work with you to fix the broken tests ASAP.

Suggestions? Concerns? Thanks,

--
Zhanyong

[tsuna]

2009/4/16 Zhanyong Wan (λx.x x) <w...@google.com>:
> Suggestions?  Concerns?  Thanks,

This sounds good to me.

--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory
Software Engineer @ Google

[Bruce Trask]

> Suggestions?  Concerns?  Thanks,

No concerns here. Let 'er rip.

Regards,
Bruce

[Zhanyong Wan (λx.x x)]

Thanks for the feedback, everyone.

2009/4/16 Simon Bowden <sbo...@google.com>:
> Hi Zhanyong,
>
> This sounds good.
>
> As a suggestion, it would be convenient if there were a function to call any
> time [mainly at the end of a single test case] to assert the no-leaks check
> earlier. Specifically, I'd like all of my test cases to assert no leaks at
> the end of the case so that error reporting is better targetted.

I like it. Does

// Returns true if the mock registry is empty; otherwise
// generates a non-fatal failure for each leaked mock object
// and returns false.
bool Mock::CatchLeakedMock();

sound good?

On a related note, we already have functions for manually verifying a
single mock:

bool Mock::VerifyAndClearExpectations(&mock_obj);
bool Mock::VerifyAndClear(&mock_obj);

(See
http://code.google.com/p/googlemock/wiki/CheatSheet#Verifying_and_Resetting_a_Mock
for more info.)

How about supporting manually verifying *all* mocks:

bool Mock::VerifyAndClearExpectationsOfAllMocks();
bool Mock::VerifyAndClearAllMocks();

?

> A nice-to-have that occurs to me now is something like this:
> MockPointer<MyMock> mymock(new MyMock(blah));
> (or NewMock<...>)
>
> MockPointer is the same as a non-null scoped_ptr, except that in its
> destructor it does a "VerifyMockWasDeleted" type operation, that asserts it
> was deleted [and thus verified] in the controllable scope. Gives a way of
> saying "new Object() and assert that it's deleted" in one go. Just a
> thought.

Interesting. This does look nice to have. Since it's not essential,
let's do the other things first and see if more people will want it.

Thanks,

> Cheers,
>
>  - Simon

>
> 2009/4/16 Zhanyong Wan (λx.x x) <w...@google.com>
>>
>>

--
Zhanyong