Makes host driven tests use the common sharder.

Makes host driven tests use the common sharder.

This also updates the sharder to give the option of 'distribute' vs
'duplicate' for running the given set of tests. 'distribute' uses the
current sharding method (create a pool of tests, have test runner draw
from this pool as they do each test). 'duplicate' is currently used only
for Monkey tests; it duplicates the tests across all test runners. This
is useful for Monkeys tests when we're running on a heterogeneous set of
devices.

In the future, we probably want to move the option to duplicate vs.
distribute to the buildbot config, since it is somewhat dependent on the
actual physics setup (e.g. we don't want to duplicate if all devices are
identical...)

BUG=176323, 259063, 259128

[gkanwar, 2013-07-12 18:04:53]

On 2013/07/12 17:46:48, gkanwar wrote:

Note: this will temporarily break Popular URL tests downstream.

[craigdh, 2013-07-12 18:14:14]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
The behavior is quite different, I think it would be better to expose two public
functions, something like: RunTestsDistributed() and RunTestsDuplicated().

Then you can put the divergent logic from lines 208-218 in those functions and
pass the lambda to a common implementation similar to the ShardAndRunTests you
have here, except with the lambda instead of the sharding string.

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:309: 
remove this new blank line

[gkanwar, 2013-07-12 18:22:41]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
On 2013/07/12 18:14:15, craigdh wrote:
> The behavior is quite different, I think it would be better to expose two
public
> functions, something like: RunTestsDistributed() and RunTestsDuplicated().
> 
> Then you can put the divergent logic from lines 208-218 in those functions and
> pass the lambda to a common implementation similar to the ShardAndRunTests you
> have here, except with the lambda instead of the sharding string.

I was discussing this with Frank in the gdoc, and I think we decided it was
overcomplicating it to create the additional two functions. I would be happy to
go either way though. Frank?

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:309: 
On 2013/07/12 18:14:15, craigdh wrote:
> remove this new blank line

Will do.

[frankf, 2013-07-12 18:49:19]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
I still think the semantics is confusing. Sharding is synonymous with
distributing. How about we call this class test_allocator.py and have two
functions:

ShardAndRunTests()
ReplicateAndRunTests()

On 2013/07/12 18:22:41, gkanwar wrote:
> On 2013/07/12 18:14:15, craigdh wrote:
> > The behavior is quite different, I think it would be better to expose two
> public
> > functions, something like: RunTestsDistributed() and RunTestsDuplicated().
> > 
> > Then you can put the divergent logic from lines 208-218 in those functions
and
> > pass the lambda to a common implementation similar to the ShardAndRunTests
you
> > have here, except with the lambda instead of the sharding string.
> 
> I was discussing this with Frank in the gdoc, and I think we decided it was
> overcomplicating it to create the additional two functions. I would be happy
to
> go either way though. Frank?

[gkanwar, 2013-07-12 18:52:03]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
On 2013/07/12 18:49:19, frankf wrote:
> I still think the semantics is confusing. Sharding is synonymous with
> distributing. How about we call this class test_allocator.py and have two
> functions:
> 
> ShardAndRunTests()
> ReplicateAndRunTests()
> 
> On 2013/07/12 18:22:41, gkanwar wrote:
> > On 2013/07/12 18:14:15, craigdh wrote:
> > > The behavior is quite different, I think it would be better to expose two
> > public
> > > functions, something like: RunTestsDistributed() and RunTestsDuplicated().
> > > 
> > > Then you can put the divergent logic from lines 208-218 in those functions
> and
> > > pass the lambda to a common implementation similar to the ShardAndRunTests
> you
> > > have here, except with the lambda instead of the sharding string.
> > 
> > I was discussing this with Frank in the gdoc, and I think we decided it was
> > overcomplicating it to create the additional two functions. I would be happy
> to
> > go either way though. Frank?
> 

That sounds reasonable to me. Craig, does that sound good to you?

[craigdh, 2013-07-12 19:03:36]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
On 2013/07/12 18:49:19, frankf wrote:
> I still think the semantics is confusing. Sharding is synonymous with
> distributing. How about we call this class test_allocator.py and have two
> functions:
> 
> ShardAndRunTests()
> ReplicateAndRunTests()
> 
> On 2013/07/12 18:22:41, gkanwar wrote:
> > On 2013/07/12 18:14:15, craigdh wrote:
> > > The behavior is quite different, I think it would be better to expose two
> > public
> > > functions, something like: RunTestsDistributed() and RunTestsDuplicated().
> > > 
> > > Then you can put the divergent logic from lines 208-218 in those functions
> and
> > > pass the lambda to a common implementation similar to the ShardAndRunTests
> you
> > > have here, except with the lambda instead of the sharding string.
> > 
> > I was discussing this with Frank in the gdoc, and I think we decided it was
> > overcomplicating it to create the additional two functions. I would be happy
> to
> > go either way though. Frank?
> 

I like the method names, but test_allocator.py is not as accurate as it could
be. How about test_dispatcher.py?

[gkanwar, 2013-07-12 19:22:31]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
File build/android/pylib/base/shard.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
build/android/pylib/base/shard.py:288: sharding='distribute'):
On 2013/07/12 19:03:36, craigdh wrote:
> On 2013/07/12 18:49:19, frankf wrote:
> > I still think the semantics is confusing. Sharding is synonymous with
> > distributing. How about we call this class test_allocator.py and have two
> > functions:
> > 
> > ShardAndRunTests()
> > ReplicateAndRunTests()
> > 
> > On 2013/07/12 18:22:41, gkanwar wrote:
> > > On 2013/07/12 18:14:15, craigdh wrote:
> > > > The behavior is quite different, I think it would be better to expose
two
> > > public
> > > > functions, something like: RunTestsDistributed() and
RunTestsDuplicated().
> > > > 
> > > > Then you can put the divergent logic from lines 208-218 in those
functions
> > and
> > > > pass the lambda to a common implementation similar to the
ShardAndRunTests
> > you
> > > > have here, except with the lambda instead of the sharding string.
> > > 
> > > I was discussing this with Frank in the gdoc, and I think we decided it
was
> > > overcomplicating it to create the additional two functions. I would be
happy
> > to
> > > go either way though. Frank?
> > 
> 
> I like the method names, but test_allocator.py is not as accurate as it could
> be. How about test_dispatcher.py?

We already have a 'dispatch.py' under pylib now (this is where we get the
devices and call this method to do the actual distribution and running). Since
we're not using the terms 'distrbuted' vs. 'duplicated' now, perhaps we could
name this test_distribution.py or distribute.py? I would consider both sharding
and replication forms of test distribution.

[gkanwar, 2013-07-12 22:14:25]

On 2013/07/12 19:22:31, gkanwar wrote:
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
> File build/android/pylib/base/shard.py (right):
> 
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/base/shar...
> build/android/pylib/base/shard.py:288: sharding='distribute'):
> On 2013/07/12 19:03:36, craigdh wrote:
> > On 2013/07/12 18:49:19, frankf wrote:
> > > I still think the semantics is confusing. Sharding is synonymous with
> > > distributing. How about we call this class test_allocator.py and have two
> > > functions:
> > > 
> > > ShardAndRunTests()
> > > ReplicateAndRunTests()
> > > 
> > > On 2013/07/12 18:22:41, gkanwar wrote:
> > > > On 2013/07/12 18:14:15, craigdh wrote:
> > > > > The behavior is quite different, I think it would be better to expose
> two
> > > > public
> > > > > functions, something like: RunTestsDistributed() and
> RunTestsDuplicated().
> > > > > 
> > > > > Then you can put the divergent logic from lines 208-218 in those
> functions
> > > and
> > > > > pass the lambda to a common implementation similar to the
> ShardAndRunTests
> > > you
> > > > > have here, except with the lambda instead of the sharding string.
> > > > 
> > > > I was discussing this with Frank in the gdoc, and I think we decided it
> was
> > > > overcomplicating it to create the additional two functions. I would be
> happy
> > > to
> > > > go either way though. Frank?
> > > 
> > 
> > I like the method names, but test_allocator.py is not as accurate as it
could
> > be. How about test_dispatcher.py?
> 
> We already have a 'dispatch.py' under pylib now (this is where we get the
> devices and call this method to do the actual distribution and running). Since
> we're not using the terms 'distrbuted' vs. 'duplicated' now, perhaps we could
> name this test_distribution.py or distribute.py? I would consider both
sharding
> and replication forms of test distribution.

Patch Set 2: I created the two new functions, fixed the shard unittests. I will
rename the file once we decide on a new name.

[frankf, 2013-07-12 22:28:28]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:210: runner_factory: callable that takes
devide and shard_index and returns
?

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:251: if not
ports.ResetTestServerPortAllocation():
There's still a lot of duplication in dispatch files. Essentially, all you're
really doing is taking options as the input, and returning the tuple (test
runner factory, tests, shard vs. replicate).

https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
File build/android/pylib/dispatch.py (right):

https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
build/android/pylib/dispatch.py:5: """Common dispatcher for all test types."""
Let's just fold this into test_runner.py

[gkanwar, 2013-07-12 22:53:34]

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
File build/android/pylib/gtest/dispatch.py (right):

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:210: runner_factory: callable that takes
devide and shard_index and returns
On 2013/07/12 22:28:28, frankf wrote:
> ?

s/devide/device/

Fixed.

https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
build/android/pylib/gtest/dispatch.py:251: if not
ports.ResetTestServerPortAllocation():
On 2013/07/12 22:28:28, frankf wrote:
> There's still a lot of duplication in dispatch files. Essentially, all you're
> really doing is taking options as the input, and returning the tuple (test
> runner factory, tests, shard vs. replicate).

I think the only duplication that exists currently is logging and the
ResetTestServerPortAllocation for content_browsertests and gtests. Perhaps we
should move logging out into test_runner. For the test server ports, I meant to
put a TODO here, but it should probably get moved out of RunGTests also... just
not sure where yet.

For gtests and content_browsertests there is also some amount of duplication in
how we get the tests, since they are so similar. Perhaps we could move that out
into a function for gtests and have content_browsertests import and call that
function with a paramater to indicate it should get the content_browser suite?

https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
File build/android/pylib/dispatch.py (right):

https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
build/android/pylib/dispatch.py:5: """Common dispatcher for all test types."""
On 2013/07/12 22:28:28, frankf wrote:
> Let's just fold this into test_runner.py

Some of the downstream scripts will be using this also (namely
run_popular_urls_test.py). I think it makes sense to leave it in pylib so they
can access it without importing test_runner.py.

[gkanwar, 2013-07-12 23:33:30]

On 2013/07/12 22:53:34, gkanwar wrote:
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> File build/android/pylib/gtest/dispatch.py (right):
> 
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> build/android/pylib/gtest/dispatch.py:210: runner_factory: callable that takes
> devide and shard_index and returns
> On 2013/07/12 22:28:28, frankf wrote:
> > ?
> 
> s/devide/device/
> 
> Fixed.
> 
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> build/android/pylib/gtest/dispatch.py:251: if not
> ports.ResetTestServerPortAllocation():
> On 2013/07/12 22:28:28, frankf wrote:
> > There's still a lot of duplication in dispatch files. Essentially, all
you're
> > really doing is taking options as the input, and returning the tuple (test
> > runner factory, tests, shard vs. replicate).
> 
> I think the only duplication that exists currently is logging and the
> ResetTestServerPortAllocation for content_browsertests and gtests. Perhaps we
> should move logging out into test_runner. For the test server ports, I meant
to
> put a TODO here, but it should probably get moved out of RunGTests also...
just
> not sure where yet.
> 
> For gtests and content_browsertests there is also some amount of duplication
in
> how we get the tests, since they are so similar. Perhaps we could move that
out
> into a function for gtests and have content_browsertests import and call that
> function with a paramater to indicate it should get the content_browser suite?
> 
>
https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
> File build/android/pylib/dispatch.py (right):
> 
>
https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
> build/android/pylib/dispatch.py:5: """Common dispatcher for all test types."""
> On 2013/07/12 22:28:28, frankf wrote:
> > Let's just fold this into test_runner.py
> 
> Some of the downstream scripts will be using this also (namely
> run_popular_urls_test.py). I think it makes sense to leave it in pylib so they
> can access it without importing test_runner.py.

Just talked to Frank -- I'm going to split this into two CLs. The first one will
be general sharding refactoring, and the second will be refactoring host_driven
tests.

[Philippe, 2013-07-17 13:44:30]

On 2013/07/12 23:33:30, gkanwar wrote:
> On 2013/07/12 22:53:34, gkanwar wrote:
> >
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> > File build/android/pylib/gtest/dispatch.py (right):
> > 
> >
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> > build/android/pylib/gtest/dispatch.py:210: runner_factory: callable that
takes
> > devide and shard_index and returns
> > On 2013/07/12 22:28:28, frankf wrote:
> > > ?
> > 
> > s/devide/device/
> > 
> > Fixed.
> > 
> >
>
https://codereview.chromium.org/18444004/diff/1/build/android/pylib/gtest/dis...
> > build/android/pylib/gtest/dispatch.py:251: if not
> > ports.ResetTestServerPortAllocation():
> > On 2013/07/12 22:28:28, frankf wrote:
> > > There's still a lot of duplication in dispatch files. Essentially, all
> you're
> > > really doing is taking options as the input, and returning the tuple (test
> > > runner factory, tests, shard vs. replicate).
> > 
> > I think the only duplication that exists currently is logging and the
> > ResetTestServerPortAllocation for content_browsertests and gtests. Perhaps
we
> > should move logging out into test_runner. For the test server ports, I meant
> to
> > put a TODO here, but it should probably get moved out of RunGTests also...
> just
> > not sure where yet.
> > 
> > For gtests and content_browsertests there is also some amount of duplication
> in
> > how we get the tests, since they are so similar. Perhaps we could move that
> out
> > into a function for gtests and have content_browsertests import and call
that
> > function with a paramater to indicate it should get the content_browser
suite?
> > 
> >
>
https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
> > File build/android/pylib/dispatch.py (right):
> > 
> >
>
https://codereview.chromium.org/18444004/diff/15001/build/android/pylib/dispa...
> > build/android/pylib/dispatch.py:5: """Common dispatcher for all test
types."""
> > On 2013/07/12 22:28:28, frankf wrote:
> > > Let's just fold this into test_runner.py
> > 
> > Some of the downstream scripts will be using this also (namely
> > run_popular_urls_test.py). I think it makes sense to leave it in pylib so
they
> > can access it without importing test_runner.py.
> 
> Just talked to Frank -- I'm going to split this into two CLs. The first one
will
> be general sharding refactoring, and the second will be refactoring
host_driven
> tests.

FYI, I added a call to 'Forwarder.UseMultiProcessing()' in
python_test_sharder.py. You will need to remove it once you got rid of
multiprocessing.