Start of code coverage for Mac....

Start of code coverage for Mac.

Only base_unittests included for now.  Linux changes added as well but
untested until Linux switches to gyp.

Enable coverage with the following command:

  src/tools/gyp/gyp_dogfood -Dcoverage=1 src/build/all.gyp


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

[Mark Mentovai, 2009-04-01 02:31:46]

LGTM

http://codereview.chromium.org/56136/diff/1003/1004
File build/common.gypi (right):

http://codereview.chromium.org/56136/diff/1003/1004#newcode53
Line 53: 'xcode_settings': { 'OTHER_LDFLAGS': [ '-lgcov' ] },
This is the only way to do it for the time being.

I do plan on supporting something like 'libraries':['-lgcov'] in the Xcode
generator too, but that doesn't work yet.

http://codereview.chromium.org/56136/diff/1003/1005
File chrome/chrome.gyp (right):

http://codereview.chromium.org/56136/diff/1003/1005#newcode2802
Line 2802: '$CONFIGURATION_BUILD_DIR' ],
Use <(PRODUCT_DIR).  That will work on Linux too.

http://codereview.chromium.org/56136/diff/1003/1006
File tools/code_coverage/coverage_posix.py (right):

http://codereview.chromium.org/56136/diff/1003/1006#newcode1
Line 1: #!/usr/bin/env python
Also set svn:eol-style = LF if you didn't.

http://codereview.chromium.org/56136/diff/1003/1006#newcode8
Line 8: Tested on Mac.  Written for Linux (but not tested there which means it
Should say "Written for Linux too" otherwise it seems like it's maybe not even
supposed to work on Macs.

http://codereview.chromium.org/56136/diff/1003/1006#newcode9
Line 9: won't work yet.)
Don't be so hard on yourself, it might.

http://codereview.chromium.org/56136/diff/1003/1006#newcode36
Line 36: self.lcov_directory = os.path.join(sys.path[0],
I don't get why you're using sys.path[0].

http://codereview.chromium.org/56136/diff/1003/1006#newcode37
Line 37: '../../third_party/lcov/bin')
Is this checked in somewhere or something, or will it be?  I don't see it in our
tree.

http://codereview.chromium.org/56136/diff/1003/1006#newcode60
Line 60: return os.uname()[0] == 'Darwin'
This is fine, but I think it's more usual to do |return sys.platform ==
'darwin'| and |return sys.platform == 'linux2'|.

http://codereview.chromium.org/56136/diff/1003/1006#newcode77
Line 77: # TODO(jrg): add timeout?
Also add --gtest_print_time or whatever it's called?

http://codereview.chromium.org/56136/diff/1003/1006#newcode97
Line 97: def main():
I would suggest making the list of unit tests an argument to this script.

If we wanted to get fancy, we could pass the test list to this script from GYP
via a variable expansion from the "coverage" target's "dependencies" list, like:

'action': ['python', 'coverage_posix.py', '--directory', '<(PRODUCT_DIR)',
'<@(_dependencies)']

Then this script would need to understand that it might receive GYP targets, and
only look at the portion after the colon, but it guarantees that the
"dependencies" list in the .gyp file is the canonical list of tests to run with
coverage.

http://codereview.chromium.org/56136/diff/1003/1006#newcode112
Line 112: return 0
Should we be capturing test exit statuses and returning nonzero if a test
returned nonzero?

[TVL, 2009-04-01 03:56:45]

drive by

http://codereview.chromium.org/56136/diff/1003/1004
File build/common.gypi (right):

http://codereview.chromium.org/56136/diff/1003/1004#newcode52
Line 52: ['_type=="executable"', {
should this be '_type=="executable" or _type=="shared_library"'?

[John Grabowski, 2009-04-03 03:42:37]

http://codereview.chromium.org/56136/diff/1003/1004
File build/common.gypi (right):

http://codereview.chromium.org/56136/diff/1003/1004#newcode52
Line 52: ['_type=="executable"', {
On 2009/04/01 03:56:46, TVL wrote:
> should this be '_type=="executable" or _type=="shared_library"'?
> 

Not sure.  I do know I do NOT want static_libraries.  We have no shared libs to
test with.

http://codereview.chromium.org/56136/diff/1003/1004#newcode53
Line 53: 'xcode_settings': { 'OTHER_LDFLAGS': [ '-lgcov' ] },
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> This is the only way to do it for the time being.
> 
> I do plan on supporting something like 'libraries':['-lgcov'] in the Xcode
> generator too, but that doesn't work yet.

You toy with me.

http://codereview.chromium.org/56136/diff/1003/1005
File chrome/chrome.gyp (right):

http://codereview.chromium.org/56136/diff/1003/1005#newcode2802
Line 2802: '$CONFIGURATION_BUILD_DIR' ],
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Use <(PRODUCT_DIR).  That will work on Linux too.


done

http://codereview.chromium.org/56136/diff/1003/1006
File tools/code_coverage/coverage_posix.py (right):

http://codereview.chromium.org/56136/diff/1003/1006#newcode1
Line 1: #!/usr/bin/env python
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Also set svn:eol-style = LF if you didn't.
added

http://codereview.chromium.org/56136/diff/1003/1006#newcode8
Line 8: Tested on Mac.  Written for Linux (but not tested there which means it
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Should say "Written for Linux too" otherwise it seems like it's maybe not even
> supposed to work on Macs.
reworded

http://codereview.chromium.org/56136/diff/1003/1006#newcode9
Line 9: won't work yet.)
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Don't be so hard on yourself, it might.

deharded

http://codereview.chromium.org/56136/diff/1003/1006#newcode36
Line 36: self.lcov_directory = os.path.join(sys.path[0],
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> I don't get why you're using sys.path[0].

cheap way to find the directory where this script lives so I can get a relative
path to lcov.

http://codereview.chromium.org/56136/diff/1003/1006#newcode37
Line 37: '../../third_party/lcov/bin')
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Is this checked in somewhere or something, or will it be?  I don't see it in
our
> tree.

other CL

http://codereview.chromium.org/56136/diff/1003/1006#newcode60
Line 60: return os.uname()[0] == 'Darwin'
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> This is fine, but I think it's more usual to do |return sys.platform ==
> 'darwin'| and |return sys.platform == 'linux2'|.

usualified

http://codereview.chromium.org/56136/diff/1003/1006#newcode77
Line 77: # TODO(jrg): add timeout?
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Also add --gtest_print_time or whatever it's called?

What I meant was, if a unit test hangs (takes forever), we should detect and
bail, just as the builders do.

http://codereview.chromium.org/56136/diff/1003/1006#newcode97
Line 97: def main():
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> I would suggest making the list of unit tests an argument to this script.
> 
> If we wanted to get fancy, we could pass the test list to this script from GYP
> via a variable expansion from the "coverage" target's "dependencies" list,
like:
> 
> 'action': ['python', 'coverage_posix.py', '--directory', '<(PRODUCT_DIR)',
> '<@(_dependencies)']
> 
> Then this script would need to understand that it might receive GYP targets,
and
> only look at the portion after the colon, but it guarantees that the
> "dependencies" list in the .gyp file is the canonical list of tests to run
with
> coverage.

Good idea.  Added a TODO; I'll hit this when I bringup more components.

http://codereview.chromium.org/56136/diff/1003/1006#newcode112
Line 112: return 0
On 2009/04/01 02:31:46, Mark Mentovai wrote:
> Should we be capturing test exit statuses and returning nonzero if a test
> returned nonzero?

Probably, but test success/failure shouldn't change coverage, so I don't know.

[Mark Mentovai, 2009-04-03 04:03:15]

LG

http://codereview.chromium.org/56136/diff/6/2003
File chrome/chrome.gyp (right):

http://codereview.chromium.org/56136/diff/6/2003#newcode2826
Line 2826: 'action': [ '../tools/code_coverage/coverage_posix.py',
Oh right, one more thing, by convention we always put 'python' first on these. 
Two reasons: it guards against scripts that accidentally lose their x-bit, and
on Windows, we need to do this to actually bring up the python interpreter.

http://codereview.chromium.org/56136/diff/6/2004
File tools/code_coverage/coverage_posix.py (right):

http://codereview.chromium.org/56136/diff/6/2004#newcode81
Line 81: subprocess.call([fulltest])
My comment about --gtest_print_time belonged on this line, not the TODO line. 
Stupid trackpad.  I meant that we run all of our unit tests from Xcode with that
argument now, we might want to add the argument here too for consistency.

[John Grabowski, 2009-04-03 04:23:06]

http://codereview.chromium.org/56136/diff/6/2003
File chrome/chrome.gyp (right):

http://codereview.chromium.org/56136/diff/6/2003#newcode2826
Line 2826: 'action': [ '../tools/code_coverage/coverage_posix.py',
On 2009/04/03 04:03:15, Mark Mentovai wrote:
> Oh right, one more thing, by convention we always put 'python' first on these.

> Two reasons: it guards against scripts that accidentally lose their x-bit, and
> on Windows, we need to do this to actually bring up the python interpreter.
added

http://codereview.chromium.org/56136/diff/6/2004
File tools/code_coverage/coverage_posix.py (right):

http://codereview.chromium.org/56136/diff/6/2004#newcode81
Line 81: subprocess.call([fulltest])
On 2009/04/03 04:03:15, Mark Mentovai wrote:
> My comment about --gtest_print_time belonged on this line, not the TODO line. 
> Stupid trackpad.  I meant that we run all of our unit tests from Xcode with
that
> argument now, we might want to add the argument here too for consistency.

We probably don't want it; don't care about time here (unlike xcode).  But I'd
like to be consistent so I added another TODO.