Set coverage path globs on a per-Test basis instead of in main().

Set coverage path globs on a per-Test basis instead of in main().

Allows GenTests methods to specify coverage path information as well.

Also remove extra_coverage_data feature as it was temporary cruft that's no
longer needed.

This is the first step towards having expect_tests auto-load tests when given
a directory.

R=agable@chromium.org, vadimsh@chromium.org
BUG=

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

[Vadim Sh., 2014-06-27 18:06:48]

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
File scripts/slave/unittests/expect_tests/pipeline.py (right):

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/pipeline.py:71: if kill_switch.is_set():
Is it still needed here?

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/pipeline.py:197: gen_cover =
test_gen._covers()  # pylint: disable=W0212
I forgot what type test_gen has here.  Why do you need 'hasattr' here? Also why
'_convers' instead of something without '_'?

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/pipeline.py:199: gen_cover =
[inspect.getabsfile(test_gen)]
Cover single file? Also, have you verified it works with both *.py and *.pyc?
(Some people have local *.pyc generation disabled, some not).

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
File scripts/slave/unittests/expect_tests/type_definitions.py (right):

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/type_definitions.py:200: match =
Test.TEST_COVERS_MATCH.match(test_file)
*.pyc may not work here.

From getabsfile:
"""Return an absolute path to the source or compiled file for an object."""

Or does it always return *.py?

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/type_definitions.py:203:
os.path.dirname(os.path.dirname(test_file)),
This encodes some magic path layout? Can you write a comment somewhere about
expected layout.

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/type_definitions.py:212: # trim off the
'.py'
again, are you 100% sure it always *.py and not *.pyc?

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
File scripts/slave/unittests/expect_tests/util.py (right):

https://codereview.chromium.org/355143002/diff/1/scripts/slave/unittests/expe...
scripts/slave/unittests/expect_tests/util.py:11: func._covers =
coverage_path_function
You do hasattr(..., '_covers') somewhere. Mention this module there. 

Or even better, add another function here:

def get_coverage_path_function(func):
  return getattr(func, '_covers', None)

(maybe with less verbose name)

and use it instead of refering '_covers' directly.

Self-documenting code!..

[iannucci, 2014-06-28 08:58:18]

ptal

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
File scripts/slave/unittests/expect_tests/pipeline.py (right):

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/pipeline.py:71: if kill_switch.is_set():
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> Is it still needed here?

I guess not, but it doesn't hurt, especially if next(gen_inst) was expensive

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/pipeline.py:197: gen_cover =
test_gen._covers()  # pylint: disable=W0212
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> I forgot what type test_gen has here.  Why do you need 'hasattr' here? Also
why
> '_convers' instead of something without '_'?

it's a user function. Moved this all to util and used a better name.

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/pipeline.py:199: gen_cover =
[inspect.getabsfile(test_gen)]
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> Cover single file? Also, have you verified it works with both *.py and *.pyc?
> (Some people have local *.pyc generation disabled, some not).

It seems to always give back a .py file. I have pyc generation enabled on my
system.

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
File scripts/slave/unittests/expect_tests/type_definitions.py (right):

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/type_definitions.py:200: match =
Test.TEST_COVERS_MATCH.match(test_file)
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> *.pyc may not work here.
> 
> From getabsfile:
> """Return an absolute path to the source or compiled file for an object."""
> 
> Or does it always return *.py?

Seems to always return .py, but even if it didn't (say, returned pyc), that's
pretty useless for the coverage module anyway

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/type_definitions.py:203:
os.path.dirname(os.path.dirname(test_file)),
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> This encodes some magic path layout? Can you write a comment somewhere about
> expected layout.

Yeah, I wrote it in the drive doc... I'd like to doc this all in a README, but
expect_tests isn't really in it's own repo yet.

Basically, given

.../test/foo_test.py

It will cover
.../test/foo_test.py
.../foo.py

by default.

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/type_definitions.py:212: # trim off the
'.py'
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> again, are you 100% sure it always *.py and not *.pyc?

Looks like it can return either. Fixed.

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
File scripts/slave/unittests/expect_tests/util.py (right):

https://chromiumcodereview.appspot.com/355143002/diff/1/scripts/slave/unittes...
scripts/slave/unittests/expect_tests/util.py:11: func._covers =
coverage_path_function
On 2014/06/27 18:06:48, Vadim Sh. wrote:
> You do hasattr(..., '_covers') somewhere. Mention this module there. 
> 
> Or even better, add another function here:
> 
> def get_coverage_path_function(func):
>   return getattr(func, '_covers', None)
> 
> (maybe with less verbose name)
> 
> and use it instead of refering '_covers' directly.
> 
> Self-documenting code!..

Yep I did that

[Vadim Sh., 2014-06-30 17:29:11]

lgtm after _covers is fixed

also I lost distinction between this CL and
https://codereview.chromium.org/354913003/, rebases for in-flight CLs are evil.

https://codereview.chromium.org/355143002/diff/20001/scripts/slave/unittests/...
File scripts/slave/unittests/expect_tests/util.py (right):

https://codereview.chromium.org/355143002/diff/20001/scripts/slave/unittests/...
scripts/slave/unittests/expect_tests/util.py:25: return
test_gen_function._covers()  # pylint: disable=W0212
_covers? 

But judging by another CL, you already have this fixed in you local checkout...

[iannucci, 2014-06-30 22:47:02]

On 2014/06/30 17:29:11, Vadim Sh. wrote:
> lgtm after _covers is fixed
> 
> also I lost distinction between this CL and
> https://codereview.chromium.org/354913003/, rebases for in-flight CLs are
evil.

*sigh* yeah :( That's why I don't like having so many CLs live at the same time.
Makes life hard.

> 
>
https://codereview.chromium.org/355143002/diff/20001/scripts/slave/unittests/...
> File scripts/slave/unittests/expect_tests/util.py (right):
> 
>
https://codereview.chromium.org/355143002/diff/20001/scripts/slave/unittests/...
> scripts/slave/unittests/expect_tests/util.py:25: return
> test_gen_function._covers()  # pylint: disable=W0212
> _covers? 
> 
> But judging by another CL, you already have this fixed in you local
checkout...

Fixed in latest patchset... I think I actually fixed this in a downstream branch
so I backported the fix.

[iannucci, 2014-06-30 22:47:29]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2014-06-30 22:49:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iannucci@chromium.org/355143002/40001

[commit-bot: I haz the power, 2014-06-30 22:50:27]

Change committed as 280684