test.py: compute coverage per top-level module.

test.py: compute coverage per top-level module.

This allows specifying minimum expected coverage per python module.

BUG=606878
R=pgervais@chromium.org

Committed: https://chromium.googlesource.com/infra/infra/+/0e47fe74a32e0030ba927470c4df050907d626ce

[Sergey Berezin, 2016-04-27 01:39:13]

sergeyberezin@chromium.org changed reviewers:
+ agable@chromium.org, lijeffrey@chromium.org, nodir@chromium.org

[Sergey Berezin, 2016-04-27 01:39:14]

PTAL.

+pgervais@chromium.org for test.py and overall design
+lijeffrey@chromium.org for findit 
+nodir@chromium.org for buildbucket 
+agable@chromium.org for monorail, mostly FYI (no code change)

This change depends on http://crrev.com/1915163005 being rolled into infra's
ENV.

All modules now have 100% expected coverage, except for monorail (82%) and
chromium_build (24%).

Though test.py now runs the modules sequentially, performance is identical to
the original on my workstation:

New version (all infra.git tests):
real	1m9.609s
user	5m3.142s
sys	1m20.402s

Old version:
real	1m9.420s
user	5m3.522s
sys	1m23.969s

[nodir, 2016-04-27 03:59:08]

buildbucket changes lgtm

[lijeffrey, 2016-04-27 05:56:34]

Findit lgtm, though we should have caught that we missed that line. (It still
reports "100.0%" coverage possibly rounded up without being truly 100% so was
probably not obvious without opening up the actual report)

I've made https://codereview.chromium.org/1926573002/ to test that branch. If
it's reviewed and committed first then the Findit modification here shouldn't be
necessary :)

[pgervais, 2016-04-27 16:41:16]

Looks good with a few comments

https://codereview.chromium.org/1917383002/diff/1/test.py
File test.py (right):

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode17
test.py:17: print """\nUsage: %s <action> [<test names>] [<expect_tests
options>]
The test/options order does not seem to matter. Is there a particular reason why
you inverted those?

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode25
test.py:25: ./test.py test infra --html-report /path/to/report/folder
Is the --html-report option mandatory?

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode106
test.py:106: if '--help' not in sys.argv and '-h' not in sys.argv:
'not in flags' instead?

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode120
test.py:120: exit_code = max(exit_code, subprocess.call(cmd))
'subprocess.call(cmd) or exit_code' instead

We want to return codes likes -1.

[Sergey Berezin, 2016-04-27 17:32:15]

https://codereview.chromium.org/1917383002/diff/1/test.py
File test.py (right):

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode17
test.py:17: print """\nUsage: %s <action> [<test names>] [<expect_tests
options>]
On 2016/04/27 16:41:16, pgervais wrote:
> The test/options order does not seem to matter. Is there a particular reason
why
> you inverted those?

It actually does now, since I need to know the list of modules to test, and have
to assume that module names come first. The reason is, I cannot tell the
difference between a module and an argument to a flag, e.g. 

./test.py test infra_libs --html-report mydir

without a full fledged argparse, it's hard to tell that mydir is not a module.
So I simply collect non-flag arguments before the first flag, and call them
modules.

It's a bit of a breaking change, but the alternative of enforcing
--html-report=mydir format is also suboptimal, since I can't use ~/path and
expect it to work.

FWIW, in the original version, ./test.py test --html-report blah did NOT work as
expected (it didn't test all the modules), but now works well in the new
version.

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode25
test.py:25: ./test.py test infra --html-report /path/to/report/folder
On 2016/04/27 16:41:16, pgervais wrote:
> Is the --html-report option mandatory?

No, I just added it as an example. I'll add another example without it, to be
clear.

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode106
test.py:106: if '--help' not in sys.argv and '-h' not in sys.argv:
On 2016/04/27 16:41:16, pgervais wrote:
> 'not in flags' instead?

Done. I also improved printing the usage info.

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode120
test.py:120: exit_code = max(exit_code, subprocess.call(cmd))
On 2016/04/27 16:41:16, pgervais wrote:
> 'subprocess.call(cmd) or exit_code' instead
> 
> We want to return codes likes -1.

Done.

Also printed the list of failing modules at the end of the run - otherwise
looking for one failure in the barrage of text is annoying.

[Sergey Berezin, 2016-04-27 17:34:08]

On 2016/04/27 05:56:34, lijeffrey wrote:
> Findit lgtm, though we should have caught that we missed that line. (It still
> reports "100.0%" coverage possibly rounded up without being truly 100% so was
> probably not obvious without opening up the actual report)
> 
> I've made https://codereview.chromium.org/1926573002/ to test that branch. If
> it's reviewed and committed first then the Findit modification here shouldn't
be
> necessary :)

Thanks for a fast fix! I rebased on top of your CL and removed the change. The
reason the coverage slipped is because we reduced it to 90% due to monorail, and
this CL fixes the situation by setting the coverage expectations per module
rather than globally.

[Sergey Berezin, 2016-04-27 17:36:33]

The CQ bit was checked by sergeyberezin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 17:36:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917383002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917383002/40001

[pgervais, 2016-04-27 17:39:52]

On 2016/04/27 17:36:37, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1917383002/40001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1917383002/40001

lgtm though I think the API is suboptimal, but there is not much we can do about
it short of making bigger changes to expect_tests.

[pgervais, 2016-04-27 17:40:00]

https://codereview.chromium.org/1917383002/diff/1/test.py
File test.py (right):

https://codereview.chromium.org/1917383002/diff/1/test.py#newcode17
test.py:17: print """\nUsage: %s <action> [<test names>] [<expect_tests
options>]
On 2016/04/27 17:32:14, Sergey Berezin wrote:
> On 2016/04/27 16:41:16, pgervais wrote:
> > The test/options order does not seem to matter. Is there a particular reason
> why
> > you inverted those?
> 
> It actually does now, since I need to know the list of modules to test, and
have
> to assume that module names come first. The reason is, I cannot tell the
> difference between a module and an argument to a flag, e.g. 
> 
> ./test.py test infra_libs --html-report mydir
> 
> without a full fledged argparse, it's hard to tell that mydir is not a module.
> So I simply collect non-flag arguments before the first flag, and call them
> modules.
> 
> It's a bit of a breaking change, but the alternative of enforcing
> --html-report=mydir format is also suboptimal, since I can't use ~/path and
> expect it to work.
> 
> FWIW, in the original version, ./test.py test --html-report blah did NOT work
as
> expected (it didn't test all the modules), but now works well in the new
> version.

I'm afraid this is only the beginning of a long list of weird corner cases :-(
This test.py/expect_tests combo is really suboptimal.

[commit-bot: I haz the power, 2016-04-27 17:41:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:41:08]

Dry run: This issue passed the CQ dry run.

[agable, 2016-04-27 18:38:47]

LGTM, please make the same changes to test.py in infra_internal to keep them in
sync.

[Sergey Berezin, 2016-04-27 18:44:40]

On 2016/04/27 18:38:47, agable wrote:
> LGTM, please make the same changes to test.py in infra_internal to keep them
in
> sync.

Thanks, I will.

[Sergey Berezin, 2016-04-27 18:44:50]

The CQ bit was checked by sergeyberezin@chromium.org

[Sergey Berezin, 2016-04-27 18:44:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
lijeffrey@chromium.org, nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1917383002/#ps40001
(title: "Addressing comments, and more polishing")

[commit-bot: I haz the power, 2016-04-27 18:44:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1917383002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1917383002/40001

[commit-bot: I haz the power, 2016-04-27 18:47:38]

Description was changed from

==========
test.py: compute coverage per top-level module.

This allows specifying minimum expected coverage per python module.

BUG=606878
R=pgervais@chromium.org
==========

to

==========
test.py: compute coverage per top-level module.

This allows specifying minimum expected coverage per python module.

BUG=606878
R=pgervais@chromium.org

Committed:
https://chromium.googlesource.com/infra/infra/+/0e47fe74a32e0030ba927470c4df0...
==========

[commit-bot: I haz the power, 2016-04-27 18:47:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/0e47fe74a32e0030ba927470c4df0...

[nodir, 2016-06-06 21:52:32]

This caused https://bugs.chromium.org/p/chromium/issues/detail?id=617775

[achami.o8i8.o2o2.i0o8, 2016-08-02 04:52:35]

On 2016/04/27 18:38:47, agable wrote:
>
<font><font>LGTMは、それらを保つためにinfra_internalにtest.pyするために同じ変更を行ってください</font></font><font></font><font><font>
> シンク。</font></font>

[achami.o8i8.o2o2.i0o8, 2016-08-11 06:25:24]

2016年8月2日(火) 13:52 <achami.o8i8.o2o2.i0o8@gmail.com>:

> On 2016/04/27 18:38:47, agable wrote:
> >
>
>
<font><font>LGTMは、それらを保つためにinfra_internalにtest.pyするために同じ変更を行ってください</font></font><font></font><font><font>
> > シンク。</font></font>
>
>
>
> https://codereview.chromium.org/1917383002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[adriansprowson, 2017-02-06 01:11:34]

adriansprowson@gmail.com changed reviewers:
+ adriansprowson@gmail.com

[adriansprowson, 2017-02-06 01:11:35]

lgtm