Issue 1415533007: [Android] Add sharding for AMP instrumentation tests.

Issue 1415533007: [Android] Add sharding for AMP instrumentation tests. (Closed)

Created:
5 years, 2 months ago by mikecase (-- gone --)

Modified:
4 years, 10 months ago

Reviewers:
rnephew (Wrong account), jbudorick, rnephew (Reviews Here)

CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Added --num-shards argument to the test_runner.py for remote tests. This argument currently only works for instrumentation tests. Also, to help with the sharding, I refactored the code that writes the appurify config files (each shard needs a slightly different config). Finally, just refactored some code as I went to make it easier to read BUG=

Patch Set 1 #

Patch Set 2 : Working version. #

Patch Set 3 : Removed Shard class. Changed implementation to be more similiar to local_device. #

Patch Set 4 : Lots of lint fixes. Cleanup. #

Patch Set 5 : #

Total comments: 44

Patch Set 6 : Fixed rnephew's nits. Most some of jbudorick's nits. #

Patch Set 7 : Fixed jbudoricks formatting nits. #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : Added context managers to manage life time. Reorganized some code. #

Patch Set 11 : #

Patch Set 12 : #

Total comments: 13

Patch Set 13 : Addressed rnephew's comments. #

Total comments: 8

Patch Set 14 : Removed RunThreadsSync and removed duplicated zip code. #

Patch Set 15 : Minor change to uirobot SetupTestShards. #

Total comments: 7

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+513 lines, -410 lines)			Patch
M	build/android/devil/utils/parallelizer.py	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	build/android/devil/utils/reraiser_thread.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+11 lines, -2 lines	2 comments	Download
A	build/android/devil/utils/tempdir.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+16 lines, -0 lines	1 comment	Download
M	build/android/pylib/remote/device/remote_device_environment.py	View	1 2 3 4 5 6 7 8 9 10 11 12	13 chunks	+72 lines, -50 lines	0 comments	Download
M	build/android/pylib/remote/device/remote_device_gtest_run.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13	3 chunks	+36 lines, -19 lines	2 comments	Download
M	build/android/pylib/remote/device/remote_device_instrumentation_test_run.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13	2 chunks	+76 lines, -39 lines	2 comments	Download
M	build/android/pylib/remote/device/remote_device_test_run.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13	7 chunks	+264 lines, -262 lines	0 comments	Download
M	build/android/pylib/remote/device/remote_device_uirobot_test_run.py	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14	1 chunk	+31 lines, -32 lines	0 comments	Download
M	build/android/test_runner.py	View	1 2 3 4 5 6 7 8 9	1 chunk	+6 lines, -4 lines	0 comments	Download

Messages

Total messages: 21 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mikecase (-- gone --)

If you are ever bored, please take a look through. Tested with instrumentation tests, gtests, ...

5 years, 1 month ago (2015-10-26 18:51:50 UTC) #3

rnephew (Reviews Here)

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_environment.py File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_environment.py#newcode364 build/android/pylib/remote/device/remote_device_environment.py:364: def network_config(self): Nit: Alphabetical order on properties. https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_gtest_run.py File ...

5 years, 1 month ago (2015-10-26 20:42:18 UTC) #4

jbudorick

Great news! I got bored while waiting for some tests to finish. https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_environment.py File build/android/pylib/remote/device/remote_device_environment.py ...

5 years, 1 month ago (2015-10-26 22:23:30 UTC) #5

mikecase (-- gone --)

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_environment.py File build/android/pylib/remote/device/remote_device_environment.py (right): https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_environment.py#newcode273 build/android/pylib/remote/device/remote_device_environment.py:273: self._devices.extend( On 2015/10/26 at 22:23:30, jbudorick wrote: > I ...

5 years, 1 month ago (2015-10-27 01:27:44 UTC) #6

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
File build/android/pylib/remote/device/remote_device_environment.py (right):

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_environment.py:273:
self._devices.extend(
On 2015/10/26 at 22:23:30, jbudorick wrote:
> I don't understand why you're adding N instances of device to self._devices.

I just want self._devices to be a list of all available devices. If there is 2
Nexus 5 and 1 Sony I want the list to be self._devices = [Nexus 5, Nexus 5,
Sony]. Later I call random.sample to get the devices I will be using from this
list.

I moved this logic to "def device_ids()" so that Im not storing multiple copies
of the whole device info dict, and instead just get multiple copies of the
device ids.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_environment.py:364: def
network_config(self):
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> Nit: Alphabetical order on properties.

Done.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
File build/android/pylib/remote/device/remote_device_gtest_run.py (right):

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_gtest_run.py:49: def
_GetTestRunnerName(self):
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> Same questions as remote_device_instrumentation_test_run about
_GetTestRunnerName and the pylint disabling.

Merged _GetTestRunnerName.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_gtest_run.py:62: # pylint:
disable=protected-access
On 2015/10/26 at 22:23:30, jbudorick wrote:
> This is a bad sign.

This was like this before. And you told me offline to keep this.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_gtest_run.py:66: flag_file =
tempfile.NamedTemporaryFile(
On 2015/10/26 at 22:23:30, jbudorick wrote:
> this is also a bad sign - is this ever removed?

yes, it is make in dir=self._base_tempfile_dir. This directory is created in
Setup and deleted in TearDown. This is done this way because context managers a
super awkward when you need #(num_shards) number of tempfiles.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_gtest_run.py:74: if
filter_string:
On 2015/10/26 at 22:23:30, jbudorick wrote:
> nit: Bring back the line breaks you deleted.

Ok. The lines are back.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
File build/android/pylib/remote/device/remote_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:20:
def _GetAdditionalApks(self):
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> It looks like this is returning the test_apk as well as additional_apks. This
name doesn't convey that very well. _GetAllApks() might be a little clearer? But
then its still missing apk_under_test, so maybe not.

Yeah, I don't like this either. So there is an --additional-apk argument to
test_runner.py for "additional apks you need to install on appurify for your
test to run." This is the list in self._test_instance.additional_apks. However,
we need to add test_apk to this list before passing it to the 'additional_apks'
appurify config. I deviced to add this new method. I'll try to think up a better
name.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:39:
def _GetTestRunnerName(self):
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> Is this used anywhere else than _GetFrameworkConfigs? If not maybe it could
just be merged with that.

Merged!

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:46:
# pylint: disable=protected-access
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> Just an idea, feel free to ignore. 
> Instead of disabling the pylint check, you could just add a AddDataDeps([list
of deps]) function to the test_instance.

We decided offline to keep as is.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:55:
test_list_file = tempfile.NamedTemporaryFile(
On 2015/10/26 at 22:23:30, jbudorick wrote:
> Again, not a huge fan of this. Restructure the code s.t. you can take
advantage of a context manager to handle the temporary file's lifetime.

Using a context manager is super awkward when you have to make #(num_shard)
number of tempfiles.
Also, I thought how it was set up before was really hard to follow...

self.SetUp()
  self._TriggerSetup()
  with test_list_file:
    self._AmInstrumentTestSetup()
      with test_zipfile:
        self._SetTestConfig()
          with test_config_file:
            self.UploadConfig()

It got super deep because everything was in a context manager. Like uploading a
config was done within 3 context manager in 3 functions. I prefer flat code over
nested code.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:112:
logging.critical('Sharding test run across %s device(s).', num_shards)
On 2015/10/26 at 22:23:30, jbudorick wrote:
> This is info, not critical.

Done.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:115: shard_env_vars
= (self._GetShardEnvironmentVars(num_shards)
On 2015/10/26 at 22:23:30, jbudorick wrote:
> I don't think this is structured properly. Most of the environment variables
should be common across all shards. The only one that shouldn't be is the list
of tests. That should be handled separately from the common environment
variables.

Yeah, but it even gets messier if you want some of the environment variables to
be shard dependent and some to be general. Im very open to suggestions, I just
couldn't think of a better way to do it.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:116: or [{} for _ in
range(num_shards)])
On 2015/10/26 at 22:23:30, jbudorick wrote:
> :/ this is ugly. Maybe [{}] * num_shards
> 
> also: when you think range, think xrange

Done.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:184:
zip(self._device_ids, self._shard_framework_configs)).pMap(trigger)
On 2015/10/26 at 22:23:30, jbudorick wrote:
> O_O
> 
> I... I don't know how I feel about this.

Ack

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:188:
self._test_run_ids).pMap(collect, parsed_results)
On 2015/10/26 at 22:23:30, jbudorick wrote:
> or, unsurprisingly, this.

Ack

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:240: def
abort_test_run(test_run_id):
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> Is there a reason you can't just pass self._AbortTestRun directly into
parallelizer?

Hmmmmmmmmmm, it seems to work your way. I assumed you couldn't do that. Because
I assumed the "self" argument would get lost if I just grabbed the
self._AbortTestRun function and passed it to the parallelizer. This doesn't seem
to be the case.

Done

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:311:
dir=self._base_tempfile_dir)
On 2015/10/26 at 22:23:30, jbudorick wrote:
> oh. This is how you're deleting the temporary files.
> 
> I would still vastly prefer a context manager.

Ack. All my temporary files are put into _base_tempfile_dir. This is created in
Setup() and deleted in TearDown(). As I said in some earlier comments, a context
manager with #(num_shard) number of tempfiles is super awkward.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:357:
access_token=self._env.token,
On 2015/10/26 at 22:23:30, jbudorick wrote:
> You sure added a lot of kwarg labels. Are they really necessary...?

Added them because it made reading the code far easier.
But seeing as you don't like them and you're the boss I'll remove (most) of
them...  o(╥﹏╥)o
Leaving kwargs I added for calls to appurify apis.

https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:434: def
_LogLogcat(self, results_zip, level=logging.CRITICAL):
On 2015/10/26 at 22:23:30, jbudorick wrote:
> methods that don't use self should usually be moved to module-scope private
functions.

Will do. Was kinda unsure since only this class really has any use for these
functions.

https://codereview.chromium.org/1415533007/diff/80001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1415533007/diff/80001/build/android/test_runn...
build/android/test_runner.py:134: help='DEPRECATED File path to download results
to.')
On 2015/10/26 at 20:42:18, rnephew1 wrote:
> So is there no way to now specify if you want to download the results zip from
Appurify? That still seems like it could be useful for devs running it, and not
having to know how to deal with appurify's website.

Well, now when you shard you are going to get num_shard zips. I guess I could
add the option to copy all of the result_zip files into a folder. Yeah, I'll do
that I suppose. I guess its still could be usable to devs if they set
--num_shards 1 and then they would get 1 zip with all of the results.

Done.

Now passing --results-dir /my/temp/dir
will put a bunch of results-<random tempfile string>.zip files into the
directory

jbudorick

(I'll re-review tomorrow.) https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_test_run.py File build/android/pylib/remote/device/remote_device_test_run.py (right): https://codereview.chromium.org/1415533007/diff/80001/build/android/pylib/remote/device/remote_device_test_run.py#newcode357 build/android/pylib/remote/device/remote_device_test_run.py:357: access_token=self._env.token, On 2015/10/27 01:27:43, mikecase wrote: ...

5 years, 1 month ago (2015-10-27 01:30:32 UTC) #7

mikecase (-- gone --)

Have an idea on how to make this CL less sucky. Hold off on the ...

5 years, 1 month ago (2015-10-28 22:23:14 UTC) #8

mikecase (-- gone --)

Please take a look through and give feedback. Tried to bring back context managers to ...

5 years, 1 month ago (2015-11-04 20:31:36 UTC) #9

rnephew (Wrong account)

https://codereview.chromium.org/1415533007/diff/220001/build/android/devil/utils/tempdir.py File build/android/devil/utils/tempdir.py (right): https://codereview.chromium.org/1415533007/diff/220001/build/android/devil/utils/tempdir.py#newcode1 build/android/devil/utils/tempdir.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. ...

5 years, 1 month ago (2015-11-04 22:22:03 UTC) #11

mikecase (-- gone --)

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/remote/device/remote_device_environment.py File build/android/pylib/remote/device/remote_device_environment.py (left): https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/remote/device/remote_device_environment.py#oldcode257 build/android/pylib/remote/device/remote_device_environment.py:257: random.shuffle(device_list) On 2015/11/04 at 22:22:02, rnephew wrote: > This ...

5 years, 1 month ago (2015-11-05 23:17:41 UTC) #12

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_environment.py (left):

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_environment.py:257:
random.shuffle(device_list)
On 2015/11/04 at 22:22:02, rnephew wrote:
> This was here so that we weren't selecting the same devices over and over so
that if there were device related problems they would be less prevalent. (I.E.
device always times out while installing apk). Is there a reason in particular
to get rid of it? Its also possible that those kinds of issues with appurify
devices are now less prevalent making this unnecessary.

Yes, this logic is still present, but got moved into remote_device_test_run.py.
I have _FindDevices now just find all available devices. Then in
remove_device_test_run.py, I select a random sample of num_shards devices from
the available devices.

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_environment.py (right):

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_environment.py:222:
api_key=self._api_key, api_secret=self._api_secret)
On 2015/11/04 at 22:22:02, rnephew wrote:
> These feel unnecessary. Having them be keyword args doesn't really make it any
clearer whats being passed.

Done.

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_environment.py:233:
access_token=self._access_token)
On 2015/11/04 at 22:22:02, rnephew wrote:
> Same.

Done.

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_gtest_run.py (right):

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_gtest_run.py:49: return False
On 2015/11/04 at 22:22:02, rnephew wrote:
> Instead of having remote_device_run.py have this raise NotImplementedError,
should we have it default to False?

Done.

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:71:
with tempdir.TempDir() as test_list_file_dir:
On 2015/11/04 at 22:22:03, rnephew wrote:
> Just a suggestion, feel free to ignore:
> This seems a bit verbose. Maybe just temp_dir, or test_temp_dir?

Changed to test_list_dir.

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_uirobot_test_run.py
(right):

https://codereview.chromium.org/1415533007/diff/220001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_uirobot_test_run.py:60: workers
= reraiser_thread.ReraiserThreadGroup(
On 2015/11/04 at 22:22:03, rnephew wrote:
> Since _ShouldShard() returns False, is it necessary to do this?
> Another thought, there seems to be a lot of boilerplate code around the
reraiser threads in all the test runs; is there a way to extract into into a
function and move it up to 'remote_device_test_run.py'? That might make less
sense than the current way its being done, just a thought.

Yeah, I'll add a RunThreadsSync function to reraiser_threads.py that takes a
list of reraiser threads and just creates a ThreadGroup, calls StartAll and then
calls JoinAll. This will remove lots of the boilerplate.

And since ShouldShard is False, no this is not neccessary. This is also not
necessary for the gtest_test_run. Removing the thread nonsense for those.

rnephew (Wrong account)

On 2015/11/12 21:15:22, mikecase wrote: > ping I'd give John another chance to review it, ...

5 years, 1 month ago (2015-11-12 22:11:30 UTC) #14

jbudorick

On 2015/11/12 22:11:30, rnephew wrote: > On 2015/11/12 21:15:22, mikecase wrote: > > ping > ...

5 years, 1 month ago (2015-11-12 22:18:32 UTC) #15

mikecase (-- gone --)

On 2015/11/12 at 22:18:32, jbudorick wrote: > On 2015/11/12 22:11:30, rnephew wrote: > > On ...

5 years, 1 month ago (2015-11-17 17:56:42 UTC) #16

jbudorick

partial review -- back to the drawing board https://codereview.chromium.org/1415533007/diff/240001/build/android/devil/utils/reraiser_thread.py File build/android/devil/utils/reraiser_thread.py (right): https://codereview.chromium.org/1415533007/diff/240001/build/android/devil/utils/reraiser_thread.py#newcode230 build/android/devil/utils/reraiser_thread.py:230: def ...

5 years, 1 month ago (2015-11-17 18:04:46 UTC) #17

mikecase (-- gone --)

Factored out shared zipfile code that was pretty much duplicated in remote_device_instrumentation_test_run.py and remote_device_gtest_test_run.py. Also ...

5 years, 1 month ago (2015-11-19 01:35:38 UTC) #18

mikecase (-- gone --)

I gave this CL another look over and I think it's in a pretty good ...

5 years, 1 month ago (2015-11-20 02:01:58 UTC) #19

mikecase (-- gone --)

On 2015/11/20 at 02:01:58, mikecase wrote: > I gave this CL another look over and ...

5 years ago (2015-11-24 16:57:25 UTC) #20

jbudorick

5 years ago (2015-12-01 04:07:13 UTC) #21

another partial review, will return tomorrow

https://codereview.chromium.org/1415533007/diff/280001/build/android/devil/ut...
File build/android/devil/utils/reraiser_thread.py (right):

https://codereview.chromium.org/1415533007/diff/280001/build/android/devil/ut...
build/android/devil/utils/reraiser_thread.py:229: args = args or
[None]*len(funcs)
where's the linter when I need it... spaces around operators, plz

https://codereview.chromium.org/1415533007/diff/280001/build/android/devil/ut...
build/android/devil/utils/reraiser_thread.py:234: ReraiserThread(f, args=a,
kwargs=k, name=n)
Adding these probably means you're abusing RunAsync somewhere...

https://codereview.chromium.org/1415533007/diff/280001/build/android/devil/ut...
File build/android/devil/utils/tempdir.py (right):

https://codereview.chromium.org/1415533007/diff/280001/build/android/devil/ut...
build/android/devil/utils/tempdir.py:10: def TempDir(suffix='', prefix='',
delete=True):
conflicts with your other review at this point, which I suppose is my fault.

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_gtest_run.py (right):

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_gtest_run.py:44: # pylint:
disable=protected-access
Why'd you move this up?

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_gtest_run.py:75: with
tempfile.NamedTemporaryFile(suffix='.zip') as test_package:
Why doesn't _PackageTest do this for us? i.e., couldn't you change _PackageTest
s.t. this becomes something like

  with self._PackageTest(self._test_instance.apk, data_deps) as test_package:

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:62:
def _SetupTestShards(self, num_shards):
This function is also doing too much. It shouldn't be generating the list of
tests in each shard and packaging & uploading all of the shards.

https://codereview.chromium.org/1415533007/diff/280001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:108:
funcs=[upload_test_shards]*num_shards,
I'd prefer you not add args, kwargs, or names to RunAsync and instead bind
whatever you need into nullary callables.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages