👔 Move side-loaded test data /sdcard -> /sdcard/gtestdata

Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
Committed: https://crrev.com/0fe567512193cc8fbfcfd006ff1d504bd6346290
Cr-Commit-Position: refs/heads/master@{#396169}

[agrieve, 2016-04-27 16:46:16]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 16:46:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/20001

[commit-bot: I haz the power, 2016-04-27 17:10:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:10:42]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[agrieve, 2016-04-27 17:16:25]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 17:16:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/40001

[commit-bot: I haz the power, 2016-04-27 17:35:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:35:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-04-27 17:40:08]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-27 17:40:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/60001

[agrieve, 2016-04-27 19:30:20]

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

[agrieve, 2016-04-27 19:30:21]

Daily dose of scary cl...

[jbudorick, 2016-04-27 19:32:32]

On 2016/04/27 19:30:21, agrieve wrote:
> Daily dose of scary cl...

uh, yeah. Do you have a compelling reason for this one?

We very rarely hit the kinds of errors that I think you're trying to address
here.

[commit-bot: I haz the power, 2016-04-27 19:41:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 19:41:29]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-04-27 19:46:23]

On 2016/04/27 19:32:32, jbudorick wrote:
> On 2016/04/27 19:30:21, agrieve wrote:
> > Daily dose of scary cl...
> 
> uh, yeah. Do you have a compelling reason for this one?
> 
> We very rarely hit the kinds of errors that I think you're trying to address
> here.

I hit them quite a bit when doing the swarming work, and have even hit it before
that (months ago). Tests pass just fine locally, then fail on the bots. 

I don't think the bots wipe the external storage directory between tests, so
this is also a correctness issue. E.g. net_unittests could always pass on the
bots because it is run after base_unittests, which puts icudtl.dat there.

I acknowledge there's clear risk in this change, but I think the extra
correctness is worth it.

[jbudorick, 2016-04-27 19:49:01]

On 2016/04/27 19:46:23, agrieve wrote:
> On 2016/04/27 19:32:32, jbudorick wrote:
> > On 2016/04/27 19:30:21, agrieve wrote:
> > > Daily dose of scary cl...
> > 
> > uh, yeah. Do you have a compelling reason for this one?
> > 
> > We very rarely hit the kinds of errors that I think you're trying to address
> > here.
> 
> I hit them quite a bit when doing the swarming work, and have even hit it
before
> that (months ago). Tests pass just fine locally, then fail on the bots. 

Not surprised w.r.t. the swarming stuff, but you were directly working with a
lot of data dependencies in that case.

> 
> I don't think the bots wipe the external storage directory between tests, so
> this is also a correctness issue. E.g. net_unittests could always pass on the
> bots because it is run after base_unittests, which puts icudtl.dat there.

between tests, no. between runs, yes. with analyze, I would suspect that this
kind of failure would happen fairly often if it existed.

> 
> I acknowledge there's clear risk in this change, but I think the extra
> correctness is worth it.

reviewing

[jbudorick, 2016-04-27 19:59:01]

This scares me more after review than it did before.

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android.cc
File base/base_paths_android.cc (right):

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android...
base/base_paths_android.cc:45: *result =
result->Append(FILE_PATH_LITERAL("gtestdata"));
What's the motivation behind putting this in "gtestdata" rather than using the
external storage directory directly? This, tbh, is the scariest part of this
change.

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
(right):

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
return PathUtils.getExternalStorageDirectory() + "/gtestdata";
If we keep the "gtestdata" part, I'd prefer this use the PathService if
possible.

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:250:
dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
I'm assuming this is the motivation for gtestdata. I don't think this is worth
it.

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:119:
dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
ditto

https://codereview.chromium.org/1926793002/diff/60001/net/test/android/javate...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1926793002/diff/60001/net/test/android/javate...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:204:
// TODO(agrieve): Update all callers to use createAndStartDefaultServer()
directly.
I've been meaning to do this but haven't gotten around to it :/

[jbudorick, 2016-04-27 20:00:31]

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android.cc
File base/base_paths_android.cc (right):

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android...
base/base_paths_android.cc:45: *result =
result->Append(FILE_PATH_LITERAL("gtestdata"));
On 2016/04/27 19:59:01, jbudorick wrote:
> What's the motivation behind putting this in "gtestdata" rather than using the
> external storage directory directly? This, tbh, is the scariest part of this
> change.

(also, in the event we keep it, "gtestdata" is a bad name. Instrumentation tests
use this, too.)

[agrieve, 2016-04-27 20:26:32]

Another reason we should not be using the sdcard root: etiquette. The sdcard is
a shared resource and we should not be polluting it.

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android.cc
File base/base_paths_android.cc (right):

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android...
base/base_paths_android.cc:45: *result =
result->Append(FILE_PATH_LITERAL("gtestdata"));
On 2016/04/27 19:59:01, jbudorick wrote:
> What's the motivation behind putting this in "gtestdata" rather than using the
> external storage directory directly? This, tbh, is the scariest part of this
> change.

It's to avoid wiping the entire /sdcard when running tests. Alternatively, we
could have some custom code that deletes all files except directories that start
with a capital letter. 

Still feels like just putting the test data in a custom directory is the nicer
way to go though. I've on occasion used my personal android phone to run a test,
and I'd be upset if it wiped my sdcard.

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android...
base/base_paths_android.cc:45: *result =
result->Append(FILE_PATH_LITERAL("gtestdata"));
On 2016/04/27 20:00:30, jbudorick wrote:
> On 2016/04/27 19:59:01, jbudorick wrote:
> > What's the motivation behind putting this in "gtestdata" rather than using
the
> > external storage directory directly? This, tbh, is the scariest part of this
> > change.
> 
> (also, in the event we keep it, "gtestdata" is a bad name. Instrumentation
tests
> use this, too.)

chromium_test_root?

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
(right):

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
return PathUtils.getExternalStorageDirectory() + "/gtestdata";
On 2016/04/27 19:59:01, jbudorick wrote:
> If we keep the "gtestdata" part, I'd prefer this use the PathService if
> possible.

PathService has methods for overriding paths, but I don't see a java-side
querying method. Might not be worth having one since then you'd also need to
generate enums from the .h files.

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:250:
dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
On 2016/04/27 19:59:01, jbudorick wrote:
> I'm assuming this is the motivation for gtestdata. I don't think this is worth
> it.

The delete_device_stale=True part is the motivation.
This mkdir line here might not be needed, but I was just trying to be cautious.

[jbudorick, 2016-04-27 20:41:36]

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android.cc
File base/base_paths_android.cc (right):

https://codereview.chromium.org/1926793002/diff/60001/base/base_paths_android...
base/base_paths_android.cc:45: *result =
result->Append(FILE_PATH_LITERAL("gtestdata"));
On 2016/04/27 20:26:31, agrieve wrote:
> On 2016/04/27 19:59:01, jbudorick wrote:
> > What's the motivation behind putting this in "gtestdata" rather than using
the
> > external storage directory directly? This, tbh, is the scariest part of this
> > change.
> 
> It's to avoid wiping the entire /sdcard when running tests. Alternatively, we
> could have some custom code that deletes all files except directories that
start
> with a capital letter. 

eh, I'd rather have our own sdcard subdirectory than something like this.

> 
> Still feels like just putting the test data in a custom directory is the nicer
> way to go though. I've on occasion used my personal android phone to run a
test,
> and I'd be upset if it wiped my sdcard.

I would generally advise against this. I do try to warn people whenever I tell
them to do something that will nuke their SD card (and the test runner doesn't
do so, obviously).

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
(right):

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
return PathUtils.getExternalStorageDirectory() + "/gtestdata";
On 2016/04/27 20:26:31, agrieve wrote:
> On 2016/04/27 19:59:01, jbudorick wrote:
> > If we keep the "gtestdata" part, I'd prefer this use the PathService if
> > possible.
> 
> PathService has methods for overriding paths, but I don't see a java-side
> querying method. Might not be worth having one since then you'd also need to
> generate enums from the .h files.

I strongly dislike hardcoding this in multiple places; I guess that's what I'd
like to avoid if at all possible.

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:250:
dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
On 2016/04/27 20:26:31, agrieve wrote:
> On 2016/04/27 19:59:01, jbudorick wrote:
> > I'm assuming this is the motivation for gtestdata. I don't think this is
worth
> > it.
> 
> The delete_device_stale=True part is the motivation.
> This mkdir line here might not be needed, but I was just trying to be
cautious.

I meant the rm -rf portion more than the mkdir portion.

As usual, I don't like chaining multiple commands outside of DeviceUtils, which
has a private interface to handle such shenanigans. (It'd be nice to have a
public one at some point, but I haven't had time or general motivation to create
one that's clean enough to make public.)

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

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/rem...
build/android/pylib/remote/device/remote_device_test_run.py:234: #     test
files under /sdcard/gtestdata.  http://crbug.com/607169
Not crazy about breaking this and leaving it broken.

[agrieve, 2016-04-29 15:56:35]

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
(right):

https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
return PathUtils.getExternalStorageDirectory() + "/gtestdata";
On 2016/04/27 20:41:36, jbudorick wrote:
> On 2016/04/27 20:26:31, agrieve wrote:
> > On 2016/04/27 19:59:01, jbudorick wrote:
> > > If we keep the "gtestdata" part, I'd prefer this use the PathService if
> > > possible.
> > 
> > PathService has methods for overriding paths, but I don't see a java-side
> > querying method. Might not be worth having one since then you'd also need to
> > generate enums from the .h files.
> 
> I strongly dislike hardcoding this in multiple places; I guess that's what I'd
> like to avoid if at all possible.

Moved it to be passed in when calling nativeRunTests()

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:250:
dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
On 2016/04/27 20:41:36, jbudorick wrote:
> On 2016/04/27 20:26:31, agrieve wrote:
> > On 2016/04/27 19:59:01, jbudorick wrote:
> > > I'm assuming this is the motivation for gtestdata. I don't think this is
> worth
> > > it.
> > 
> > The delete_device_stale=True part is the motivation.
> > This mkdir line here might not be needed, but I was just trying to be
> cautious.
> 
> I meant the rm -rf portion more than the mkdir portion.
> 
> As usual, I don't like chaining multiple commands outside of DeviceUtils,
which
> has a private interface to handle such shenanigans. (It'd be nice to have a
> public one at some point, but I haven't had time or general motivation to
create
> one that's clean enough to make public.)

Alright, I don't think this will be a case that slows things down any, so
switched it to two calls.

[agrieve, 2016-05-05 00:41:10]

On 2016/04/29 15:56:35, agrieve wrote:
>
https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
> File base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
> (right):
> 
>
https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
> base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
> return PathUtils.getExternalStorageDirectory() + "/gtestdata";
> On 2016/04/27 20:41:36, jbudorick wrote:
> > On 2016/04/27 20:26:31, agrieve wrote:
> > > On 2016/04/27 19:59:01, jbudorick wrote:
> > > > If we keep the "gtestdata" part, I'd prefer this use the PathService if
> > > > possible.
> > > 
> > > PathService has methods for overriding paths, but I don't see a java-side
> > > querying method. Might not be worth having one since then you'd also need
to
> > > generate enums from the .h files.
> > 
> > I strongly dislike hardcoding this in multiple places; I guess that's what
I'd
> > like to avoid if at all possible.
> 
> Moved it to be passed in when calling nativeRunTests()
> 
>
https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
> File build/android/pylib/local/device/local_device_gtest_run.py (right):
> 
>
https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_gtest_run.py:250:
> dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
> On 2016/04/27 20:41:36, jbudorick wrote:
> > On 2016/04/27 20:26:31, agrieve wrote:
> > > On 2016/04/27 19:59:01, jbudorick wrote:
> > > > I'm assuming this is the motivation for gtestdata. I don't think this is
> > worth
> > > > it.
> > > 
> > > The delete_device_stale=True part is the motivation.
> > > This mkdir line here might not be needed, but I was just trying to be
> > cautious.
> > 
> > I meant the rm -rf portion more than the mkdir portion.
> > 
> > As usual, I don't like chaining multiple commands outside of DeviceUtils,
> which
> > has a private interface to handle such shenanigans. (It'd be nice to have a
> > public one at some point, but I haven't had time or general motivation to
> create
> > one that's clean enough to make public.)
> 
> Alright, I don't think this will be a case that slows things down any, so
> switched it to two calls.

oooookay... I think the GYP is fixed up now. Even without android supporting GYP
anymore, webrtc still at least requires apk_test.gypi to work, so still needed
to fix it up.

[agrieve, 2016-05-12 01:43:21]

On 2016/05/05 00:41:10, agrieve wrote:
> On 2016/04/29 15:56:35, agrieve wrote:
> >
>
https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
> > File
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1926793002/diff/60001/base/test/android/javat...
> >
base/test/android/javatests/src/org/chromium/base/test/util/UrlUtils.java:40:
> > return PathUtils.getExternalStorageDirectory() + "/gtestdata";
> > On 2016/04/27 20:41:36, jbudorick wrote:
> > > On 2016/04/27 20:26:31, agrieve wrote:
> > > > On 2016/04/27 19:59:01, jbudorick wrote:
> > > > > If we keep the "gtestdata" part, I'd prefer this use the PathService
if
> > > > > possible.
> > > > 
> > > > PathService has methods for overriding paths, but I don't see a
java-side
> > > > querying method. Might not be worth having one since then you'd also
need
> to
> > > > generate enums from the .h files.
> > > 
> > > I strongly dislike hardcoding this in multiple places; I guess that's what
> I'd
> > > like to avoid if at all possible.
> > 
> > Moved it to be passed in when calling nativeRunTests()
> > 
> >
>
https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
> > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > 
> >
>
https://codereview.chromium.org/1926793002/diff/60001/build/android/pylib/loc...
> > build/android/pylib/local/device/local_device_gtest_run.py:250:
> > dev.RunShellCommand('r=%s;rm -rf $r;mkdir -p $r' % device_root)
> > On 2016/04/27 20:41:36, jbudorick wrote:
> > > On 2016/04/27 20:26:31, agrieve wrote:
> > > > On 2016/04/27 19:59:01, jbudorick wrote:
> > > > > I'm assuming this is the motivation for gtestdata. I don't think this
is
> > > worth
> > > > > it.
> > > > 
> > > > The delete_device_stale=True part is the motivation.
> > > > This mkdir line here might not be needed, but I was just trying to be
> > > cautious.
> > > 
> > > I meant the rm -rf portion more than the mkdir portion.
> > > 
> > > As usual, I don't like chaining multiple commands outside of DeviceUtils,
> > which
> > > has a private interface to handle such shenanigans. (It'd be nice to have
a
> > > public one at some point, but I haven't had time or general motivation to
> > create
> > > one that's clean enough to make public.)
> > 
> > Alright, I don't think this will be a case that slows things down any, so
> > switched it to two calls.
> 
> oooookay... I think the GYP is fixed up now. Even without android supporting
GYP
> anymore, webrtc still at least requires apk_test.gypi to work, so still needed
> to fix it up.

Feelin it?

[jbudorick, 2016-05-18 14:23:38]

While this still scares me, I'm increasingly convinced that this is a good idea
overall.

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
File base/test/test_support_android.cc (right):

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:21: base::FilePath* g_test_data_dir = nullptr;
I'm really not crazy about introducing a global here or about introducing an
implicit ordering requirement between GetTestProviderPath and InitPathProvider,
although I suppose it was doing that before (in a less explicit way) for
DIR_ANDROID_APP_DATA.

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:142: CHECK(g_test_data_dir != nullptr);
Can this just be CHECK(g_test_data_dir) ?

It's been a while since I've written C++, and consequently I haven't worked with
nullptr much :|

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:244:
'chromium_tests_root')
I still don't really like having this hard-coded both here and in UrlUtils.java.
I'm wondering if we can provide this some other way (<meta-data> in the
manifest?) s.t. it's only specified once.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:251:
dev.RunShellCommand(['rm', '-rf', device_root])
check_return=True here and below

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:65:
return '/'.join(p if p else device_root for p in d)
nit: since you're here, this should be posixpath.join

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:110:
'chromium_tests_root')
3 places :(

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:120:
dev.RunShellCommand(['rm', '-rf', device_root])
again, check_return=True

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/re...
File build/android/pylib/remote/device/remote_device_test_run.py (right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/re...
build/android/pylib/remote/device/remote_device_test_run.py:233: #
TODO(agrieve): If AMP is ever ressurected, this needs to be changed to put
I suspect we may eventually resurrect this with cloud test lab
(https://developers.google.com/cloud-test-lab/), but for now, I'm ok with this.

https://codereview.chromium.org/1926793002/diff/200001/build/apk_test.gypi
File build/apk_test.gypi (right):

https://codereview.chromium.org/1926793002/diff/200001/build/apk_test.gypi#ne...
build/apk_test.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
Are we still maintaining the gyp build?

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:204:
// TODO(agrieve): Update all callers to use createAndStartDefaultServer()
directly.
You can TODO me on this if you'd like.

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java
(left):

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:26:
PathUtils.getExternalStorageDirectory() + "/net/data/ssl/certificates/";
:/

https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
(right):

https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:141:
mStdoutFifo, getApplicationContext(), UrlUtils.getIsolatedTestRoot());
actually, rather than doing something with <meta-data> the manifest, we could
have this driven solely by the test runner & have the directory get passed in
via intent extra...

[agrieve, 2016-05-19 02:11:53]

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
File base/test/test_support_android.cc (right):

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:21: base::FilePath* g_test_data_dir = nullptr;
On 2016/05/18 14:23:38, jbudorick wrote:
> I'm really not crazy about introducing a global here or about introducing an
> implicit ordering requirement between GetTestProviderPath and
InitPathProvider,
> although I suppose it was doing that before (in a less explicit way) for
> DIR_ANDROID_APP_DATA.

I think GetTestProvider being a non-exported function makes this okay. Clients
can't mess it up.

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:142: CHECK(g_test_data_dir != nullptr);
On 2016/05/18 14:23:38, jbudorick wrote:
> Can this just be CHECK(g_test_data_dir) ?
> 
> It's been a while since I've written C++, and consequently I haven't worked
with
> nullptr much :|

That would work too, but I don't love the implicit pointer -> boolean cast that
that does. Couldn't find anything in the style guide that talks about it though.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:244:
'chromium_tests_root')
On 2016/05/18 14:23:38, jbudorick wrote:
> I still don't really like having this hard-coded both here and in
UrlUtils.java.
> I'm wondering if we can provide this some other way (<meta-data> in the
> manifest?) s.t. it's only specified once.

I don't think manifest will work since tests can specify their own manifests.
Command-line flag is also annoying since there are different mechanics for
instrumentation tests / gtests. Perhaps as a bundle extra to the
instrumentation, but I think it's nicer to keep the number of args there low.
Perhaps easiest way would be to just regexp extract the value from the .java
file. However, I think with the name being much more searchable it's fine to
have it in the two spots (it used to be in 3 spots and less searchable, so
marked improvement at least)

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:251:
dev.RunShellCommand(['rm', '-rf', device_root])
On 2016/05/18 14:23:38, jbudorick wrote:
> check_return=True here and below

Done.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:65:
return '/'.join(p if p else device_root for p in d)
On 2016/05/18 14:23:38, jbudorick wrote:
> nit: since you're here, this should be posixpath.join

Done.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:110:
'chromium_tests_root')
On 2016/05/18 14:23:38, jbudorick wrote:
> 3 places :(

The entire pushing of .isolate data is duplicated. Didn't want this change to
creep that much.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:120:
dev.RunShellCommand(['rm', '-rf', device_root])
On 2016/05/18 14:23:38, jbudorick wrote:
> again, check_return=True

Done.

https://codereview.chromium.org/1926793002/diff/200001/build/apk_test.gypi
File build/apk_test.gypi (right):

https://codereview.chromium.org/1926793002/diff/200001/build/apk_test.gypi#ne...
build/apk_test.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2016/05/18 14:23:38, jbudorick wrote:
> Are we still maintaining the gyp build?

I believe webrtc still has bots that use it.

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
File
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java
(right):

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServer.java:204:
// TODO(agrieve): Update all callers to use createAndStartDefaultServer()
directly.
On 2016/05/18 14:23:38, jbudorick wrote:
> You can TODO me on this if you'd like.

Done.

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java
(left):

https://codereview.chromium.org/1926793002/diff/200001/net/test/android/javat...
net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:26:
PathUtils.getExternalStorageDirectory() + "/net/data/ssl/certificates/";
On 2016/05/18 14:23:38, jbudorick wrote:
> :/

Acknowledged.

[jbudorick, 2016-05-23 17:27:31]

https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
(right):

https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:141:
mStdoutFifo, getApplicationContext(), UrlUtils.getIsolatedTestRoot());
On 2016/05/18 14:23:38, jbudorick wrote:
> actually, rather than doing something with <meta-data> the manifest, we could
> have this driven solely by the test runner & have the directory get passed in
> via intent extra...

Any thoughts on this?

[agrieve, 2016-05-24 14:05:19]

On 2016/05/23 17:27:31, jbudorick wrote:
>
https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
> File
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
> (right):
> 
>
https://codereview.chromium.org/1926793002/diff/200001/testing/android/native...
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:141:
> mStdoutFifo, getApplicationContext(), UrlUtils.getIsolatedTestRoot());
> On 2016/05/18 14:23:38, jbudorick wrote:
> > actually, rather than doing something with <meta-data> the manifest, we
could
> > have this driven solely by the test runner & have the directory get passed
in
> > via intent extra...
> 
> Any thoughts on this?

Thoughts are the same as
https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...

I think just regexp'ing it java->py would be easier / nicer, but on the whole
think it's fine how it is since the term is easily code-searchable now.

[jbudorick, 2016-05-24 19:13:52]

This still scares me, but lgtm. Please watch this + warn sheriffs when you land
it.

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
File base/test/test_support_android.cc (right):

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:21: base::FilePath* g_test_data_dir = nullptr;
On 2016/05/19 02:11:53, agrieve wrote:
> On 2016/05/18 14:23:38, jbudorick wrote:
> > I'm really not crazy about introducing a global here or about introducing an
> > implicit ordering requirement between GetTestProviderPath and
> InitPathProvider,
> > although I suppose it was doing that before (in a less explicit way) for
> > DIR_ANDROID_APP_DATA.
> 
> I think GetTestProvider being a non-exported function makes this okay. Clients
> can't mess it up. 

ah, I didn't catch that before.

https://codereview.chromium.org/1926793002/diff/200001/base/test/test_support...
base/test/test_support_android.cc:142: CHECK(g_test_data_dir != nullptr);
On 2016/05/19 02:11:53, agrieve wrote:
> On 2016/05/18 14:23:38, jbudorick wrote:
> > Can this just be CHECK(g_test_data_dir) ?
> > 
> > It's been a while since I've written C++, and consequently I haven't worked
> with
> > nullptr much :|
> 
> That would work too, but I don't love the implicit pointer -> boolean cast
that
> that does. Couldn't find anything in the style guide that talks about it
though.

Acknowledged.

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/200001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:244:
'chromium_tests_root')
On 2016/05/19 02:11:53, agrieve wrote:
> On 2016/05/18 14:23:38, jbudorick wrote:
> > I still don't really like having this hard-coded both here and in
> UrlUtils.java.
> > I'm wondering if we can provide this some other way (<meta-data> in the
> > manifest?) s.t. it's only specified once.
> 
> I don't think manifest will work since tests can specify their own manifests.
> Command-line flag is also annoying since there are different mechanics for
> instrumentation tests / gtests. Perhaps as a bundle extra to the
> instrumentation, but I think it's nicer to keep the number of args there low.
> Perhaps easiest way would be to just regexp extract the value from the .java
> file. However, I think with the name being much more searchable it's fine to
> have it in the two spots (it used to be in 3 spots and less searchable, so
> marked improvement at least)

I disagree, particularly with the regexp extraction part, but I can live with
this for now.

https://codereview.chromium.org/1926793002/diff/220001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1926793002/diff/220001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_gtest_run.py:250:
dev.PushChangedFiles(host_device_tuples, delete_device_stale=True)
Beware that this is liable to make test execution time on the bots slightly
longer in the short term. (Once everything is on swarming, it won't matter.)

[agrieve, 2016-05-25 13:28:50]

agrieve@chromium.org changed reviewers:
+ mef@chromium.org, thakis@chromium.org

[agrieve, 2016-05-25 13:28:51]

thakis@chromium.org: Please review changes in base/, chrome/

mef@chromium.org: Please review changes in net/

[Nico, 2016-05-25 13:35:26]

lgtm

good luck!

[mef, 2016-05-25 14:36:27]

+xunjieli

net/ lgtm

Do we need to do any similar changes for cronet tests?

[agrieve, 2016-05-25 14:41:39]

On 2016/05/25 14:36:27, mef wrote:
> +xunjieli
> 
> net/ lgtm
> 
> Do we need to do any similar changes for cronet tests?

I used codesearch to find all references to EXTERNAL_STORAGE and the only one I
managed to find was the change in CertTestUtil.java. That said, it's hard to say
definitively...

[agrieve, 2016-05-25 14:41:56]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2016-05-25 14:42:02]

The CQ bit was unchecked by agrieve@chromium.org

[xunjieli, 2016-05-25 14:43:31]

On 2016/05/25 14:36:27, mef wrote:
> +xunjieli
> 
> net/ lgtm
> 
> Do we need to do any similar changes for cronet tests?

Our tests files are in data directory which is cleared for each test run.
NetLogs might be written to /sdcard/ (see
https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/...),
but we aren't using it in the tests. I think we are good. I kicked off a run on
android_cronet_tester trybot just to make sure this change won't break cronet
bots which are not on CQ.

[agrieve, 2016-05-25 14:43:33]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 14:43:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/240001

[agrieve, 2016-05-25 14:45:00]

Description was changed from

==========
Move side-loaded test data /sdcard -> /sdcard/gtestdata

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

to

==========
Move side-loaded test data /sdcard -> /sdcard/gtestdata

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

[commit-bot: I haz the power, 2016-05-25 16:42:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 16:42:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[agrieve, 2016-05-25 18:10:26]

Description was changed from

==========
Move side-loaded test data /sdcard -> /sdcard/gtestdata

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

to

==========
Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

[agrieve, 2016-05-25 18:10:46]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-05-25 18:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, mef@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1926793002/#ps260001
(title: "Fix embedded test server not initializing PathProvider overrides")

[agrieve, 2016-05-25 18:10:47]

The CQ bit was unchecked by agrieve@chromium.org

[agrieve, 2016-05-25 18:10:52]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2016-05-25 18:10:59]

The CQ bit was unchecked by agrieve@chromium.org

[agrieve, 2016-05-25 18:11:06]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 18:11:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/260001

[agrieve, 2016-05-25 18:12:12]

The CQ bit was unchecked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-05-25 18:12:45]

Dry run: None

[agrieve, 2016-05-26 02:55:14]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 02:55:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/320001

[commit-bot: I haz the power, 2016-05-26 04:33:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 04:33:02]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 04:33:09]

Dry run: None

[agrieve, 2016-05-26 13:46:13]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-05-26 13:46:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, mef@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1926793002/#ps320001
(title: "Was passing due to stale files on my /sdcard. Now should be fixed.
QuicTestServer had a ref to e::a…")

[commit-bot: I haz the power, 2016-05-26 13:46:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926793002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926793002/320001

[commit-bot: I haz the power, 2016-05-26 13:50:28]

Description was changed from

==========
Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

to

==========
Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

[commit-bot: I haz the power, 2016-05-26 13:50:30]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-05-26 13:51:40]

Description was changed from

==========
Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169
==========

to

==========
Move side-loaded test data /sdcard -> /sdcard/chromium_tests_root

And use delete_device_stale=True when pushing. This will prevent tests
mistakenly passing when they depend on stale data files from a previous test.

BUG=607169

Committed: https://crrev.com/0fe567512193cc8fbfcfd006ff1d504bd6346290
Cr-Commit-Position: refs/heads/master@{#396169}
==========

[commit-bot: I haz the power, 2016-05-26 13:51:41]

Patchset 17 (id:??) landed as
https://crrev.com/0fe567512193cc8fbfcfd006ff1d504bd6346290
Cr-Commit-Position: refs/heads/master@{#396169}

[agrieve, 2016-05-26 14:52:13]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/2011763007/ by agrieve@chromium.org.

The reason for reverting is: Broke webview tests:
https://build.chromium.org/p/chromium.android/builders/Android%20Webview%20M%....