Handle unused recipe expectations in new 'test' command

Handle unused recipe expectations in new 'test' command

BUG=chromium:699120
Review-Url: https://codereview.chromium.org/2758923002
Committed: https://github.com/luci/recipes-py/commit/e300f3a3b8193b84b88599faa30c0301e155c906

[Paweł Hajdan Jr., 2017-03-17 21:07:50]

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

[commit-bot: I haz the power, 2017-03-17 21:07:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 21:12:48]

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

[commit-bot: I haz the power, 2017-03-17 21:12:48]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2017-03-17 21:14:24]

phajdan.jr@chromium.org changed reviewers:
+ iannucci@chromium.org

[iannucci, 2017-03-17 22:04:20]

lgtm w/ caution around passing the actual result between processes

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

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:159: with open(test_description.expectation_path, 'w') as
f:
I think you may want to open 'wb' to prevent windows line endings

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:177: return TestResult(test_description, failures,
coverage_data, actual)
'actual' is probably large and will be slow to pickle; is there a subset of it
you can move over the multiprocess ipc?

edit: it looks like you're using it as a boolean?

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:403: full_subentry = os.path.join(full_entry, subentry)
only want subentry if it ends with .json. Otherwise you might pick up OWNERS
files, README.md, etc.

[iannucci, 2017-03-17 22:04:45]

Actually, if you ever need the actual result, it would be cheaper to read it
from the disk (which you should be able to do because the multiprocess shard
just wrote it there)

[Paweł Hajdan Jr., 2017-03-20 11:29:14]

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

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:159: with open(test_description.expectation_path, 'w') as
f:
On 2017/03/17 22:04:19, iannucci wrote:
> I think you may want to open 'wb' to prevent windows line endings

Done.

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:177: return TestResult(test_description, failures,
coverage_data, actual)
On 2017/03/17 22:04:19, iannucci wrote:
> 'actual' is probably large and will be slow to pickle; is there a subset of it
> you can move over the multiprocess ipc?
> 
> edit: it looks like you're using it as a boolean?

This didn't seem to have measurable effect on the performance (testing with
build repo). Done anyway.

FWIW, scanning for unused expectations does incur some hit, so if we are going
to be nitpicky about that we may want to put that behind a flag (enabled on the
bots of course).

https://codereview.chromium.org/2758923002/diff/1/recipe_engine/test.py#newco...
recipe_engine/test.py:403: full_subentry = os.path.join(full_entry, subentry)
On 2017/03/17 22:04:19, iannucci wrote:
> only want subentry if it ends with .json. Otherwise you might pick up OWNERS
> files, README.md, etc.

Good point, done. Also added tests.

[Paweł Hajdan Jr., 2017-03-20 11:29:18]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2017-03-20 11:29:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2758923002/#ps20001
(title: "fixes")

[commit-bot: I haz the power, 2017-03-20 11:29:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 11:36:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490009358799520,
"parent_rev": "78f105cbebb6e8dc99d64638d400868851cb0c40", "commit_rev":
"e300f3a3b8193b84b88599faa30c0301e155c906"}

[commit-bot: I haz the power, 2017-03-20 11:36:13]

Description was changed from

==========
Handle unused recipe expectations in new 'test' command

BUG=chromium:699120
==========

to

==========
Handle unused recipe expectations in new 'test' command

BUG=chromium:699120

Review-Url: https://codereview.chromium.org/2758923002
Committed:
https://github.com/luci/recipes-py/commit/e300f3a3b8193b84b88599faa30c0301e15...
==========

[commit-bot: I haz the power, 2017-03-20 11:36:13]

Committed patchset #2 (id:20001) as
https://github.com/luci/recipes-py/commit/e300f3a3b8193b84b88599faa30c0301e15...