[Typ] Add support for sharding

[Typ] Add support for sharding

BUG=chromium:620959

[nednguyen, 2016-06-17 04:44:20]

PTAL

If the overall approach looks ok to you, I will add unittest.

[dpranke, 2016-06-17 05:12:00]

probably would've been better to do this on github, but no matter.

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py
File typ/runner.py (right):

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode93
typ/runner.py:93: (shard_index, total_shards))
These should just be errors that are returned, not assertions, since they come
from the user, and you should validate the args in
arg_parser.ArgumentParser.parse_args() instead of here.

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode99
typ/runner.py:99: return sharded_tests_list
you can do the actual sharding with:

  return _sort_inputs(tests_list)[shard_index::total_shards]

and I would actually just do that in the calling routine, e.g.:

test_set.parallel_tests =
_sort_inputs(test_set.parallel_tests)[args.shard_index::args.total_shards]

etc...

Also, use regular PEP-8 style for naming and indentation :).

[nednguyen, 2016-06-17 17:27:12]

PTAL again

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py
File typ/runner.py (right):

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode93
typ/runner.py:93: (shard_index, total_shards))
On 2016/06/17 05:12:00, dpranke wrote:
> These should just be errors that are returned, not assertions, since they come
> from the user, and you should validate the args in
> arg_parser.ArgumentParser.parse_args() instead of here.

Since the runner.args can be mutated directly before runner.run() is called.
(telemetry's run_test.py does it), I think it still makes sense to validate the
args here instead of arg_parser. wdyt?

https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode99
typ/runner.py:99: return sharded_tests_list
On 2016/06/17 05:12:00, dpranke wrote:
> you can do the actual sharding with:
> 
>   return _sort_inputs(tests_list)[shard_index::total_shards]
> 
> and I would actually just do that in the calling routine, e.g.:
> 
> test_set.parallel_tests =
> _sort_inputs(test_set.parallel_tests)[args.shard_index::args.total_shards]
> 
> etc...
> 
> Also, use regular PEP-8 style for naming and indentation :).

Done. This is much more pythonic :-P

[dpranke, 2016-06-17 17:35:23]

On 2016/06/17 17:27:12, nednguyen wrote:
> PTAL again
> 
> https://codereview.appspot.com/301970043/diff/40001/typ/runner.py
> File typ/runner.py (right):
> 
> https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode93
> typ/runner.py:93: (shard_index, total_shards))
> On 2016/06/17 05:12:00, dpranke wrote:
> > These should just be errors that are returned, not assertions, since they
come
> > from the user, and you should validate the args in
> > arg_parser.ArgumentParser.parse_args() instead of here.
> 
> Since the runner.args can be mutated directly before runner.run() is called.
> (telemetry's run_test.py does it), I think it still makes sense to validate
the
> args here instead of arg_parser. wdyt?

I'm fine w/ asserts in run(), since those would indicate programming errors, 
but you should still validate the input and handle bad arguments cleanly 
in parse_args(), since those can come from the user.

[nednguyen, 2016-06-17 17:51:43]

On 2016/06/17 17:35:23, dpranke wrote:
> On 2016/06/17 17:27:12, nednguyen wrote:
> > PTAL again
> > 
> > https://codereview.appspot.com/301970043/diff/40001/typ/runner.py
> > File typ/runner.py (right):
> > 
> > https://codereview.appspot.com/301970043/diff/40001/typ/runner.py#newcode93
> > typ/runner.py:93: (shard_index, total_shards))
> > On 2016/06/17 05:12:00, dpranke wrote:
> > > These should just be errors that are returned, not assertions, since they
> come
> > > from the user, and you should validate the args in
> > > arg_parser.ArgumentParser.parse_args() instead of here.
> > 
> > Since the runner.args can be mutated directly before runner.run() is called.
> > (telemetry's run_test.py does it), I think it still makes sense to validate
> the
> > args here instead of arg_parser. wdyt?
> 
> I'm fine w/ asserts in run(), since those would indicate programming errors, 
> but you should still validate the input and handle bad arguments cleanly 
> in parse_args(), since those can come from the user.

Done. PTAL again

[dpranke, 2016-06-17 19:35:13]

The changes to runner.py and arg_parser.py now look fine.

I appreciate that you went to a lot of effort to write tests for this, but they
don't actually fit the style of testing that typ does, and you built up a lot of
scaffolding that I think we can avoid because other scaffolding already exists.

Take a look at the tests that are in main_test.py . They're written in a much
more black-box acceptance style, but because typ fakes out all of the
interaction with the host, they're still stable and fast. 

You can probably get comparable coverage by writing a couple tests in
main_test.TestCli (see e.g., how test_find() tests the lists that are
discovered).

Does that make sense?

[nednguyen, 2016-06-17 20:36:03]

Done. That's much easier, thanks Dirk. PTAL

[dpranke, 2016-06-17 20:41:49]

close enough. I may rewrite the tests more later :).

lgtm. I'll apply the patch and roll it into src/.

[nednguyen, 2016-06-17 20:45:48]

On 2016/06/17 20:41:49, dpranke wrote:
> close enough. I may rewrite the tests more later :).
> 
> lgtm. I'll apply the patch and roll it into src/.

Thanks Dirk! Should I just close this review?

[dpranke, 2016-06-17 22:45:31]

On 2016/06/17 20:45:48, nednguyen wrote:
> On 2016/06/17 20:41:49, dpranke wrote:
> > close enough. I may rewrite the tests more later :).
> > 
> > lgtm. I'll apply the patch and roll it into src/.
> 
> Thanks Dirk! Should I just close this review?

Once I've landed it, sure.