[Android] Redesigns the sharder to allow replicated vs distributed tests

[Android] Redesigns the sharder to allow replicated vs distributed tests

In addition, cleans up dispatch.

BUG=259063, 259128
NOTRY=True

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

[gkanwar, 2013-07-12 23:45:40]

This issue pulls out the sharding-related code changes from issue 18444004. Some
significant changes on the way, so please wait to review.

[gkanwar, 2013-07-15 23:27:04]

On 2013/07/12 23:45:40, gkanwar wrote:
> This issue pulls out the sharding-related code changes from issue 18444004.
Some
> significant changes on the way, so please wait to review.

This is now at a reviewable state.

[frankf, 2013-07-16 00:02:18]

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
File build/android/pylib/base/dispatch.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:5: """Implements test sharding logic."""
rename to test_dispatcher.py

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:352: """Distrbutes all tests over devices
through a shared pool of tests.
It's sufficient to say "Shards tests over devices"

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:364: if not tests:
There's a lot duplication between these two methods. You can delegate everything
to a private method and add ifs for distribution mode. Or just expose the
parameter like we discussed earlier.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:380: def _RunTests(test_collection_factory,
devices, runner_factory,
It's convention to move callee above caller

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
build/android/pylib/browsertests/setup.py:5: """Runs content_browsertests."""
Update these.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
build/android/pylib/browsertests/setup.py:45: constants.BROWSERTEST_SUITE_NAME +
'.apk')
indent issue

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
build/android/pylib/gtest/setup.py:255: deps_dir =
_GenerateDepsDirUsingIsolate(suite_name, build_type)
this won't work. You're just overwriting the deps dir in each iteration. It
makes sense to make -s mandatory in a separate CL as prerequisite.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/instru...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/instru...
build/android/pylib/instrumentation/test_runner.py:55: def __init__(self,
build_type, test_data, install_apk, save_perf_json,
For ever function signature you modify, make sure there are no dependencies
downstream.

https://codereview.chromium.org/18770008/diff/8001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/test_runner....
build/android/test_runner.py:144: # TODO(gkanwar): Make this option required
I was thinking that content_browsertests should just be gtest -s
content_browsertests

[gkanwar, 2013-07-16 00:47:03]

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
File build/android/pylib/base/dispatch.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:5: """Implements test sharding logic."""
On 2013/07/16 00:02:18, frankf wrote:
> rename to test_dispatcher.py

Done.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:352: """Distrbutes all tests over devices
through a shared pool of tests.
On 2013/07/16 00:02:18, frankf wrote:
> It's sufficient to say "Shards tests over devices"

Done.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:364: if not tests:
On 2013/07/16 00:02:18, frankf wrote:
> There's a lot duplication between these two methods. You can delegate
everything
> to a private method and add ifs for distribution mode. Or just expose the
> parameter like we discussed earlier.

I ended up combining the two methods back together, since they actually only
diverge slightly.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/base/d...
build/android/pylib/base/dispatch.py:380: def _RunTests(test_collection_factory,
devices, runner_factory,
On 2013/07/16 00:02:18, frankf wrote:
> It's convention to move callee above caller

Done.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
build/android/pylib/browsertests/setup.py:5: """Runs content_browsertests."""
On 2013/07/16 00:02:18, frankf wrote:
> Update these.

Done.

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/browse...
build/android/pylib/browsertests/setup.py:45: constants.BROWSERTEST_SUITE_NAME +
'.apk')
On 2013/07/16 00:02:18, frankf wrote:
> indent issue

Done.

https://codereview.chromium.org/18770008/diff/8001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/test_runner....
build/android/test_runner.py:144: # TODO(gkanwar): Make this option required
On 2013/07/16 00:02:18, frankf wrote:
> I was thinking that content_browsertests should just be gtest -s
> content_browsertests

That seems reasonable. I think there's some amount of setup/teardown that would
need to be moved, so it should probably happen in a different CL.

[gkanwar, 2013-07-16 17:44:36]

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
build/android/pylib/gtest/setup.py:255: deps_dir =
_GenerateDepsDirUsingIsolate(suite_name, build_type)
On 2013/07/16 00:02:18, frankf wrote:
> this won't work. You're just overwriting the deps dir in each iteration. It
> makes sense to make -s mandatory in a separate CL as prerequisite.

Moved dealing with no test suite specified into test_runner.py. This function
now assumes test_suite is specified, and we're only running one suite at a time.

[gkanwar, 2013-07-16 17:45:18]

On 2013/07/16 17:44:36, gkanwar wrote:
>
https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
> File build/android/pylib/gtest/setup.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/8001/build/android/pylib/gtest/...
> build/android/pylib/gtest/setup.py:255: deps_dir =
> _GenerateDepsDirUsingIsolate(suite_name, build_type)
> On 2013/07/16 00:02:18, frankf wrote:
> > this won't work. You're just overwriting the deps dir in each iteration. It
> > makes sense to make -s mandatory in a separate CL as prerequisite.
> 
> Moved dealing with no test suite specified into test_runner.py. This function
> now assumes test_suite is specified, and we're only running one suite at a
time.

I believe I've addressed all comments at this point, ptal when you get the
chance. I will take a look at what needs to be changed downstream.

[frankf, 2013-07-16 18:42:32]

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:5: """Implements test sharding
logic."""
Update

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:145: '%s_%s' % (runner.device,
test_result.GetName()),
Use last 4 digits of device serial.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:146: test_result.GetType(),
test_result.GetLog())
As the name suggest, BaseTestResult can be derived from. This doesn't work.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:166: # for replicated tests.
Is this comment misplaced?

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:313: assert test_device in
attached_devices
add a message to assert

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:325: test_allocation='shard',
I think a boolean called 'shard' makes more sense, and cuts down the validation
below.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:333: tests: list of tests to run.
Capital first letter.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:347: identify which device ran each
copy of the test, and to ensure each copy
Update this

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:353: # Validation
this is obvious. remove comment.

[frankf, 2013-07-16 18:51:34]

https://codereview.chromium.org/18770008/diff/20001/build/android/pylib/brows...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/20001/build/android/pylib/brows...
build/android/pylib/browsertests/setup.py:5: """Generate test runner factory and
tests for content_browsertests."""
Do a git move for these files to preserve history/diff

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:165: for test_suite in
gtest_config.STABLE_TEST_SUITES:
print the name not the tuple

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:371: results.AddTestRunResults(test_results)
be consistant with whitespace

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:375: report_results.LogFull(
Was the old behaviour to tally the results?

[gkanwar, 2013-07-16 20:27:31]

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:5: """Implements test sharding
logic."""
On 2013/07/16 18:42:32, frankf wrote:
> Update

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:145: '%s_%s' % (runner.device,
test_result.GetName()),
On 2013/07/16 18:42:32, frankf wrote:
> Use last 4 digits of device serial.

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:146: test_result.GetType(),
test_result.GetLog())
On 2013/07/16 18:42:32, frankf wrote:
> As the name suggest, BaseTestResult can be derived from. This doesn't work.

Updated to move tag into the BaseTestResult class itself.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:166: # for replicated tests.
On 2013/07/16 18:42:32, frankf wrote:
> Is this comment misplaced?

Oops, removed.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:313: assert test_device in
attached_devices
On 2013/07/16 18:42:32, frankf wrote:
> add a message to assert

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:325: test_allocation='shard',
On 2013/07/16 18:42:32, frankf wrote:
> I think a boolean called 'shard' makes more sense, and cuts down the
validation
> below.

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:333: tests: list of tests to run.
On 2013/07/16 18:42:32, frankf wrote:
> Capital first letter. 

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:347: identify which device ran each
copy of the test, and to ensure each copy
On 2013/07/16 18:42:32, frankf wrote:
> Update this

Done.

https://codereview.chromium.org/18770008/diff/12021/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:353: # Validation
On 2013/07/16 18:42:32, frankf wrote:
> this is obvious. remove comment.

Done.

[gkanwar, 2013-07-16 20:48:32]

https://codereview.chromium.org/18770008/diff/20001/build/android/pylib/brows...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/20001/build/android/pylib/brows...
build/android/pylib/browsertests/setup.py:5: """Generate test runner factory and
tests for content_browsertests."""
On 2013/07/16 18:51:34, frankf wrote:
> Do a git move for these files to preserve history/diff

Looks like passing --similarity 15 got it to recognize the move correctly.

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:165: for test_suite in
gtest_config.STABLE_TEST_SUITES:
On 2013/07/16 18:51:34, frankf wrote:
> print the name not the tuple

Done.

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:371: results.AddTestRunResults(test_results)
On 2013/07/16 18:51:34, frankf wrote:
> be consistant with whitespace

Done.

https://codereview.chromium.org/18770008/diff/20001/build/android/test_runner...
build/android/test_runner.py:375: report_results.LogFull(
On 2013/07/16 18:51:34, frankf wrote:
> Was the old behaviour to tally the results?

Ah, looks like the old behavior was to log each suite separately... fixed.

[frankf, 2013-07-17 04:07:20]

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/base_test_result.py:25: def __init__(self, name,
test_type, tag='', log=''):
Why all this complexity? Just update the name using SetName()

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:5: """Dispatches tests, either
sharding or replicating them."""
Expand this a little about. Include the fact, this dispatches tests by assigning
pools of tests which are run by test runners.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:139: # Used to tag all results to
identify which device caused failing tests
This is misleading. This also tags passing tests, although it's not displayed.
The main reason for this is to have unique test resuts as we use a set.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:140: def
TagTestRunResults(test_run_results):
Move comment here.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:213: the test was run to the test
name. Used by ReplicateAndRunTests to
Please address all comments. This needs to be update since there's no
ReplicateAndRunTests.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:330: TestRunner object.
I think we use 2 spaces for indentation or align it with 'Callable'.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:333: shard: True if we should shard,
False if we should replicate tests.
Expand this to define replicate.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:335: test_timeout: Watchdog timeout
in seconds for running tests, defaults to the
Remove "defaults to default timeout".

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher_unittest.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:123: """Tests for
_RunAllTests and _CreateRunners."""
Be consistant with including the module name

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:166: """Tests for
test_dispatcher.ShardAndRunTests."""
Update comment

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:191: """Tests for
test_dispatcher.ReplicateAndRunTests."""
Same here

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (left):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:300: test_timeout=None,
num_retries=options.num_retries)
What's the difference between test_timeout and options.timeout above?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:141: suite_name: the suite name specified on
the command line.
Capital first letter. Also in other places.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:141: suite_name: the suite name specified on
the command line.
Sometimes this is called test_suite, other places suite_name. Make sure we use
consistant name and docstring

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:163: name, path =
GetQualifiedSuite(test_suite)
is name same as suite_name?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:266: # TODO(gkanwar): Sharding shouldn't
happen here.
Why not? Test types are free to define what a 'test' is. In this case, it's a
collection of gtests.

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

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/test_runner.py:20: # dependencies here. Look at the
TODO in setup.py.
Rebase, this was removed

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
File build/android/pylib/uiautomator/setup.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
build/android/pylib/uiautomator/setup.py:19: def Setup(uiautomator_jar,
uiautomator_info_jar, annotations,
Make sure you run tests that don't run on trybots manually

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
build/android/pylib/uiautomator/setup.py:53: wait_for_debugger = False
just inline this below

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:51: def AddCommonOptions(option_parser,
default_timeout=60):
Just move the default down to the option

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:435: test_results, test_exit_code =
(python_dispatch.
Indent on next line

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:479: test_results, test_exit_code =
(python_dispatch.
Same here

[frankf, 2013-07-17 06:02:17]

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (left):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:266: if not
ports.ResetTestServerPortAllocation():
What happened to the calls to these?

[gkanwar, 2013-07-17 20:31:26]

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/base_test_result.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/base_test_result.py:25: def __init__(self, name,
test_type, tag='', log=''):
On 2013/07/17 04:07:20, frankf wrote:
> Why all this complexity? Just update the name using SetName()

We can't make this object mutable, since it will be going into a set, and
especially since the hash is based on the name. I could have just made the code
recreate the object and change the name, but I feel like this is cleaner, and
will allow us to group the tests that are the same name but on different
devices, if we should need that for logging in the future.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:5: """Dispatches tests, either
sharding or replicating them."""
On 2013/07/17 04:07:20, frankf wrote:
> Expand this a little about. Include the fact, this dispatches tests by
assigning
> pools of tests which are run by test runners.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:139: # Used to tag all results to
identify which device caused failing tests
On 2013/07/17 04:07:20, frankf wrote:
> This is misleading. This also tags passing tests, although it's not displayed.
> The main reason for this is to have unique test resuts as we use a set.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:140: def
TagTestRunResults(test_run_results):
On 2013/07/17 04:07:20, frankf wrote:
> Move comment here.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:213: the test was run to the test
name. Used by ReplicateAndRunTests to
On 2013/07/17 04:07:20, frankf wrote:
> Please address all comments. This needs to be update since there's no
> ReplicateAndRunTests.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:330: TestRunner object.
On 2013/07/17 04:07:20, frankf wrote:
> I think we use 2 spaces for indentation or align it with 'Callable'.

According to go/pyguide it's 4 spaces:
http://www.corp.google.com/eng/doc/pyguide.xml#Comments

Is this different in the Chromium tree?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:333: shard: True if we should shard,
False if we should replicate tests.
On 2013/07/17 04:07:20, frankf wrote:
> Expand this to define replicate.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:335: test_timeout: Watchdog timeout
in seconds for running tests, defaults to the
On 2013/07/17 04:07:20, frankf wrote:
> Remove "defaults to default timeout".

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher_unittest.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:123: """Tests for
_RunAllTests and _CreateRunners."""
On 2013/07/17 04:07:20, frankf wrote:
> Be consistant with including the module name

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:166: """Tests for
test_dispatcher.ShardAndRunTests."""
On 2013/07/17 04:07:20, frankf wrote:
> Update comment

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher_unittest.py:191: """Tests for
test_dispatcher.ReplicateAndRunTests."""
On 2013/07/17 04:07:20, frankf wrote:
> Same here

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (left):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:266: if not
ports.ResetTestServerPortAllocation():
On 2013/07/17 06:02:17, frankf wrote:
> What happened to the calls to these?

I think I removed these because I thought we were going to make the -s switch
required, in which case one call in test_runner.py would have been sufficient.
On second thought, I think this makes sense to stay in Setup(), so I'll add this
back.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:300: test_timeout=None,
num_retries=options.num_retries)
On 2013/07/17 04:07:20, frankf wrote:
> What's the difference between test_timeout and options.timeout above?

Looks like test_timeout sets the watchdog timeout. This is the timeout for
thread inactivity in the _RunAllTests function. If no thread has started a new
test for that period of time, the JoinAll function will raise a TimeoutError. It
looks like this setting was intended to be set per test_type, not as a
user-facing setting. I can set this back to None for all test types, but in that
case I'm not sure what purpose it's serving, so maybe it should just be removed?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:141: suite_name: the suite name specified on
the command line.
On 2013/07/17 04:07:20, frankf wrote:
> Capital first letter. Also in other places.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:141: suite_name: the suite name specified on
the command line.
On 2013/07/17 04:07:20, frankf wrote:
> Capital first letter. Also in other places.

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:163: name, path =
GetQualifiedSuite(test_suite)
On 2013/07/17 04:07:20, frankf wrote:
> is name same as suite_name?

Reworked to remove unnecessary code. This line doesn't exist any more.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:266: # TODO(gkanwar): Sharding shouldn't
happen here.
On 2013/07/17 04:07:20, frankf wrote:
> Why not? Test types are free to define what a 'test' is. In this case, it's a
> collection of gtests.

I think it's fine for test types to determine what their 'test' is, but in this
case, we're specifically breaking up tests based on the number of devices, which
makes me feel like this is treading on the toes of test_dispatcher. You
mentioned that you were refactoring gtests, however? Would that refactor affect
this?

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

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/test_runner.py:20: # dependencies here. Look at the
TODO in setup.py.
On 2013/07/17 04:07:20, frankf wrote:
> Rebase, this was removed

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
File build/android/pylib/uiautomator/setup.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
build/android/pylib/uiautomator/setup.py:19: def Setup(uiautomator_jar,
uiautomator_info_jar, annotations,
On 2013/07/17 04:07:20, frankf wrote:
> Make sure you run tests that don't run on trybots manually

Will do.

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/uiaut...
build/android/pylib/uiautomator/setup.py:53: wait_for_debugger = False
On 2013/07/17 04:07:20, frankf wrote:
> just inline this below

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:51: def AddCommonOptions(option_parser,
default_timeout=60):
On 2013/07/17 04:07:20, frankf wrote:
> Just move the default down to the option

Done.

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:435: test_results, test_exit_code =
(python_dispatch.
On 2013/07/17 04:07:20, frankf wrote:
> Indent on next line

By this you mean move the python_dispatch to the next line indented? If so,
done.

https://codereview.chromium.org/18770008/diff/28001/build/android/test_runner...
build/android/test_runner.py:479: test_results, test_exit_code =
(python_dispatch.
On 2013/07/17 04:07:20, frankf wrote:
> Same here

Done.

[frankf, 2013-07-17 21:08:40]

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:330: TestRunner object.
We diverge from the style guide for somethings. Let's be consistant everywhere.
We can always do a mass style fix later.

On 2013/07/17 20:31:26, gkanwar wrote:
> On 2013/07/17 04:07:20, frankf wrote:
> > I think we use 2 spaces for indentation or align it with 'Callable'.
> 
> According to go/pyguide it's 4 spaces:
> http://www.corp.google.com/eng/doc/pyguide.xml#Comments
> 
> Is this different in the Chromium tree?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (left):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:300: test_timeout=None,
num_retries=options.num_retries)
Let's not touch this in this CL and revisit it.

On 2013/07/17 20:31:26, gkanwar wrote:
> On 2013/07/17 04:07:20, frankf wrote:
> > What's the difference between test_timeout and options.timeout above?
> 
> Looks like test_timeout sets the watchdog timeout. This is the timeout for
> thread inactivity in the _RunAllTests function. If no thread has started a new
> test for that period of time, the JoinAll function will raise a TimeoutError.
It
> looks like this setting was intended to be set per test_type, not as a
> user-facing setting. I can set this back to None for all test types, but in
that
> case I'm not sure what purpose it's serving, so maybe it should just be
removed?

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/28001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:266: # TODO(gkanwar): Sharding shouldn't
happen here.
test_dispatcher doesn't know what a 'test' is. This is an optimization that
gtests do since it's very inefficient for each single test to start/kill a
process (there are thousands of tests in some suites).

On 2013/07/17 20:31:26, gkanwar wrote:
> On 2013/07/17 04:07:20, frankf wrote:
> > Why not? Test types are free to define what a 'test' is. In this case, it's
a
> > collection of gtests.
> 
> I think it's fine for test types to determine what their 'test' is, but in
this
> case, we're specifically breaking up tests based on the number of devices,
which
> makes me feel like this is treading on the toes of test_dispatcher. You
> mentioned that you were refactoring gtests, however? Would that refactor
affect
> this?

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:16: generated from the test
collection factory until there are no tests left.
This should be a high level overview of what this module does. Not specific
instructions how to use methods here.

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
build/android/pylib/browsertests/setup.py:47: suite_dir =
os.path.join(cmd_helper.OutDirectory.get(), build_type)
Get rid of intermediate var

[gkanwar, 2013-07-17 21:16:50]

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:16: generated from the test
collection factory until there are no tests left.
On 2013/07/17 21:08:41, frankf wrote:
> This should be a high level overview of what this module does. Not specific
> instructions how to use methods here.

This is a high-level overview of what the module does. It lists the things done
in this module, not what to do to use the module. I could make it more
high-level perhaps (or change the phrasing slightly)?

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
File build/android/pylib/browsertests/setup.py (right):

https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
build/android/pylib/browsertests/setup.py:47: suite_dir =
os.path.join(cmd_helper.OutDirectory.get(), build_type)
On 2013/07/17 21:08:41, frankf wrote:
> Get rid of intermediate var

Done.

[gkanwar, 2013-07-17 22:39:06]

On 2013/07/17 21:16:50, gkanwar wrote:
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
> File build/android/pylib/base/test_dispatcher.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
> build/android/pylib/base/test_dispatcher.py:16: generated from the test
> collection factory until there are no tests left.
> On 2013/07/17 21:08:41, frankf wrote:
> > This should be a high level overview of what this module does. Not specific
> > instructions how to use methods here.
> 
> This is a high-level overview of what the module does. It lists the things
done
> in this module, not what to do to use the module. I could make it more
> high-level perhaps (or change the phrasing slightly)?
> 
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
> File build/android/pylib/browsertests/setup.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
> build/android/pylib/browsertests/setup.py:47: suite_dir =
> os.path.join(cmd_helper.OutDirectory.get(), build_type)
> On 2013/07/17 21:08:41, frankf wrote:
> > Get rid of intermediate var
> 
> Done.

Please hold off review on the latest PS for a few minutes, I'm making a few more
fixes.

[gkanwar, 2013-07-17 22:41:40]

On 2013/07/17 22:39:06, gkanwar wrote:
> On 2013/07/17 21:16:50, gkanwar wrote:
> >
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
> > File build/android/pylib/base/test_dispatcher.py (right):
> > 
> >
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/base/...
> > build/android/pylib/base/test_dispatcher.py:16: generated from the test
> > collection factory until there are no tests left.
> > On 2013/07/17 21:08:41, frankf wrote:
> > > This should be a high level overview of what this module does. Not
specific
> > > instructions how to use methods here.
> > 
> > This is a high-level overview of what the module does. It lists the things
> done
> > in this module, not what to do to use the module. I could make it more
> > high-level perhaps (or change the phrasing slightly)?
> > 
> >
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
> > File build/android/pylib/browsertests/setup.py (right):
> > 
> >
>
https://codereview.chromium.org/18770008/diff/46001/build/android/pylib/brows...
> > build/android/pylib/browsertests/setup.py:47: suite_dir =
> > os.path.join(cmd_helper.OutDirectory.get(), build_type)
> > On 2013/07/17 21:08:41, frankf wrote:
> > > Get rid of intermediate var
> > 
> > Done.
> 
> Please hold off review on the latest PS for a few minutes, I'm making a few
more
> fixes.

Made some fixes to running python tests. I'll keep testing out the various
command line flags, but in the meanwhile feel free to review the latest patch.

[frankf, 2013-07-17 22:52:50]

lgtm

Please wait for Craig.

[craigdh, 2013-07-17 23:10:25]

lgtm w/ nits.

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:350: shard: True if we should shard,
False if we should replicate tests.
Do we not want to make this True by default?

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
File build/android/pylib/host_driven/python_test_base.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
build/android/pylib/host_driven/python_test_base.py:5: """Base class for Android
Python-driven tests.
While changing all this, should we rename this file to match the current naming
convention of using "host" instead of "python"?

https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner...
build/android/test_runner.py:536: raise Exception('Unknown test type state')
why the word "state"?

[gkanwar, 2013-07-17 23:24:19]

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
File build/android/pylib/base/test_dispatcher.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
build/android/pylib/base/test_dispatcher.py:350: shard: True if we should shard,
False if we should replicate tests.
On 2013/07/17 23:10:25, craigdh wrote:
> Do we not want to make this True by default?

My original thought on this was that anything that uses RunTests should have to
explicitly state whether it was sharding or replicating, since it seemed like a
very fundamental option. Since most test types just want sharding, however,
changing this makes sense. It also gives the added benefit of clarifying on the
caller side (which was something I was worried about with making this a boolean)
by forcing 'shard=True' as the argument, instead of just 'True'.

Based on that, updated to have True as the default.

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
File build/android/pylib/host_driven/python_test_base.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
build/android/pylib/host_driven/python_test_base.py:5: """Base class for Android
Python-driven tests.
On 2013/07/17 23:10:25, craigdh wrote:
> While changing all this, should we rename this file to match the current
naming
> convention of using "host" instead of "python"?

I plan to rename several host-driven files in the host-driven refactor, which
will be a different CL. For the sake of not changing too many things, I would
rather leave this as-is for this CL.

https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner...
build/android/test_runner.py:536: raise Exception('Unknown test type state')
On 2013/07/17 23:10:25, craigdh wrote:
> why the word "state"?

That's a good question... Fixed.

[gkanwar, 2013-07-17 23:25:35]

On 2013/07/17 23:24:19, gkanwar wrote:
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> File build/android/pylib/base/test_dispatcher.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> build/android/pylib/base/test_dispatcher.py:350: shard: True if we should
shard,
> False if we should replicate tests.
> On 2013/07/17 23:10:25, craigdh wrote:
> > Do we not want to make this True by default?
> 
> My original thought on this was that anything that uses RunTests should have
to
> explicitly state whether it was sharding or replicating, since it seemed like
a
> very fundamental option. Since most test types just want sharding, however,
> changing this makes sense. It also gives the added benefit of clarifying on
the
> caller side (which was something I was worried about with making this a
boolean)
> by forcing 'shard=True' as the argument, instead of just 'True'.
> 
> Based on that, updated to have True as the default.
> 
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> File build/android/pylib/host_driven/python_test_base.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> build/android/pylib/host_driven/python_test_base.py:5: """Base class for
Android
> Python-driven tests.
> On 2013/07/17 23:10:25, craigdh wrote:
> > While changing all this, should we rename this file to match the current
> naming
> > convention of using "host" instead of "python"?
> 
> I plan to rename several host-driven files in the host-driven refactor, which
> will be a different CL. For the sake of not changing too many things, I would
> rather leave this as-is for this CL.
> 
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner.py
> File build/android/test_runner.py (right):
> 
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner...
> build/android/test_runner.py:536: raise Exception('Unknown test type state')
> On 2013/07/17 23:10:25, craigdh wrote:
> > why the word "state"?
> 
> That's a good question... Fixed.

Setting NOTRY=True. Will commit if the trybots look good.

[gkanwar, 2013-07-17 23:35:48]

On 2013/07/17 23:25:35, gkanwar wrote:
> On 2013/07/17 23:24:19, gkanwar wrote:
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> > File build/android/pylib/base/test_dispatcher.py (right):
> > 
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> > build/android/pylib/base/test_dispatcher.py:350: shard: True if we should
> shard,
> > False if we should replicate tests.
> > On 2013/07/17 23:10:25, craigdh wrote:
> > > Do we not want to make this True by default?
> > 
> > My original thought on this was that anything that uses RunTests should have
> to
> > explicitly state whether it was sharding or replicating, since it seemed
like
> a
> > very fundamental option. Since most test types just want sharding, however,
> > changing this makes sense. It also gives the added benefit of clarifying on
> the
> > caller side (which was something I was worried about with making this a
> boolean)
> > by forcing 'shard=True' as the argument, instead of just 'True'.
> > 
> > Based on that, updated to have True as the default.
> > 
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> > File build/android/pylib/host_driven/python_test_base.py (right):
> > 
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> > build/android/pylib/host_driven/python_test_base.py:5: """Base class for
> Android
> > Python-driven tests.
> > On 2013/07/17 23:10:25, craigdh wrote:
> > > While changing all this, should we rename this file to match the current
> > naming
> > > convention of using "host" instead of "python"?
> > 
> > I plan to rename several host-driven files in the host-driven refactor,
which
> > will be a different CL. For the sake of not changing too many things, I
would
> > rather leave this as-is for this CL.
> > 
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner.py
> > File build/android/test_runner.py (right):
> > 
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner...
> > build/android/test_runner.py:536: raise Exception('Unknown test type state')
> > On 2013/07/17 23:10:25, craigdh wrote:
> > > why the word "state"?
> > 
> > That's a good question... Fixed.
> 
> Setting NOTRY=True. Will commit if the trybots look good.

(Waiting until tomorrow to land this actually)

[gkanwar, 2013-07-18 17:37:04]

On 2013/07/17 23:35:48, gkanwar wrote:
> On 2013/07/17 23:25:35, gkanwar wrote:
> > On 2013/07/17 23:24:19, gkanwar wrote:
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> > > File build/android/pylib/base/test_dispatcher.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/base/...
> > > build/android/pylib/base/test_dispatcher.py:350: shard: True if we should
> > shard,
> > > False if we should replicate tests.
> > > On 2013/07/17 23:10:25, craigdh wrote:
> > > > Do we not want to make this True by default?
> > > 
> > > My original thought on this was that anything that uses RunTests should
have
> > to
> > > explicitly state whether it was sharding or replicating, since it seemed
> like
> > a
> > > very fundamental option. Since most test types just want sharding,
however,
> > > changing this makes sense. It also gives the added benefit of clarifying
on
> > the
> > > caller side (which was something I was worried about with making this a
> > boolean)
> > > by forcing 'shard=True' as the argument, instead of just 'True'.
> > > 
> > > Based on that, updated to have True as the default.
> > > 
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> > > File build/android/pylib/host_driven/python_test_base.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/pylib/host_...
> > > build/android/pylib/host_driven/python_test_base.py:5: """Base class for
> > Android
> > > Python-driven tests.
> > > On 2013/07/17 23:10:25, craigdh wrote:
> > > > While changing all this, should we rename this file to match the current
> > > naming
> > > > convention of using "host" instead of "python"?
> > > 
> > > I plan to rename several host-driven files in the host-driven refactor,
> which
> > > will be a different CL. For the sake of not changing too many things, I
> would
> > > rather leave this as-is for this CL.
> > > 
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner.py
> > > File build/android/test_runner.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18770008/diff/67002/build/android/test_runner...
> > > build/android/test_runner.py:536: raise Exception('Unknown test type
state')
> > > On 2013/07/17 23:10:25, craigdh wrote:
> > > > why the word "state"?
> > > 
> > > That's a good question... Fixed.
> > 
> > Setting NOTRY=True. Will commit if the trybots look good.
> 
> (Waiting until tomorrow to land this actually)

Rebased onto Frank's patch. Waiting on trybots, then will land this.

[frankf, 2013-07-18 18:08:47]

Some nits. Still lgtm.

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:150: """Get a list of absolute paths to test
suite targets.
This needs to be update. I'll also change the name to something more descriptive

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:185: test_suite: Name of the test suite (e.g.
base_unittests).
It seems the names have diverged again :)

[gkanwar, 2013-07-18 18:48:10]

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:150: """Get a list of absolute paths to test
suite targets.
On 2013/07/18 18:08:48, frankf wrote:
> This needs to be update. I'll also change the name to something more
descriptive

Done.

https://codereview.chromium.org/18770008/diff/98001/build/android/pylib/gtest...
build/android/pylib/gtest/setup.py:185: test_suite: Name of the test suite (e.g.
base_unittests).
On 2013/07/18 18:08:48, frankf wrote:
> It seems the names have diverged again :)

Done.

[commit-bot: I haz the power, 2013-07-18 20:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@google.com/18770008/114001

[gkanwar, 2013-07-19 16:31:30]

On 2013/07/18 20:36:31, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/gkanwar%40google.com/18770008/114001

It looks like all relevant bots are green (fyi_dbg_triggered has been
consistently failing for external reasons), I'm going to CQ this.

[commit-bot: I haz the power, 2013-07-19 16:31:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@google.com/18770008/110002

[commit-bot: I haz the power, 2013-07-19 16:31:56]

Failed to apply patch for build/android/pylib/base/test_dispatcher.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         build/android/pylib/base/test_dispatcher.py
  Copied build/android/pylib/base/shard.py ->
build/android/pylib/base/test_dispatcher.py
  patching file build/android/pylib/base/test_dispatcher.py
  Hunk #2 succeeded at 56 (offset -1 lines).
  Hunk #3 succeeded at 67 (offset -1 lines).
  Hunk #4 succeeded at 129 (offset -1 lines).
  Hunk #5 succeeded at 141 (offset -1 lines).
  Hunk #6 succeeded at 169 (offset -1 lines).
  Hunk #7 succeeded at 199 (offset -1 lines).
  Hunk #8 succeeded at 215 (offset -1 lines).
  Hunk #9 succeeded at 269 (offset -1 lines).
  Hunk #10 succeeded at 294 (offset -1 lines).
  Hunk #11 succeeded at 304 (offset -1 lines).
  Hunk #12 FAILED at 366.
  1 out of 12 hunks FAILED -- saving rejects to file
build/android/pylib/base/test_dispatcher.py.rej

Patch:   NR 
build/android/pylib/base/shard.py->build/android/pylib/base/test_dispatcher.py
Index: build/android/pylib/base/test_dispatcher.py
diff --git a/build/android/pylib/base/shard.py
b/build/android/pylib/base/test_dispatcher.py
similarity index 58%
rename from build/android/pylib/base/shard.py
rename to build/android/pylib/base/test_dispatcher.py
index
8c429f7d3d01a9df148d25089260e9f085f50724..b45dab6a4360fb9ddbae98c176f9816033de8781
100644
--- a/build/android/pylib/base/shard.py
+++ b/build/android/pylib/base/test_dispatcher.py
@@ -2,7 +2,19 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
-"""Implements test sharding logic."""
+"""Dispatches tests, either sharding or replicating them.
+
+To dispatch, performs the following steps:
+* Create a test collection factory, using the given tests
+  - If sharding: test collection factory returns the same shared test
collection
+    to all test runners
+  - If replciating: test collection factory returns a unique test collection to
+    each test runner, with the same set of tests in each.
+* Get the list of devices to run on
+* Create test runners
+* Run each test runner in its own thread, pulling tests from the test
collection
+  generated from the test collection factory until there are no tests left.
+"""
 
 import logging
 import threading
@@ -45,8 +57,8 @@ class _Test(object):
     """Initializes the _Test object.
 
     Args:
-      test: the test.
-      tries: number of tries so far.
+      test: The test.
+      tries: Number of tries so far.
     """
     self.test = test
     self.tries = tries
@@ -56,7 +68,7 @@ class _TestCollection(object):
   """A threadsafe collection of tests.
 
   Args:
-    tests: list of tests to put in the collection.
+    tests: List of tests to put in the collection.
   """
 
   def __init__(self, tests=[]):
@@ -118,7 +130,7 @@ class _TestCollection(object):
 
 
 def _RunTestsFromQueue(runner, test_collection, out_results, watcher,
-                       num_retries):
+                       num_retries, tag_results_with_device=False):
   """Runs tests from the test_collection until empty using the given runner.
 
   Adds TestRunResults objects to the out_results list and may add tests to the
@@ -130,7 +142,25 @@ def _RunTestsFromQueue(runner, test_collection,
out_results, watcher,
     out_results: A list to add TestRunResults to.
     watcher: A watchdog_timer.WatchdogTimer object, used as a shared timeout.
     num_retries: Number of retries for a test.
+    tag_results_with_device: If True, appends the name of the device on which
+        the test was run to the test name. Used when replicating to identify
+        which device ran each copy of the test, and to ensure each copy of the
+        test is recorded separately.
   """
+
+  def TagTestRunResults(test_run_results):
+    """Tags all results with the last 4 digits of the device id.
+
+    Used when replicating tests to distinguish the same tests run on different
+    devices. We use a set to store test results, so the hash (generated from
+    name and tag) must be unique to be considered different results.
+    """
+    new_test_run_results = base_test_result.TestRunResults()
+    for test_result in test_run_results.GetAll():
+      test_result.SetTag(runner.device[-4:])
+      new_test_run_results.AddResult(test_result)
+    return new_test_run_results
+
   for test in test_collection:
     watcher.Reset()
     try:
@@ -140,6 +170,8 @@ def _RunTestsFromQueue(runner, test_collection, out_results,
watcher,
         logging.warning(msg)
         raise android_commands.errors.DeviceUnresponsiveError(msg)
       result, retry = runner.RunTest(test.test)
+      if tag_results_with_device:
+        result = TagTestRunResults(result)
       test.tries += 1
       if retry and test.tries <= num_retries:
         # Retry non-passing results, only record passing results.
@@ -168,11 +200,11 @@ def _SetUp(runner_factory, device, out_runners,
threadsafe_counter):
     added to out_runners.
 
   Args:
-    runner_factory: callable that takes a device and index and returns a
+    runner_factory: Callable that takes a device and index and returns a
       TestRunner object.
-    device: the device serial number to set up.
-    out_runners: list to add the successfully set up TestRunner object.
-    threadsafe_counter: a _ThreadSafeCounter object used to get shard indices.
+    device: The device serial number to set up.
+    out_runners: List to add the successfully set up TestRunner object.
+    threadsafe_counter: A _ThreadSafeCounter object used to get shard indices.
   """
   try:
     index = threadsafe_counter.GetAndIncrement()
@@ -184,28 +216,34 @@ def _SetUp(runner_factory, device, out_runners,
threadsafe_counter):
     logging.warning('Failed to create shard for %s: [%s]', device, e)
 
 
-def _RunAllTests(runners, tests, num_retries, timeout=None):
+def _RunAllTests(runners, test_collection_factory, num_retries, timeout=None,
+                 tag_results_with_device=False):
   """Run all tests using the given TestRunners.
 
   Args:
-    runners: a list of TestRunner objects.
-    tests: a list of Tests to run using the given TestRunners.
-    num_retries: number of retries for a test.
-    timeout: watchdog timeout in seconds, defaults to the default timeout.
+    runners: A list of TestRunner objects.
+    test_collection_factory: A callable to generate a _TestCollection object
for
+        each test runner.
+    num_retries: Number of retries for a test.
+    timeout: Watchdog timeout in seconds.
+    tag_results_with_device: If True, appends the name of the device on which
+        the test was run to the test name. Used when replicating to identify
+        which device ran each copy of the test, and to ensure each copy of the
+        test is recorded separately.
 
   Returns:
     A tuple of (TestRunResults object, exit code)
   """
-  logging.warning('Running %s tests with %s test runners.' %
-                  (len(tests), len(runners)))
-  tests_collection = _TestCollection([_Test(t) for t in tests])
+  logging.warning('Running tests with %s test runners.' % (len(runners)))
   results = []
   exit_code = 0
   watcher = watchdog_timer.WatchdogTimer(timeout)
+
   workers = reraiser_thread.ReraiserThreadGroup(
       [reraiser_thread.ReraiserThread(
           _RunTestsFromQueue,
-          [r, tests_collection, results, watcher, num_retries],
+          [r, test_collection_factory(), results, watcher, num_retries,
+           tag_results_with_device],
           name=r.device[-4:])
        for r in runners])
   run_results = base_test_result.TestRunResults()
@@ -232,10 +270,10 @@ def _CreateRunners(runner_factory, devices, timeout=None):
     included in the returned list.
 
   Args:
-    runner_factory: callable that takes a device and index and returns a
+    runner_factory: Callable that takes a device and index and returns a
       TestRunner object.
-    devices: list of device serial numbers as strings.
-    timeout: watchdog timeout in seconds, defaults to the default timeout.
+    devices: List of device serial numbers as strings.
+    timeout: Watchdog timeout in seconds, defaults to the default timeout.
 
   Returns:
     A list of TestRunner objects.
@@ -257,8 +295,8 @@ def _TearDownRunners(runners, timeout=None):
   """Calls TearDown() for each test runner in parallel.
 
   Args:
-    runners: a list of TestRunner objects.
-    timeout: watchdog timeout in seconds, defaults to the default timeout.
+    runners: A list of TestRunner objects.
+    timeout: Watchdog timeout in seconds, defaults to the default timeout.
   """
   threads = reraiser_thread.ReraiserThreadGroup(
       [reraiser_thread.ReraiserThread(r.TearDown, name=r.device[-4:])
@@ -267,23 +305,59 @@ def _TearDownRunners(runners, timeout=None):
   threads.JoinAll(watchdog_timer.WatchdogTimer(timeout))
 
 
-def ShardAndRunTests(runner_factory, devices, tests, build_type='Debug',
-                     test_timeout=DEFAULT_TIMEOUT,
-                     setup_timeout=DEFAULT_TIMEOUT,
-                     num_retries=2):
+
+def _GetAttachedDevices(wait_for_debugger=False, test_device=None):
+  """Get all attached devices.
+
+  If we are using a debugger, limit to only one device.
+
+  Args:
+    wait_for_debugger: True if this run will use a debugger.
+    test_device: Name of a specific device to use.
+
+  Returns:
+    A list of attached devices.
+  """
+  attached_devices = []
+
+  attached_devices = android_commands.GetAttachedDevices()
+  if test_device:
+    assert (test_device in attached_devices,
+            'Did not find device %s among attached device. Attached devices:
%s'
+            % (test_device, ', '.join(attached_devices)))
+    attached_devices = [test_device]
+
+  if len(attached_devices) > 1 and wait_for_debugger:
+    logging.warning('Debugger can not be sharded, using first available
device')
+    attached_devices = attached_devices[:1]
+
+  return attached_devices
…
(message too large)

[gkanwar, 2013-07-19 16:33:39]

Looks like I have to rebase. Will do when I get into the office.

[gkanwar, 2013-07-19 17:08:58]

On 2013/07/19 16:33:39, gkanwar wrote:
> Looks like I have to rebase. Will do when I get into the office.

Rebased, waiting on try bot again.

Frank -- could you take a look at the isolate deps dir change I made last night
and confirm that it does the correct thing? I moved the deps dir cleanup out of
setup so it's not deleted before the tests are run.

[frankf, 2013-07-19 17:32:10]

Make sure you do git cl try -r <revision> since you're conflicting with LKGR.

https://codereview.chromium.org/18770008/diff/148002/build/android/test_runne...
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/148002/build/android/test_runne...
build/android/test_runner.py:110: option_parser.add_option('--gtest_filter',
dest='test_filter',
-f short form was removed at some point. Please add it back.

[gkanwar, 2013-07-19 17:34:36]

https://codereview.chromium.org/18770008/diff/148002/build/android/test_runne...
File build/android/test_runner.py (right):

https://codereview.chromium.org/18770008/diff/148002/build/android/test_runne...
build/android/test_runner.py:110: option_parser.add_option('--gtest_filter',
dest='test_filter',
On 2013/07/19 17:32:10, frankf wrote:
> -f short form was removed at some point. Please add it back.

Done.

[commit-bot: I haz the power, 2013-07-19 22:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gkanwar@google.com/18770008/155003

[commit-bot: I haz the power, 2013-07-19 22:08:40]

Change committed as 212663