Add gmock into our dependencies.

Closing out of date bug

[awong, 2009-05-08 20:24:56]

Gmock addition into deps.

This will end up including a duplicate gtest tree, but it should be okay I
think.

There's one TODO in there that I'd like to resolve before commit, but I'm afraid
I don't understand git, or our build setup conventions enough to figure it out
myself.  FYI, the gmock.gyp file is basically ripped off of the gunit.gyp file.

[John Grabowski, 2009-05-08 20:32:03]

LGTM (but wait for confirmation from Nicholas)

You are awesome!

It might be nice to have a trivial example in the Chrome code as a proof of
concept (e.g. to show how to link in gmock in a unittest bundle) but that's not
really necessary.

http://codereview.chromium.org/113163/diff/1/2
File DEPS (right):

http://codereview.chromium.org/113163/diff/1/2#newcode22
Line 22: "http://googlemock.googlecode.com/svn/trunk@150",
As discussed on email and in chat, it's generally been agreed that it's OK for
now (even for the not-short-term) to have two copies of gtest.

http://codereview.chromium.org/113163/diff/1/4
File testing/gmock.gyp (right):

http://codereview.chromium.org/113163/diff/1/4#newcode49
Line 49: # TODO(ajwong): Why do we need this if we're always #including off
project root?
Possible answer: because gmock itself doesn't #include "testing/gmock/include";
it #includes "gmock/include".  Different root.
cc'ing mark mentovai on reviewers for advice.

[awong, 2009-05-08 20:32:22]

> I don't understand git, or our build setup conventions enough to 

and by git, I meant gyp...

[Nicolas Sylvain, 2009-05-08 20:52:24]

The code that I see LGTM.  I'm not the best to review GYP, but i'm pretty sure
you based it off gtest.gyp right?

Who is using this code?  If you plan to use it on Windows before we switch to
gyp, you'll need to modify chrome.sln to include the generated gmock.vcproj.

+cc sgk for gyp godness.

LGTM

[sgk, 2009-05-08 23:20:34]

http://codereview.chromium.org/113163/diff/1/4
File testing/gmock.gyp (right):

http://codereview.chromium.org/113163/diff/1/4#newcode7
Line 7: 'chromium_code': 1,
This is intended for code that's "specific to chromium," which doesn't include
gmock.

The specific effect of setting this is to build with -Wall -Werror (on Linux,
and corresponding settings on the other platforms) because we can *enforce* that
the code will never  generate warnings for any platform on which we'll build
chromium in the future.  Sure, gmock is Googley and we "know" that it has a
similar standard of not allowing warnings.  But if we were to start building
with a toolchain that wasn't supported by upstream gmock, there might be periods
of time where we'd have to live with warnings (presumably while we worked on
getting changes accepted upstream to accomodate the new toolchain).

In short, since this really isn't chromium specific, take this out.  The
upstream project will have its own standards for only allowing warning-free
code; we don't need to redundantly enforce that.

http://codereview.chromium.org/113163/diff/1/4#newcode49
Line 49: # TODO(ajwong): Why do we need this if we're always #including off
project root?
This 'direct_dependent_settings' will add
-Itesting/gmock/include

to the compilation for all of the *other* targets in the project that list gmock
as a dependency.  If the coding standard will be for all other chromium code to:

#include <testing/gmock/include/gmock_foobar.h>

then you shouldn't need this.  You should only need this if you expect other
code to be able to do:

#include <gmock_foobar.h>

[awong, 2009-05-09 00:32:50]

Address the gyp issues.

John, I added a unittest showing off some of the features of gmock.  The main
one that I didn't add (mainly cause I got tired) was an example of invoking a
callback via gmock using the InvokeArgument action.  If I get unlazy I'll add it
monday morning.

Let me know what you think of the new tests.  Note that since i've now added a
unittest, I'm running it through the try servers since the added dependency
could potentially break msvc.  We'll find out in a few hours.

http://codereview.chromium.org/113163/diff/1/4
File testing/gmock.gyp (right):

http://codereview.chromium.org/113163/diff/1/4#newcode7
Line 7: 'chromium_code': 1,
On 2009/05/08 23:20:34, sgk wrote:
> This is intended for code that's "specific to chromium," which doesn't include
> gmock.
> 
> The specific effect of setting this is to build with -Wall -Werror (on Linux,
> and corresponding settings on the other platforms) because we can *enforce*
that
> the code will never  generate warnings for any platform on which we'll build
> chromium in the future.  Sure, gmock is Googley and we "know" that it has a
> similar standard of not allowing warnings.  But if we were to start building
> with a toolchain that wasn't supported by upstream gmock, there might be
periods
> of time where we'd have to live with warnings (presumably while we worked on
> getting changes accepted upstream to accomodate the new toolchain).
> 
> In short, since this really isn't chromium specific, take this out.  The
> upstream project will have its own standards for only allowing warning-free
> code; we don't need to redundantly enforce that.

Thanks for the explanation.  I removed it from here and the gtest.gyp file.

http://codereview.chromium.org/113163/diff/1/4#newcode49
Line 49: # TODO(ajwong): Why do we need this if we're always #including off
project root?
On 2009/05/08 23:20:34, sgk wrote:
> This 'direct_dependent_settings' will add
> -Itesting/gmock/include
> 
> to the compilation for all of the *other* targets in the project that list
gmock
> as a dependency.  If the coding standard will be for all other chromium code
to:
> 
> #include <testing/gmock/include/gmock_foobar.h>
> 
> then you shouldn't need this.  You should only need this if you expect other
> code to be able to do:
> 
> #include <gmock_foobar.h>

Turns out John was right.  Gmock, and I assume gtest, need their respective
include directories in order to build.  I removed the extra gtest/ from the
gtest.gyp and added comments in both.

Running through try bots now to see how well it fares.

[John Grabowski, 2009-05-09 01:03:03]

Still LGTM

I don't think a callback test is needed; even hello world is fine (to confirm
link happiness).  You have already gone beyond expectation.

A pox on CLs without unit tests.

http://codereview.chromium.org/113163/diff/1018/17
File base/gmock_unittest.cc (right):

http://codereview.chromium.org/113163/diff/1018/17#newcode9
Line 9: #include "testing/gmock/include/gmock/gmock.h"
This file is fantastic as a proof of concept that your work can be immediately
used by anyone in Chrome, no excuses.

[sgk, 2009-05-09 01:46:25]

lgtm, one nit

http://codereview.chromium.org/113163/diff/22/29
File testing/gtest.gyp (right):

http://codereview.chromium.org/113163/diff/22/29#newcode56
Line 56: 'gtest/include',  # So that gmock headers can find themselves.
nit:  gmock, or gtest headers can find themselves?