[Android] Re-write the gtest TestRunner and introduce a new generic sharder.

[Android] Re-write the gtest TestRunner.

Introduces a new generic sharder and removes the gtest TestSharder. Other test types to follow.

BUG=167334, 176041, 176325
TEST=run_tests.py

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183635

[craigdh, 2013-02-15 23:25:31]

This isn't ready to be committed yet as the edge cases need to be more fully
tested and unittests written for shard.py.

[craigdh, 2013-02-16 02:08:48]

+cc nilesh

[craigdh, 2013-02-19 21:45:08]

Ready for review, ptal.

[frankf, 2013-02-19 22:48:50]

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:92: for device in devices:
Don't delete the TODOs :)

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:160: [':'.join(all_tests[i::num_devices])
for i in xrange(num_devices)]
This is getting too complicated for list comprehension. Also, when can 't' be
empty? Is it just a precaution?

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:177: self._cleanup_test_files,
Should we pass this down? Don't think it's used elsewhere.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:212: # TODO(craigdh): There is no
reason for this to be part of TestRunner.
Right, most of these methods should be in test_package.py which generally
doesn't have a good abstraction.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:255: # TODO(frankf): Do not break
TestResults encapsulation.
There's no point to having these lines in a finally block

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:261: retry = ':'.join([t.name for t in
test_results.GetAllBroken()])
As discussed offline, we probably don't want to retry failed tests, only
crashed/timedout/unknown. But this could be in a future CL.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard.py
File build/android/pylib/shard.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:103: except
android_commands.errors.DeviceUnresponsiveError as e:
Is there a point to this except, we catch DeviceUnresponsiveError on line 132 as
well.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:114: tests: list of Test objects to run.
tests is just a list of strings.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:115: build_type: either 'Debug' or 'Release'.
Side note: This is another global config that is trickled down into every
function.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:123: if not devices or tries > retries:
tries >= retires?

[nilesh, 2013-02-19 23:00:39]

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
build/android/pylib/base/shard.py:1: # Copyright (c) 2012 The Chromium Authors.
All rights reserved.
2013

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
build/android/pylib/base/shard.py:113: def Shard(runner_factory, devices, tests,
build_type='Debug', retries=3,
Maybe rename to ShardAndRunTests ?

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:61: """Get a list of test from a device,
excluding disabled tests.
list of tests

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:77: """Get all enabled tests and available
devices.
Why does it say "all enabled tests and available devices"?

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:80: then filters it again using the
diabled list on the host.
disabled.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:154: all_tests = [t for t in
options.gtest_filter.split(':') if t]
This is changing the current behavior right? We didn't use to shard if a test
filter is specified and now we will try to.

I dont think it matters much, but filter is only used when debugging or running
tests by developers, and they can always attach only one device to disable
sharding. Just wanted to check if this was intended.

[craigdh, 2013-02-19 23:38:30]

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:92: for device in devices:
On 2013/02/19 22:48:50, frankf wrote:
> Don't delete the TODOs :)

If you look closely you'll see I actually fixed your TODO. That's why it was
deleted :-)

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:160: [':'.join(all_tests[i::num_devices])
for i in xrange(num_devices)]
On 2013/02/19 22:48:50, frankf wrote:
> This is getting too complicated for list comprehension. Also, when can 't' be
> empty? Is it just a precaution?

I remember adding it because I was getting empty strings. I believe when there
are fewer tests than devices you will end up with empty strings (ex. provide a
very specific gtest_filter option with multiple devices attached. This wasn't an
issue before because we didn't shard when passed a filter).

I'll break it up into two lines for readability.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
File build/android/pylib/gtest/test_runner.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:212: # TODO(craigdh): There is no
reason for this to be part of TestRunner.
On 2013/02/19 22:48:50, frankf wrote:
> Right, most of these methods should be in test_package.py which generally
> doesn't have a good abstraction.

This actually just depends on TestPackage's test_suite_basename, which really
doesn't even need to be in TestPackage. I've seen this in a few places so it
deserves its own cl.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:255: # TODO(frankf): Do not break
TestResults encapsulation.
On 2013/02/19 22:48:50, frankf wrote:
> There's no point to having these lines in a finally block

Done. Good point.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/gtest/...
build/android/pylib/gtest/test_runner.py:261: retry = ':'.join([t.name for t in
test_results.GetAllBroken()])
On 2013/02/19 22:48:50, frankf wrote:
> As discussed offline, we probably don't want to retry failed tests, only
> crashed/timedout/unknown. But this could be in a future CL.

Yeah, I'd like to not change behavior like that in this cl or people might think
it's a bug an revert.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard.py
File build/android/pylib/shard.py (right):

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:103: except
android_commands.errors.DeviceUnresponsiveError as e:
On 2013/02/19 22:48:50, frankf wrote:
> Is there a point to this except, we catch DeviceUnresponsiveError on line 132
as
> well.

This is called before the try block that line 132 catches. Essentially we just
skip creating a TestRunner for any device that fails. Line 132 is for failures
that occur while running a test.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:114: tests: list of Test objects to run.
On 2013/02/19 22:48:50, frankf wrote:
> tests is just a list of strings.

Good catch. Originally it wasn't going to be. Fixed.

https://codereview.chromium.org/12278020/diff/4002/build/android/pylib/shard....
build/android/pylib/shard.py:123: if not devices or tries > retries:
On 2013/02/19 22:48:50, frankf wrote:
> tries >= retires?

Depends, should retries count the first try or just retries?

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
build/android/pylib/base/shard.py:1: # Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> 2013

Done.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/base/s...
build/android/pylib/base/shard.py:113: def Shard(runner_factory, devices, tests,
build_type='Debug', retries=3,
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> Maybe rename to ShardAndRunTests ?

Done.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:61: """Get a list of test from a device,
excluding disabled tests.
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> list of tests

Done.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:77: """Get all enabled tests and available
devices.
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> Why does it say "all enabled tests and available devices"?

Done.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:80: then filters it again using the
diabled list on the host.
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> disabled.

Done.

https://codereview.chromium.org/12278020/diff/9001/build/android/pylib/gtest/...
build/android/pylib/gtest/dispatch.py:154: all_tests = [t for t in
options.gtest_filter.split(':') if t]
On 2013/02/19 23:00:39, nileshagrawal1 wrote:
> This is changing the current behavior right? We didn't use to shard if a test
> filter is specified and now we will try to.
> 
> I dont think it matters much, but filter is only used when debugging or
running
> tests by developers, and they can always attach only one device to disable
> sharding. Just wanted to check if this was intended.

Yeah, it simplified the code and seemed like a good thing to support. People can
always pass the list of devices they want to use on the command line if they
don't want to use all attached devices.

[frankf, 2013-02-19 23:57:26]

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:92: """Creates a test runner for each device.
Expand to include throwing out devices.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:109: logging.critical('****Failed to create a
shard: [%s]', e)
This is warning not critical.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:114: runner_factory, devices, tests,
build_type='Debug', retries=3,
retries -> tries to avoid off-by-1.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:145: logging.critical('****Failed to run test:
[%s]', e)
warning not critical.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:147: final_results.ok += results.ok
Move to end of try.

[craigdh, 2013-02-20 00:05:25]

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:92: """Creates a test runner for each device.
On 2013/02/19 23:57:26, frankf wrote:
> Expand to include throwing out devices.

Done.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:109: logging.critical('****Failed to create a
shard: [%s]', e)
On 2013/02/19 23:57:26, frankf wrote:
> This is warning not critical.

Done.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:114: runner_factory, devices, tests,
build_type='Debug', retries=3,
On 2013/02/19 23:57:26, frankf wrote:
> retries -> tries to avoid off-by-1.

Done.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:145: logging.critical('****Failed to run test:
[%s]', e)
On 2013/02/19 23:57:26, frankf wrote:
> warning not critical.

Done.

https://codereview.chromium.org/12278020/diff/14003/build/android/pylib/base/...
build/android/pylib/base/shard.py:147: final_results.ok += results.ok
On 2013/02/19 23:57:26, frankf wrote:
> Move to end of try.

Done.

[frankf, 2013-02-20 18:55:31]

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:20: Places results in the results_list.
results_list -> out_results

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:40: """Run tests from the queue until it is
empty, storing results.
be more precise than "storing results" :)

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:42: Overriden from threading.Thread, runs in a
separate thread.
I think we do something like #Override before the method.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:58: """Reraise an exception raised in the
thread."""
Reraise _if_ an exception...

Maybe rename to better reflect the conditional

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:112: logging.warning('****Failed to create a
shard: [%s]', e)
Does this print the device that failed?

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:126: retries: number of retries before
accepting failure.
update this.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:127: get_attached_devices_callable: only
override default value for testing.
Can you use pymox to mock out the calls to adb instead?

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:139: logging.critical('No devices attached and
visible to run tests!')
msg = ...

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:141: if try_count >= tries:
Add some comments for these termination conditions

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:85: is_on_emulator: whether to exclude
tests disabled on the emulator.
This isn't used anywhere, can be removed.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:93: print 'trying device: %s' % device
we only use logging

[craigdh, 2013-02-20 19:38:09]

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:20: Places results in the results_list.
On 2013/02/20 18:55:31, frankf wrote:
> results_list -> out_results

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:40: """Run tests from the queue until it is
empty, storing results.
On 2013/02/20 18:55:31, frankf wrote:
> be more precise than "storing results" :)

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:42: Overriden from threading.Thread, runs in a
separate thread.
On 2013/02/20 18:55:31, frankf wrote:
> I think we do something like #Override before the method.

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:58: """Reraise an exception raised in the
thread."""
On 2013/02/20 18:55:31, frankf wrote:
> Reraise _if_ an exception...
> 
> Maybe rename to better reflect the conditional

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:112: logging.warning('****Failed to create a
shard: [%s]', e)
On 2013/02/20 18:55:31, frankf wrote:
> Does this print the device that failed?

I'm not sure if the exception message includes the device or just the reason for
the failure, but if not the device is listed two lines above where it says
'Creating shard NUM for DEVICE.'

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:126: retries: number of retries before
accepting failure.
On 2013/02/20 18:55:31, frankf wrote:
> update this.

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:127: get_attached_devices_callable: only
override default value for testing.
On 2013/02/20 18:55:31, frankf wrote:
> Can you use pymox to mock out the calls to adb instead?

Removed this, it was a trivial one-liner to mock out in the unittest. PyMox
looks neat, but it may be overkill here. If we find ourselves requring pymox
later on for more involved mocking it will be trivial to switch this out, but
for now I'd rather not put another road block between people
running/contributing unittests (as pymox has to be installed from source).

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:139: logging.critical('No devices attached and
visible to run tests!')
On 2013/02/20 18:55:31, frankf wrote:
> msg = ...

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/base/...
build/android/pylib/base/shard.py:141: if try_count >= tries:
On 2013/02/20 18:55:31, frankf wrote:
> Add some comments for these termination conditions 

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:85: is_on_emulator: whether to exclude
tests disabled on the emulator.
On 2013/02/20 18:55:31, frankf wrote:
> This isn't used anywhere, can be removed.

Done.

https://codereview.chromium.org/12278020/diff/13003/build/android/pylib/gtest...
build/android/pylib/gtest/dispatch.py:93: print 'trying device: %s' % device
On 2013/02/20 18:55:31, frankf wrote:
> we only use logging

Done.

[frankf, 2013-02-20 20:57:21]

lgtm

[craigdh, 2013-02-20 22:37:04]

Committed patchset #7 manually as r183635 (presubmit successful).