Add option to push files to device using isolate for instrumentation tests.

Add option to push files to device using isolate for instrumentation tests.

BUG=400499
Committed: https://crrev.com/526d68ea2d7b6e5acf8960e0634bb82c49929c1b
Cr-Commit-Position: refs/heads/master@{#304870}

[mikecase (-- gone --), 2014-11-01 06:11:06]

https://codereview.chromium.org/689293002/diff/1/build/android/pylib/instrume...
File build/android/pylib/instrumentation/setup.py (right):

https://codereview.chromium.org/689293002/diff/1/build/android/pylib/instrume...
build/android/pylib/instrumentation/setup.py:62: 
Wondering if you have a good solution to this. I would prefer to only have 1
function that does any data pushing. The reason I have _PushExtraSuiteDataDeps
is because for example with the ContentShellTests...

src/content/test/data/android/device_files/ 
to end up in
sdcard/CHROME/TEST/DATA/content/test/data/android/device_files

BUT

src/net/data/ssl/certificates/
to end up in 
sdcard/net/data/sll/certificates/

This means that if we used isolate for everything, we would have to push half of
ISOLATE_DEPS_DIR to sdcard/chrome/test/data/ and the other half of
ISOLATE_DEPS_DIR to sdcard/

Three solution I can think of but none are clean.

1. Leave _PushExtraSuiteDataDeps as is and don't use isolate to push
src/net/data/sll/certificates/

2. After running isolate remap, go into the ISOLATE_DEPS_DIR and change its
structure prepending chrome/test/data/ to some file paths.

3. Have everything pushed to the base sdcard/ directory. This would include a
good number of changes to individual tests that hardcode where they grab the
dependencies from. But mostly would require lots of LGTMs.

[mikecase (-- gone --), 2014-11-01 07:35:23]

mikecase@chromium.org changed reviewers:
+ jbudorick@chromium.org

[mikecase (-- gone --), 2014-11-01 07:35:23]

Things changed a lot since I submitted my original CL to get isolate working for
instrumentation tests. I created a new issue because not much is shared besides
the .isolate files.

I only briefly tested my changes and things seem to work as expected. Monday, I
will make sure all the tests this CL affects aren't broken. 

Also, this shouldn't break any existing tests (test data can either be specified
in the test_data option like before, or using a .isolate file)

[jbudorick, 2014-11-03 15:52:33]

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
File build/android/pylib/base/base_setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:12: 
nit: no blank line here

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:15: def
GenerateDepsDirUsingIsolate(suite_name, isolate_file_path,
Either:
 - leave the params on the first line and indent subsequent lines to the depth
of the first param, or
 - move all params to subsequent lines and indent 4 spaces.

Don't mix the two.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:22: file path for the suite_name, this
will override it.
nit: indent these consistently

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
File build/android/pylib/base/base_test_runner.py (left):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_test_runner.py:75: def PushDataDeps(self):
Isn't this a merge conflict with your other CL?

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/gtes...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:212: (lambda d, opts:
base_setup.PushDataDeps(d, DeviceDir(d), opts)),
Instead of defining DeviceDir as a separate function, define this entire lambda
and pMap that.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:24: 'MojoTest':
'mojo/mojo_test_apk.isolate',
I'll talk to you about this today, but it's probably safest to handle Mojo in a
separate CL. It moved into its own repository that gets synced into chromium a
little while back, and it's got its own bots.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:36: test_files =
'net/data/ssl/certificates'
Why isn't this directory handled in the .isolate files?

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:88: if test_options.test_data:
This is only staying in here until you switch the bots over, right?

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:97: return
os.path.join(device.GetExternalStoragePath(), DEVICE_DATA_DIR)
What happened to trying to get rid of DEVICE_DATA_DIR?

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:100: (lambda d, opts:
base_setup.PushDataDeps(d, DeviceDir(d), opts)),
again w.r.t defining DeviceDir vs defining the lambda.

https://codereview.chromium.org/689293002/diff/20001/mojo/mojo_test_apk.isolate
File mojo/mojo_test_apk.isolate (right):

https://codereview.chromium.org/689293002/diff/20001/mojo/mojo_test_apk.isola...
mojo/mojo_test_apk.isolate:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
I think you'll have to add this in the mojo repository instead.

[mikecase (-- gone --), 2014-11-03 19:20:05]

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
File build/android/pylib/base/base_setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:12: 
On 2014/11/03 15:52:32, jbudorick wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:15: def
GenerateDepsDirUsingIsolate(suite_name, isolate_file_path,
On 2014/11/03 15:52:32, jbudorick wrote:
> Either:
>  - leave the params on the first line and indent subsequent lines to the depth
> of the first param, or
>  - move all params to subsequent lines and indent 4 spaces.
> 
> Don't mix the two.

Done.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_setup.py:22: file path for the suite_name, this
will override it.
On 2014/11/03 15:52:32, jbudorick wrote:
> nit: indent these consistently

Done.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
File build/android/pylib/base/base_test_runner.py (left):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/base...
build/android/pylib/base/base_test_runner.py:75: def PushDataDeps(self):
On 2014/11/03 15:52:32, jbudorick wrote:
> Isn't this a merge conflict with your other CL?

Super simple rebase. Just will need to remove 1 line.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:24: 'MojoTest':
'mojo/mojo_test_apk.isolate',
On 2014/11/03 15:52:32, jbudorick wrote:
> I'll talk to you about this today, but it's probably safest to handle Mojo in
a
> separate CL. It moved into its own repository that gets synced into chromium a
> little while back, and it's got its own bots.

Acknowledged.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:36: test_files =
'net/data/ssl/certificates'
On 2014/11/03 15:52:33, jbudorick wrote:
> Why isn't this directory handled in the .isolate files?

I would prefer to only have 1 function that does any data pushing. The reason I
have _PushExtraSuiteDataDeps is because for example with the
ContentShellTests...

src/content/test/data/android/device_files/ 
to end up in
sdcard/CHROME/TEST/DATA/content/test/data/android/device_files

BUT

src/net/data/ssl/certificates/
to end up in 
sdcard/net/data/sll/certificates/

This means that if we used isolate for everything, we would have to push half of
ISOLATE_DEPS_DIR to sdcard/chrome/test/data/ and the other half of
ISOLATE_DEPS_DIR to sdcard/

Three solution I can think of but none are clean.

1. Leave _PushExtraSuiteDataDeps as is and don't use isolate to push
src/net/data/sll/certificates/

2. After running isolate remap, go into the ISOLATE_DEPS_DIR and change its
structure, prepending chrome/test/data/ to some file paths.

3. Have everything pushed to the base sdcard/ directory. This would include a
good number of changes to individual tests that hardcode where they grab the
dependencies from. But mostly would require lots of LGTMs.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:88: if test_options.test_data:
On 2014/11/03 15:52:33, jbudorick wrote:
> This is only staying in here until you switch the bots over, right?
Yes, once the recipes switch from using --test-data to isolate I will remove the
function _PushDataDeps. 

_PushExtraSuiteDataDeps will probably have to stay for the time being (read
comment above).

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:97: return
os.path.join(device.GetExternalStoragePath(), DEVICE_DATA_DIR)
On 2014/11/03 15:52:32, jbudorick wrote:
> What happened to trying to get rid of DEVICE_DATA_DIR?
If I do it, it will be in a different CL. It will require a lot of owner
reviews, and I would prefer to get this in first.

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:100: (lambda d, opts:
base_setup.PushDataDeps(d, DeviceDir(d), opts)),
On 2014/11/03 15:52:32, jbudorick wrote:
> again w.r.t defining DeviceDir vs defining the lambda.

Done.

[mikecase (-- gone --), 2014-11-06 00:12:21]

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/gtes...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/689293002/diff/20001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:212: (lambda d, opts:
base_setup.PushDataDeps(d, DeviceDir(d), opts)),
On 2014/11/03 15:52:32, jbudorick wrote:
> Instead of defining DeviceDir as a separate function, define this entire
lambda
> and pMap that.

Done.

https://codereview.chromium.org/689293002/diff/20001/mojo/mojo_test_apk.isolate
File mojo/mojo_test_apk.isolate (right):

https://codereview.chromium.org/689293002/diff/20001/mojo/mojo_test_apk.isola...
mojo/mojo_test_apk.isolate:1: # Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2014/11/03 15:52:33, jbudorick wrote:
> I think you'll have to add this in the mojo repository instead.

Removed this file. Will add to mojo repo in another CL.

[jbudorick, 2014-11-11 16:31:47]

build/android/* lgtm w/ nits

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:212: def PushDataDepsToDeviceDir(device):
nit: I've been moving toward naming local functions as
lower_case_with_underscores to differentiate from functions defined at the top
level.

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:214: if test_package.suite_name ==
'breakpad_unittests'
nit: 4 space indent for continuation lines

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/inst...
File build/android/pylib/instrumentation/setup.py (right):

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:97: def
PushDataDepsToDeviceDir(device):
nit: same w.r.t. local functions.

[mikecase (-- gone --), 2014-11-11 23:23:21]

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
File build/android/pylib/gtest/setup.py (right):

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:212: def PushDataDepsToDeviceDir(device):
On 2014/11/11 16:31:47, jbudorick wrote:
> nit: I've been moving toward naming local functions as
> lower_case_with_underscores to differentiate from functions defined at the top
> level.

Done.

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/gtes...
build/android/pylib/gtest/setup.py:214: if test_package.suite_name ==
'breakpad_unittests'
On 2014/11/11 16:31:47, jbudorick wrote:
> nit: 4 space indent for continuation lines

Done.

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/inst...
File build/android/pylib/instrumentation/setup.py (right):

https://codereview.chromium.org/689293002/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/setup.py:97: def
PushDataDepsToDeviceDir(device):
On 2014/11/11 16:31:47, jbudorick wrote:
> nit: same w.r.t. local functions.

Done.

[mikecase (-- gone --), 2014-11-12 19:28:07]

mikecase@chromium.org changed reviewers:
+ csharp@chromium.org, torne@chromium.org

[mikecase (-- gone --), 2014-11-12 19:28:07]

Adding torne@ for OWNERS review for adding isolate file to android_webview/

Adding csharp@ for OWNERS review for adding isolate file to chrome/ and content/

[mikecase (-- gone --), 2014-11-12 19:28:09]

Adding torne@ for OWNERS review for adding isolate file to android_webview/

Adding csharp@ for OWNERS review for adding isolate file to chrome/ and content/

[Torne, 2014-11-12 19:30:17]

android_webview LGTM

[Torne, 2014-11-12 19:30:18]

android_webview LGTM

[jbudorick, 2014-11-18 00:17:16]

jbudorick@chromium.org changed reviewers:
+ maruel@chromium.org

[jbudorick, 2014-11-18 00:17:17]

+maruel for chrome/chrome_shell_test_apk.isolate and
content/content_shell_test_apk.isolate

[M-A Ruel, 2014-11-18 00:54:51]

lgtm with nits

https://codereview.chromium.org/689293002/diff/100001/android_webview/android...
File android_webview/android_webview_test_apk.isolate (right):

https://codereview.chromium.org/689293002/diff/100001/android_webview/android...
android_webview/android_webview_test_apk.isolate:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/689293002/diff/100001/build/android/pylib/bas...
File build/android/pylib/base/base_setup.py (right):

https://codereview.chromium.org/689293002/diff/100001/build/android/pylib/bas...
build/android/pylib/base/base_setup.py:1: # Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2014 in general :)

[mikecase (-- gone --), 2014-11-18 17:51:41]

https://codereview.chromium.org/689293002/diff/100001/android_webview/android...
File android_webview/android_webview_test_apk.isolate (right):

https://codereview.chromium.org/689293002/diff/100001/android_webview/android...
android_webview/android_webview_test_apk.isolate:1: # Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/11/18 00:54:51, M-A Ruel wrote:
> 2014

Done.

https://codereview.chromium.org/689293002/diff/100001/build/android/pylib/bas...
File build/android/pylib/base/base_setup.py (right):

https://codereview.chromium.org/689293002/diff/100001/build/android/pylib/bas...
build/android/pylib/base/base_setup.py:1: # Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/11/18 00:54:51, M-A Ruel wrote:
> 2014 in general :)

Done.

[mikecase (-- gone --), 2014-11-19 19:06:04]

The CQ bit was checked by mikecase@chromium.org

[commit-bot: I haz the power, 2014-11-19 19:06:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/689293002/120001

[commit-bot: I haz the power, 2014-11-19 20:02:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-19 20:03:01]

Patchset 7 (id:??) landed as
https://crrev.com/526d68ea2d7b6e5acf8960e0634bb82c49929c1b
Cr-Commit-Position: refs/heads/master@{#304870}

[M-A Ruel, 2014-11-24 20:30:22]

https://codereview.chromium.org/689293002/diff/120001/android_webview/android...
File android_webview/android_webview_test_apk.isolate (right):

https://codereview.chromium.org/689293002/diff/120001/android_webview/android...
android_webview/android_webview_test_apk.isolate:8:
'isolate_dependency_untracked': [
Sorry I missed it but it is not valid anymore, you can 'files' instead on each
.isolate file.