Changes to code coverage infrastructure to allow subset coverage analysis.

Changes to code coverage infrastructure to allow subset coverage analysis.

This change adds controls to tools/code_coverage/coverage_posix.py to
allow specification of test filters for a coverage run, modifies 
handling of filters to match what gtest expects (all positive filters, a
'-', and all negative filters), disables FLAKY and FAILS tests from 
being included in code coverage, and adds a downloads system specific file 
for constructing html output (restricting output to just the source files 
of interest).


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

[Randy Smith (Not in Mondays), 2011-11-18 01:23:54]

John: When you get a chance, could you take a look at this CL too?  It's
dependent on http://codereview.chromium.org/8588048/.  

I have more concerns about this CL; specifically, what I've put together is a
somewhat hacky way to allow subsystems to do subset coverage analyses.  Maybe
the solution is to add build targets, but that seems like it just proliferates
targets on an ad-hoc basis--if we don't have some general way to do the
subsetting (maybe a database that specified which lines each individual test
covered?) it seems like we should just provide easy-to-use tools and let people
write their own scripts to run them.

I also feel like downloads.croc is a hack--I couldn't see any way to disable
just the single line in common.croc that specified the directories to cover with
an overriding .croc file, so I just copied common.croc, renamed it, and changed
that one line.  There has to be a better way--any ideas?

Anyway, your comments welcome when you have a moment.

[John Grabowski, 2011-11-21 21:21:19]

I agree that proliferation of targets is the wrong way.  

Perhaps we can encode more logic into the coverage_posix.py script itself?  The
default coverage target can just call it with it's list of (default) deps. 
Other people can use coverage_posix.py with just a single test to do just that
test.

WDYT?  I wouldn't want you to spend major time redesigning this.

I will LGTM this if you switch downloads.croc from a copy to an include.

http://codereview.chromium.org/8598003/diff/1/build/downloads.croc
File build/downloads.croc (right):

http://codereview.chromium.org/8598003/diff/1/build/downloads.croc#newcode2
build/downloads.croc:2: # Crocodile config file for Chromium - settings common
to all platforms
Perhaps it would be simpler to add an 'include' type statement to croc?
Then this whole file could be 

{ 'includes': [ 'common.croc' ] }

[Randy Smith (Not in Mondays), 2011-11-28 21:23:28]

On 2011/11/21 21:21:19, John Grabowski wrote:
> I agree that proliferation of targets is the wrong way.  
> 
> Perhaps we can encode more logic into the coverage_posix.py script itself? 
The
> default coverage target can just call it with it's list of (default) deps. 
> Other people can use coverage_posix.py with just a single test to do just that
> test.
> 
> WDYT?  I wouldn't want you to spend major time redesigning this.

Me neither :-}.

I think coverage_posix.py taking a list of deps and being callable by
individuals is a fine plan--I'm happy to write my own scripts to call it to get
baselines, and I think that makes more sense than actually embedding stuff into
the build system.  

> I will LGTM this if you switch downloads.croc from a copy to an include.

I think there's a misunderstanding ?  downloads.croc isn't a pure copy; it
tweaks the regexp line that specifies the directories to summarize in the .html
output.  I would love to do that in some cleaner way, but the ordering of those
regexp lines seems crucial to me; specifically, I think the directory regexp
('include' : 1) must be before the suppression regexps ('include' : 0).  Maybe
adding an ability to specify configs on the command line?  Or splitting the
{common/downloads}.croc into two files, one of which *just* specifies the 
 directories to process?  This'll only work if the 'roots' and 'rules' sections
are independent of order.

Let me bundle this up in specific questions for you:

* Would you LGTM just the coverage_posix.py change?  If so, I'm inclined to put
that in and worry about the other issues separately.
* What do you think is the cleanest way to run croc.py on a configuration like
the common one, but with only the directory specification changed.

[John Grabowski, 2011-11-28 21:48:38]

I can LGTM it all.  At quick glance and from your comment, it looked like
downloads.croc was a cut and paste job.  Maybe better would be to add an include
mechanism.  I don't have an easy design idea in mind.  Maybe there isn't one.

Don't have a good idea re: directory spec changed, other than a command line
mechanism (similar to gyp variable overrides, perhaps even with an env var like
CROC_DEFINES).

[Randy Smith (Not in Mondays), 2011-11-30 00:43:18]

John: I decided that I wasn't comfortable with downloads.croc, just because it
was so subsystem specific.  I presume your LGTM stands without it?

[John Grabowski, 2011-11-30 00:52:11]

Yes; LGTM if you fix the string thing

http://codereview.chromium.org/8598003/diff/7001/tools/code_coverage/coverage...
File tools/code_coverage/coverage_posix.py (right):

http://codereview.chromium.org/8598003/diff/7001/tools/code_coverage/coverage...
tools/code_coverage/coverage_posix.py:664: result = "--gtest_filter="
prefer single quotes on strings (multiple spots)

http://codereview.chromium.org/8598003/diff/7001/tools/code_coverage/coverage...
tools/code_coverage/coverage_posix.py:950: dest="test_files",
like this one

http://codereview.chromium.org/8598003/diff/7001/tools/code_coverage/coverage...
tools/code_coverage/coverage_posix.py:952: action="append",
and this one

[Randy Smith (Not in Mondays), 2011-12-01 22:10:52]

Fixed.  Taking LGTM as given; will commit.  Thanks!

(I may come back with alternate proposals for dealing with the downloads.croc
issue in the future.)

[commit-bot: I haz the power, 2011-12-01 22:11:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8598003/11001

[commit-bot: I haz the power, 2011-12-02 02:17:53]

Change committed as 112599