Checkin a version of gmock, modified to use our boost_tuple in VS2005.

Checkin a version of gmock, modified to use our boost_tuple in VS2005.

This checkin adds gmock, and a small example of how to write a gmock-based unittest.

[awong, 2009-05-23 08:39:54]

Here's the gmock checkin.  I opted for just checking in a copy of the tree.

The main files of interest are:
  testing/GMOCK_VERSION
  testing/README.gmock
  testing/gmock.patch
  third_party/boost/boost.gyp

Here are the other changed files that are not just the gmock checkout.  These
have been reviewed previously during my first attempt, and have not been
changed.
  base/base.gyp
  base/gmock_unittest.cc
  build/all.gyp
  chrome/chrome.sln
  testing/gtest.gyp

M-A, Zhanyong, you two would likely be most interested in gmock.patch and
README.gmock.

John, if you'd be willing to sanity check the rest, that'd be great. 

This has passed all the try servers for all 3 platforms.

Thanks!

-Albert

[M-A Ruel, 2009-05-23 10:14:30]

I'm the fastest to review! :P

http://codereview.chromium.org/113807/diff/1/1005
File testing/README.gmock (right):

http://codereview.chromium.org/113807/diff/1/1005#newcode1
Line 1: Gmock requires tr1 tuple.  However, Visual Studio 2005 does not include
See http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/ for an
example on how to name the files. For instance, this file should be named
README.chromium. Include the svn revision number in this file, the file
GMOCK_VERSION is not needed.

You can name the patch "chromium.patch"

http://codereview.chromium.org/113807/diff/1/93
File testing/gmock_boost.patch (right):

http://codereview.chromium.org/113807/diff/1/93#newcode10
Line 10: ! // If the compiler is not GCC 4.0+, we assume the user is using a
I've never seen ! in a diff. Maybe you forgot a git merge?

[awong, 2009-05-23 11:03:56]

Thanks for the fast review!

Made the changes.  Going to sleep now.

http://codereview.chromium.org/113807/diff/1/1005
File testing/README.gmock (right):

http://codereview.chromium.org/113807/diff/1/1005#newcode1
Line 1: Gmock requires tr1 tuple.  However, Visual Studio 2005 does not include
On 2009/05/23 10:14:30, M-A wrote:
> See http://src.chromium.org/viewvc/chrome/trunk/src/third_party/libxml/ for an
> example on how to name the files. For instance, this file should be named
> README.chromium. Include the svn revision number in this file, the file
> GMOCK_VERSION is not needed.
> 
> You can name the patch "chromium.patch"

Changed as suggested, though I'm a bit duboious about not specifying "gmock" in
the filenames somewhere since this directory contains both gmock and gtest.

http://codereview.chromium.org/113807/diff/1/93
File testing/gmock_boost.patch (right):

http://codereview.chromium.org/113807/diff/1/93#newcode10
Line 10: ! // If the compiler is not GCC 4.0+, we assume the user is using a
On 2009/05/23 10:14:30, M-A wrote:
> I've never seen ! in a diff. Maybe you forgot a git merge?

It's actually correct.  The format is a context diff, which has ! for lines that
have been changed.

http://en.wikipedia.org/wiki/Diff#Context_format

I redid it with a unified diff so it doesn't look as odd. It's also shorter. :)

[zhanyong.wan, 2009-05-24 06:54:53]

http://codereview.chromium.org/113807/diff/1085/1090
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/1085/1090#newcode1
Line 1: We include a  snapshot of gmock from
http://googlemock.googlecode.com/svn/trunk
Nit: remove one space before "snapshot".

http://codereview.chromium.org/113807/diff/1085/1090#newcode4
Line 4: Current version: 157
Nit: s/version/revision/

http://codereview.chromium.org/113807/diff/1085/1090#newcode8
Line 8: tree for chromium.  Our version of gmock has been patched to search for
Why not adding the boost path to your include search path instead of patching
gmock?  If you have concern with doing that globally, can you do it when
compiling the tests only?

http://codereview.chromium.org/113807/diff/1085/1090#newcode11
Line 11: chromium.patch modifies gmock/include/gmock/internal/gmock-port.h so
that
It's best to avoid this hack as it depends on the implementation detail of both
gmock and boost.

(Boost tr1 tuple requires both boost_root/boost/tr1/tr1 and boost_root to be in
the include search path.  If you need to upgrade to a new version of boost where
boost_root/boost/tr1/tr1/tuple includes something in boost_root/boost/tr1/tr1,
your approach will stop working.)

http://codereview.chromium.org/113807/diff/1085/1090#newcode18
Line 18: getting an extra copy of gmock.  Use the following command:
s/gmock/gtest/

http://codereview.chromium.org/113807/diff/1085/1090#newcode19
Line 19: svn checkout --ignore-externals \
Shouldn't this command specify a revision number?

http://codereview.chromium.org/113807/diff/1085/1090#newcode21
Line 21: 2) Delete all the .svn directories.
FYI: If you are concerned with minimizing the number of files, I can give you a
list of files that you don't need.

http://codereview.chromium.org/113807/diff/1085/1090#newcode23
Line 23: 3) Patch it with gmock_boost.patch.
s/gmock/chromium/

http://codereview.chromium.org/113807/diff/1085/101
File testing/chromium.patch (right):

http://codereview.chromium.org/113807/diff/1085/101#newcode10
Line 10: +// searching from boost's root.  This is to avoid extra dirtying of
the
I think "dirtying" the include search path is the cleaner approach.  Anything
else is a hack.

If you still don't want to add the boost paths to your include search path
though, I'd suggest adding a "tuple" file to gmock/include/ with the sole
purpose of pulling in boost/tr1/tr1/tuple.  Then you do not need to patch gmock.

[M-A Ruel, 2009-05-24 10:56:47]

http://codereview.chromium.org/113807/diff/1085/1090
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/1085/1090#newcode19
Line 19: svn checkout --ignore-externals \
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> Shouldn't this command specify a revision number?

No but it should use `svn export` instead of `svn checkout` and deleting the
.svn manually.

http://codereview.chromium.org/113807/diff/1085/101
File testing/chromium.patch (right):

http://codereview.chromium.org/113807/diff/1085/101#newcode10
Line 10: +// searching from boost's root.  This is to avoid extra dirtying of
the
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> I think "dirtying" the include search path is the cleaner approach.  Anything
> else is a hack.
> 
> If you still don't want to add the boost paths to your include search path
> though, I'd suggest adding a "tuple" file to gmock/include/ with the sole
> purpose of pulling in boost/tr1/tr1/tuple.  Then you do not need to patch
gmock.

Albert, please don't change anything and keep the patch as-is. We'll keep this
hack because this line uses #include <tuple> instead of #include "tuple" (which
is fine in theory) but makes it possible that the current directory is not first
in the include path.

[zhanyong.wan, 2009-05-24 20:05:29]

http://codereview.chromium.org/113807/diff/1085/1090
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/1085/1090#newcode19
Line 19: svn checkout --ignore-externals \
On 2009/05/24 10:56:47, M-A wrote:
> On 2009/05/24 06:54:53, zhanyong.wan wrote:
> > Shouldn't this command specify a revision number?
> 
> No but it should use `svn export` instead of `svn checkout` and deleting the
> .svn manually.

I thought you want to control which revision you  get?

http://codereview.chromium.org/113807/diff/1085/101
File testing/chromium.patch (right):

http://codereview.chromium.org/113807/diff/1085/101#newcode10
Line 10: +// searching from boost's root.  This is to avoid extra dirtying of
the
On 2009/05/24 10:56:47, M-A wrote:
> On 2009/05/24 06:54:53, zhanyong.wan wrote:
> > I think "dirtying" the include search path is the cleaner approach. 
Anything
> > else is a hack.
> > 
> > If you still don't want to add the boost paths to your include search path
> > though, I'd suggest adding a "tuple" file to gmock/include/ with the sole
> > purpose of pulling in boost/tr1/tr1/tuple.  Then you do not need to patch
> gmock.
> 
> Albert, please don't change anything and keep the patch as-is. We'll keep this
> hack because this line uses #include <tuple> instead of #include "tuple"
(which
> is fine in theory) but makes it possible that the current directory is not
first
> in the include path.

Ultimately it's you guys' call, as you get to maintain it.  However I'd like to
understand your concern on "#include <tuple>".  I'm not sure why it's important
whether the current directory is the first in the include search path.  As I
understand it, you created this patch because the tuple file cannot be found in
your include search path.  Therefore if we add a file named tuple and put it
*somewhere* in the include path, it is guaranteed to be found by "#include
<tuple>".

While both are hacks, I think the approach I suggested requires less
maintenance, as it's insensitive to changes in gmock-port.h, which we *do*
update from time to time.  It will not work if someday you acquire another
"tuple" file in your source tree and that file appears earlier in the include
path.  However, in that case, probably you can just use that "tuple" file and
don't need any hack any more.

[John Grabowski, 2009-05-26 17:13:41]

Minor tweaks please.

http://codereview.chromium.org/113807/diff/1085/99
File build/all.gyp (right):

http://codereview.chromium.org/113807/diff/1085/99#newcode31
Line 31: '../testing/gmock.gyp:*',
All is dependent on gmock, but there may be other explicit deps you need.  E.g.
base_unittests dep on gmock.

http://codereview.chromium.org/113807/diff/1085/1090
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/1085/1090#newcode2
Line 2: with chromium.patch applied.
Idea: if someone is looking around for 'how do I use gmock?', this is probably
the file they'd find.  Add a brief reference to gmock_unittest.cc as the example
of using in Chromium.

http://codereview.chromium.org/113807/diff/1085/1090#newcode6
Line 6: Gmock requires tr1 tuple.  However, Visual Studio 2005 does not include
Despite all the comments, I'm glad you made the effort to document what
happened.

http://codereview.chromium.org/113807/diff/1085/101
File testing/chromium.patch (right):

http://codereview.chromium.org/113807/diff/1085/101#newcode10
Line 10: +// searching from boost's root.  This is to avoid extra dirtying of
the
On 2009/05/24 20:05:29, zhanyong.wan wrote:
> On 2009/05/24 10:56:47, M-A wrote:
> > On 2009/05/24 06:54:53, zhanyong.wan wrote:
> > > I think "dirtying" the include search path is the cleaner approach. 
> Anything
> > > else is a hack.
> > > 
> > > If you still don't want to add the boost paths to your include search path
> > > though, I'd suggest adding a "tuple" file to gmock/include/ with the sole
> > > purpose of pulling in boost/tr1/tr1/tuple.  Then you do not need to patch
> > gmock.
> > 
> > Albert, please don't change anything and keep the patch as-is. We'll keep
this
> > hack because this line uses #include <tuple> instead of #include "tuple"
> (which
> > is fine in theory) but makes it possible that the current directory is not
> first
> > in the include path.
> 
> Ultimately it's you guys' call, as you get to maintain it.  However I'd like
to
> understand your concern on "#include <tuple>".  I'm not sure why it's
important
> whether the current directory is the first in the include search path.  As I
> understand it, you created this patch because the tuple file cannot be found
in
> your include search path.  Therefore if we add a file named tuple and put it
> *somewhere* in the include path, it is guaranteed to be found by "#include
> <tuple>".
> 
> While both are hacks, I think the approach I suggested requires less
> maintenance, as it's insensitive to changes in gmock-port.h, which we *do*
> update from time to time.  It will not work if someday you acquire another
> "tuple" file in your source tree and that file appears earlier in the include
> path.  However, in that case, probably you can just use that "tuple" file and
> don't need any hack any more.

For what it's worth, I like the patch.  Simple, unambiguous, and unlikely to
accidentally modify a non-VS2005 build.

http://codereview.chromium.org/113807/diff/1085/177
File third_party/boost/boost.gyp (right):

http://codereview.chromium.org/113807/diff/1085/177#newcode4
Line 4: 
Briefly explain why this is here.  E.g.
- we generally don't want boost in Chromium
- Use base::Tuple if you need a tuple
- This is only here for gmock + VS2005

[awong, 2009-05-26 20:56:28]

Thanks for all the comments!

Fixed the typos + grammar issues.  Also added a bit more detail into the readme
for why decisions were made, etc.

Let me know what you think.

BTw, if rietveld hangs, try refersh 2-3 times.  It seems to work after it warms
up...someone should go in and patch it to handle large data more gracefully...

http://codereview.chromium.org/113807/diff/1085/99
File build/all.gyp (right):

http://codereview.chromium.org/113807/diff/1085/99#newcode31
Line 31: '../testing/gmock.gyp:*',
On 2009/05/26 17:13:41, John Grabowski wrote:
> All is dependent on gmock, but there may be other explicit deps you need. 
E.g.
> base_unittests dep on gmock.

I added it into base.gyp.  I think that's all that's needed...

http://codereview.chromium.org/113807/diff/1085/1090
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/1085/1090#newcode1
Line 1: We include a  snapshot of gmock from
http://googlemock.googlecode.com/svn/trunk
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> Nit: remove one space before "snapshot".

Done.

http://codereview.chromium.org/113807/diff/1085/1090#newcode2
Line 2: with chromium.patch applied.
On 2009/05/26 17:13:41, John Grabowski wrote:
> Idea: if someone is looking around for 'how do I use gmock?', this is probably
> the file they'd find.  Add a brief reference to gmock_unittest.cc as the
example
> of using in Chromium. 

Add a reference to the example test and the googlemock website.

http://codereview.chromium.org/113807/diff/1085/1090#newcode4
Line 4: Current version: 157
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> Nit: s/version/revision/

Done.

http://codereview.chromium.org/113807/diff/1085/1090#newcode8
Line 8: tree for chromium.  Our version of gmock has been patched to search for
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> Why not adding the boost path to your include search path instead of patching
> gmock?  If you have concern with doing that globally, can you do it when
> compiling the tests only?

The problem is that, aside from testing for _MSC_VER, it's hard to hit the
wanted selection criteria for wanting boost (MSVC < VS 2008).  The build system
does not support this sort of thing naturally, so this felt like the lower
impact solution -- espcially since we're already checking in gmock rather than
grabbing from the svn head due the issues with svn:external.

http://codereview.chromium.org/113807/diff/1085/1090#newcode11
Line 11: chromium.patch modifies gmock/include/gmock/internal/gmock-port.h so
that
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> It's best to avoid this hack as it depends on the implementation detail of
both
> gmock and boost.
> 
> (Boost tr1 tuple requires both boost_root/boost/tr1/tr1 and boost_root to be
in
> the include search path.  If you need to upgrade to a new version of boost
where
> boost_root/boost/tr1/tr1/tuple includes something in boost_root/boost/tr1/tr1,
> your approach will stop working.)

Yeah, I think at this point, I'm willing to pay the possible maintenance cost. 
Though the patch "peers behind the walls," you've gotta admit, it's pretty
minor.  If there is a break on an upgrade in the future, we can go the other
route then.

http://codereview.chromium.org/113807/diff/1085/1090#newcode19
Line 19: svn checkout --ignore-externals \
On 2009/05/24 20:05:29, zhanyong.wan wrote:
> On 2009/05/24 10:56:47, M-A wrote:
> > On 2009/05/24 06:54:53, zhanyong.wan wrote:
> > > Shouldn't this command specify a revision number?
> > 
> > No but it should use `svn export` instead of `svn checkout` and deleting the
> > .svn manually.
> 
> I thought you want to control which revision you  get?

:-/  Switched to export.  Also added a * that addresses the revision issue. 
Hopefully people are smart enough to get the revision number from the earlier
part of the file if they care.

http://codereview.chromium.org/113807/diff/1085/1090#newcode21
Line 21: 2) Delete all the .svn directories.
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> FYI: If you are concerned with minimizing the number of files, I can give you
a
> list of files that you don't need.

It wasn't so much about minimizing the number of files as trying not to confuse
svn. :)  Unlike the "full package" of boost_tuple, gmock isn't 9 megs, so I
don't think we need to crunch this one down yet.   Thanks for the offer though!

http://codereview.chromium.org/113807/diff/1085/1090#newcode23
Line 23: 3) Patch it with gmock_boost.patch.
On 2009/05/24 06:54:53, zhanyong.wan wrote:
> s/gmock/chromium/

Done.

[zhanyong.wan, 2009-05-26 21:04:21]

Looks good. (I only looked at README.chromium and chromium.patch.)

Thanks for answering my questions, everyone.  Thanks for the hard work, Albert.

http://codereview.chromium.org/113807/diff/3007/1184
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/3007/1184#newcode18
Line 18: In particular, the "For Dummies," guide a good place to start.  The
Missing "... is ..."?

[John Grabowski, 2009-05-26 21:17:58]

LGTM

(But you might want to wait a little -- tree closed most of today so best not
get caught up in the fray)

[awong, 2009-05-26 21:18:42]

Fixed the missing "is."  I apparently need to take remedial English. :-/

http://codereview.chromium.org/113807/diff/3007/1184
File testing/README.chromium (right):

http://codereview.chromium.org/113807/diff/3007/1184#newcode18
Line 18: In particular, the "For Dummies," guide a good place to start.  The
On 2009/05/26 21:04:21, zhanyong.wan wrote:
> Missing "... is ..."?

Done.