Support for better integration between OCMock and gtest.

Support for better integration between OCMock and gtest.

- Add a chromium method that will use OCMock's verify support, but add any
  failure to the gtest failure log and return something that can be asserted
  in the test.
- Update the code using OCMock -verify to use the new util.
- Remove the OCMock includes in files that weren't actually using it.

TEST=none
BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85676

[TVL, 2011-05-16 21:26:20]

Note: I'm not trilled with the namespace I put this in or the source name. 
Better ideas welcome.  :)

[TVL, 2011-05-16 21:29:06]

And here is what the output ends up looking like:


/Users/thomasvl/src/chrome/src/third_party/ocmock/gtest_support.mm:31: Failure
Failed
Error validating OCMock: OCPartialMockObject[BookmarkMenu]: expected method was
not invoked: cancelTracking
/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1573:
Failure
Value of: testing::mac::VerifyOCMock(fakeMenu)
  Actual: false
Expected: true


I know c++ defaults are allowed, but we could bend the rules and make a version
that takes __FILE__ and __LINE__ to use the version of ADD_FAILURE that takes a
file/line to have the first report also be from the source in question instead.

[Robert Sesek, 2011-05-16 21:30:44]

My only concern is discoverability: How are new people writing OCMock tests
going to know to use this instead of the normal |-verify|?

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_support.h
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
third_party/ocmock/gtest_support.h:17: // gtest EXECPT_TRUE/ASSERT_TRUE fashion.
For extra clarification, maybe add a code snippet?

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
third_party/ocmock/gtest_support.mm:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/ocmock.gyp
File third_party/ocmock/ocmock.gyp (left):

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/ocmock.gyp#o...
third_party/ocmock/ocmock.gyp:28: 'OCMock/OCMArg.h',
Yuck. Sorry about that.

[TVL, 2011-05-16 21:34:51]

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_support.h
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
third_party/ocmock/gtest_support.h:17: // gtest EXECPT_TRUE/ASSERT_TRUE fashion.
On 2011/05/16 21:30:44, rsesek wrote:
> For extra clarification, maybe add a code snippet?

Done.

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/1/third_party/ocmock/gtest_suppor...
third_party/ocmock/gtest_support.mm:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/16 21:30:44, rsesek wrote:
> 2011

Done.

[TVL, 2011-05-16 21:38:47]

On 2011/05/16 21:30:44, rsesek wrote:
> My only concern is discoverability: How are new people writing OCMock tests
> going to know to use this instead of the normal |-verify|?

Yea, but given the total lock of any help on the ocmock headers, I'm not sure
what else we can do.  :(

[Mark Mentovai, 2011-05-16 22:16:38]

http://codereview.chromium.org/7031014/diff/14/chrome/browser/ui/cocoa/bookma...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

http://codereview.chromium.org/7031014/diff/14/chrome/browser/ui/cocoa/bookma...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1573:
ASSERT_TRUE(testing::mac::VerifyOCMock(fakeMenu));
You’ve used ASSERT pretty consistently here. Any reason for that instead of
EXPECT?

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:8: 
I am unnecessary.

(The blank line, that is.)

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:15: // generated, adding it to the gtest
failures and return true/false
Proofread this comment.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:19: //     id mockFoo = [OCMockObject
mockForClass:[Foo class]];
Perhaps show an example of the traditional OCMock -verify that your VerifyOCMock
function is intended to replace.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:21: //    
ASSERT_TRUE(testing::mac::VerifyOCMock(mockFoo));
If you switch to EXPECT at the actual call sites, you should switch the example
to EXPECT as well. Even if you wind up with a mix of EXPECT and ASSERT, I’d
write EXPECT here because it’s generally preferable and in most of our tests,
far more common.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:24: 
Is it worth providing macros like EXPECT_OCMOCK_VERIFY and ASSERT_OCMOCK_VERIFY?

You can say no. Macros are evil, but it’s how gtest works, and
|EXPECT_TRUE(testing::mac::VerifyOCMock| is a mouthful.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:29: @catch (NSException* e) {
I read @try/@catch as part of the same construct, just like if/else. According
to the style we’re supposed to use, the @catch should be moved up to be nestled
between the } @braces {.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:31: ADD_FAILURE() << "Error validating
OCMock: "
Reword? It’s not really validating OCMock, it’s validating what you asked OCMock
to do.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:32: << [[e description] UTF8String];
Line the << up with the << above.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp
File third_party/ocmock/ocmock.gyp (left):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:22: 'OCMock/NSMethodSignature+OCMAdditions.h',
Heh, who let the tabs in here?

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp
File third_party/ocmock/ocmock.gyp (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:14: 'include_dirs': [ '.', '../..', ],
Surprised you needed this. I assume it was for the gtest include. gtest doesn’t
put the right thing into its own direct_dependent_settings?

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:19: '<(DEPTH)/testing/gtest.gyp:gtest',
Don’t use DEPTH unless you need it. Just write a relative path here.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:25: # OCMock sources.
Blank before.

[TVL, 2011-05-17 14:57:16]

Mark - Any opinion on the output format and using defaults to pick up __FILE__,
__LINE__ so the one output line will point to the unittest and not the support
file?

http://codereview.chromium.org/7031014/diff/14/chrome/browser/ui/cocoa/bookma...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

http://codereview.chromium.org/7031014/diff/14/chrome/browser/ui/cocoa/bookma...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1573:
ASSERT_TRUE(testing::mac::VerifyOCMock(fakeMenu));
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> You’ve used ASSERT pretty consistently here. Any reason for that instead of
> EXPECT?

Habit.  I've seen too many tests where something goes wrong, but it's all EXPECT
checks, so the rest of the test not only fails, but crashes, preventing the rest
of the unittests in the binary from running.  I'll go ahead and switch them to
EXCPECTs.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:8: 
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> I am unnecessary.
> 
> (The blank line, that is.)

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:15: // generated, adding it to the gtest
failures and return true/false
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Proofread this comment.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:19: //     id mockFoo = [OCMockObject
mockForClass:[Foo class]];
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Perhaps show an example of the traditional OCMock -verify that your
VerifyOCMock
> function is intended to replace.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:21: //    
ASSERT_TRUE(testing::mac::VerifyOCMock(mockFoo));
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> If you switch to EXPECT at the actual call sites, you should switch the
example
> to EXPECT as well. Even if you wind up with a mix of EXPECT and ASSERT, I’d
> write EXPECT here because it’s generally preferable and in most of our tests,
> far more common.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:24: 
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Is it worth providing macros like EXPECT_OCMOCK_VERIFY and
ASSERT_OCMOCK_VERIFY?
> 
> You can say no. Macros are evil, but it’s how gtest works, and
> |EXPECT_TRUE(testing::mac::VerifyOCMock| is a mouthful.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:29: @catch (NSException* e) {
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> I read @try/@catch as part of the same construct, just like if/else. According
> to the style we’re supposed to use, the @catch should be moved up to be
nestled
> between the } @braces {.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:31: ADD_FAILURE() << "Error validating
OCMock: "
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Reword? It’s not really validating OCMock, it’s validating what you asked
OCMock
> to do.

I'm not liking this choice better, do you have a suggestion here?

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:32: << [[e description] UTF8String];
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Line the << up with the << above.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp
File third_party/ocmock/ocmock.gyp (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:14: 'include_dirs': [ '.', '../..', ],
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Surprised you needed this. I assume it was for the gtest include. gtest
doesn’t
> put the right thing into its own direct_dependent_settings?

Almost every projects start be setting the include_dirs to the root of the tree.
 I think base is one of the few that does set it so a few things that directly
depend on base end up not needing it, but that's it.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:19: '<(DEPTH)/testing/gtest.gyp:gtest',
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Don’t use DEPTH unless you need it. Just write a relative path here.

Done.

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/ocmock.gyp#...
third_party/ocmock/ocmock.gyp:25: # OCMock sources.
On 2011/05/16 22:16:38, Mark Mentovai wrote:
> Blank before.

Done.

[Robert Sesek, 2011-05-17 15:01:15]

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/14/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:31: ADD_FAILURE() << "Error validating
OCMock: "
On 2011/05/17 14:57:16, TVL wrote:
> On 2011/05/16 22:16:38, Mark Mentovai wrote:
> > Reword? It’s not really validating OCMock, it’s validating what you asked
> OCMock
> > to do.
> 
> I'm not liking this choice better, do you have a suggestion here?

This also struck me as a bit off, but I didn't leave a comment. How about
'verifying' instead of 'validating'? Maybe "Failed to validate OCMock: "?

[Mark Mentovai, 2011-05-17 15:06:16]

What do you mean “using defaults to pick up __FILE__ and __LINE__”. Just based
on what gtest provides, I don’t see a really good way to do __FILE__ and
__LINE__.

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
third_party/ocmock/gtest_support.mm:30: ADD_FAILURE() << "OCMock call(s)
mismatch: "
How ’bout "OCMock validation failed: "?

[Mark Mentovai, 2011-05-17 15:07:57]

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
third_party/ocmock/gtest_support.mm:30: ADD_FAILURE() << "OCMock call(s)
mismatch: "
Never mind my previous brain fart of a comment. Make this function accept file
and line arguments, and use ADD_FAILURE_AT here.

[Mark Mentovai, 2011-05-17 15:10:16]

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
third_party/ocmock/gtest_support.h:35: // Helper macros that looks a little more
like gtest macros.
looks -> look

[TVL, 2011-05-17 19:19:38]

Mark - is there a better way to deal with getting __FILE__, __LINE__ in there? 
This results in:


/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/chrome_event_processing_window_unittest.mm:87:
Failure
Failed
OCMock validation failed: OCMockObject[BrowserWindowController]: expected method
was not invoked: executeCommand:34018
/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/chrome_event_processing_window_unittest.mm:87:
Failure
Value of: testing::mac::VerifyOCMockFileLine((delegate), __FILE__, __LINE__)
  Actual: false
Expected: true

Which gets the OCMock line to properly tag the right file/line.  But you can't
use defaults on VerifyOCMockFile or it records the helper header file and not
the unittest.

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
third_party/ocmock/gtest_support.h:35: // Helper macros that looks a little more
like gtest macros.
On 2011/05/17 15:10:16, Mark Mentovai wrote:
> looks -> look

Done.

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/7001/third_party/ocmock/gtest_sup...
third_party/ocmock/gtest_support.mm:30: ADD_FAILURE() << "OCMock call(s)
mismatch: "
On 2011/05/17 15:06:16, Mark Mentovai wrote:
> How ’bout "OCMock validation failed: "?

Done.

[Mark Mentovai, 2011-05-17 19:23:59]

thomasvl@chromium.org wrote:
> Mark - is there a better way to deal with getting __FILE__, __LINE__ in
> there?
> This results in:
>
>
>
/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/chrome_event_processing_window_unittest.mm:87:
> Failure
> Failed
> OCMock validation failed: OCMockObject[BrowserWindowController]: expected
> method
> was not invoked: executeCommand:34018
>
/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/chrome_event_processing_window_unittest.mm:87:
> Failure
> Value of: testing::mac::VerifyOCMockFileLine((delegate), __FILE__, __LINE__)
>  Actual: false
> Expected: true
>
> Which gets the OCMock line to properly tag the right file/line.  But you can't
> use defaults on VerifyOCMockFile or it records the helper header file and not
> the unittest.

I don’t understand what you mean. Which “defaults?” Are you upset
about seeing “Value of: testing::mac::VerifyOCMockFileLine((delegate),
__FILE__, __LINE__)” instead of the expression that’s actually at
chrome_event_processing_window_unittest.mm:87?

[TVL, 2011-05-17 19:27:37]

On Tue, May 17, 2011 at 3:23 PM, Mark Mentovai <mark@chromium.org> wrote:

> thomasvl@chromium.org wrote:
> > Mark - is there a better way to deal with getting __FILE__, __LINE__ in
> > there?
> > This results in:
> >
> >
> > /Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/
> chrome_event_processing_window_unittest.mm:87:
> > Failure
> > Failed
> > OCMock validation failed: OCMockObject[BrowserWindowController]: expected
> > method
> > was not invoked: executeCommand:34018
> > /Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/
> chrome_event_processing_window_unittest.mm:87:
> > Failure
> > Value of: testing::mac::VerifyOCMockFileLine((delegate), __FILE__,
> __LINE__)
> >  Actual: false
> > Expected: true
> >
> > Which gets the OCMock line to properly tag the right file/line.  But you
> can't
> > use defaults on VerifyOCMockFile or it records the helper header file and
> not
> > the unittest.
>
> I don’t understand what you mean. Which “defaults?” Are you upset
> about seeing “Value of: testing::mac::VerifyOCMockFileLine((delegate),
> __FILE__, __LINE__)” instead of the expression that’s actually at
> chrome_event_processing_window_unittest.mm:87?
>


My first attempt was:

bool VerifyOCMock(OCMockObject* mock, const char* file = __FILE__, int line
= __LINE__);

But that recorded the .h file instead of the unittest.

[Mark Mentovai, 2011-05-17 19:30:52]

Thomas Van Lenten wrote:
> My first attempt was:
> bool VerifyOCMock(OCMockObject* mock, const char* file = __FILE__, int line
> = __LINE__);
> But that recorded the .h file instead of the unittest.

Oh, you meant C++ default arguments.

That’s guaranteed to behave as you saw it behaving, which is to say,
it’s guaranteed NOT to behave how you want it to behave.

Why not just make the EXPECT_ and ASSERT_ macros the only supported
way of using this? Put the function inside yet another namespace with
“internal” in the name to discourage people from using it directly.

As for this:

> Failure
> Value of: testing::mac::VerifyOCMockFileLine((delegate), __FILE__, __LINE__)
>  Actual: false
> Expected: true

in case you care, the only way to get the “right” expression to show
up would be to call into gtest internals on your own. You can do that,
but since you’ve already got this:

>
/Users/thomasvl/src/chrome/src/chrome/browser/ui/cocoa/chrome_event_processing_window_unittest.mm:87:

it’s probably not worth it.

[TVL, 2011-05-17 19:37:08]

new diffs up

[Mark Mentovai, 2011-05-17 19:41:10]

LGTM

http://codereview.chromium.org/7031014/diff/21/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/21/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.h:15: }  // namespace mac
// namespace internal

http://codereview.chromium.org/7031014/diff/21/third_party/ocmock/gtest_suppo...
File third_party/ocmock/gtest_support.mm (right):

http://codereview.chromium.org/7031014/diff/21/third_party/ocmock/gtest_suppo...
third_party/ocmock/gtest_support.mm:23: bool VerifyOCMockFileLine(OCMockObject*
mock, const char* file, int line) {
You can just call this VerifyOCMockFile since you’ve removed the non-FileLine
variant.

[commit-bot: I haz the power, 2011-05-17 21:09:38]

Change committed as 85676

[Mark Mentovai, 2011-05-17 21:13:59]

http://codereview.chromium.org/7031014/diff/12005/third_party/ocmock/gtest_su...
File third_party/ocmock/gtest_support.h (right):

http://codereview.chromium.org/7031014/diff/12005/third_party/ocmock/gtest_su...
third_party/ocmock/gtest_support.h:15: }  // namespace mac
You didn’t change this to “// namespace internal”.