[Android] Add an out-of-app instrumentation driver APK.

[Android] Add an out-of-app instrumentation driver APK.

BUG=444049
Committed: https://crrev.com/79d22ed2b74d0ff1082f6febfd3fd93c0574d130
Cr-Commit-Position: refs/heads/master@{#326900}

[jbudorick, 2015-03-26 03:33:11]

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

[jbudorick, 2015-03-26 03:33:12]

Context: this lets us run instrumentation tests independently in a single call
to |am instrument|, i.e., a single Appurify invocation.

[jbudorick, 2015-03-26 03:34:33]

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

[jbudorick, 2015-03-26 03:34:34]

+klundberg for build/android/

I'll add OWNERS for the small changes in the other directories later.

[klundberg, 2015-03-27 16:13:30]

build/android/ lgtm

I looked at the other changes but prefer to let Ted give his opinion on the
general approach and the Java pieces.

https://codereview.chromium.org/1034053002/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1034053002/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:40:
(os.path.abspath(test_list_file.name), None))
Do you really need the extra parentheses in
"(os.path.abspath(test_list_file.name), None)" inside append(...)?

[Ted C, 2015-04-06 17:43:53]

tedchoc@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Ted C, 2015-04-06 17:43:55]

A couple high level questions,
1.) What happens if Outstrumentation is killed during the run of the test? 
(i.e. if you set Android's don't keep activity flag, does it actually still
work)?  If not, then this might be flaky during high memory pressure situations.

This is definitely the major thing that concerns me with this approach.

2.) Outstrumentation is probably "too cute" for it's own good.  I think
something like OnDeviceInstrumentationDriver or something more verbose but less
code name-y seems better in the long term.

+yfriedman as he might be interested

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:69:
mTargetArgs.remove(EXTRA_TARGET_PACKAGE);
why remove things?

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:88:
testListFilePath);
java indenting is 8 from the previous line

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:92:
while (true) {
nit: I normally see this as:

String test;
while ((test = testListFileReader.readLine()) != null) {
    mTestClasses.add(test);
}

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:126:
public void onDestroy() {
onStop is the corollary to onStart.  So the thread above "could" in theory be
created multiple times if the app is brought to the foreground before being
destroyed (after being backgrounded).

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:135:
private Messenger mReportingService;
why not declared inside of ReportingServiceConnection?

same with mServiceLock?

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:166:
public void registerWithReportingService(Messenger receiver)
javadoc all the public methods (and in ReportHandler)

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:250:
default:
probably should fail in this case as it would indicate a programming error.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:285:
OutstrumentationSlaveActivity.class.getName()));
same indent comment...8 from the previous line

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:340:
sendStatus(1, status);
where are these status values defined (1, 0, -1)?

Maybe they should be constants.

Same with "class" and "test" since it's all repeated.

I guess you could also have a:

private void sendTestStatus(int status, String testClass, String testMethod) {
    Bundle statusBundle = new Bundle();
    statusBundle.putString("class", testClass);
    statusBundle.putString("test", testMethod);
    sendStatus(status, statusBundle);
}

Then they could all delegate into that (akin to your failImpl above).

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/reporter/Reporter.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/Reporter.java:78:
mLock.wait();
what happens if the service were to be disconnected?  Would this hang forever? 
Do we expect disconnected/connected to happen multiple times?  I'm wondering if
we should be tracking state as well and fail quickly if something unexpected has
happened.

And for these waits, should you specify timeouts and fail if they don't happen
within X multiples of a reasonable timeout?

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java:36:
public void addFailure(Test test, AssertionFailedError e) {
javadoc

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java:46:
}
Could it ever not be a TestCase?

Should there be an else handling the error?

Maybe we should just take a class cast exception and not if check it (adding a
findbugs annotation to ignore it's complaint)

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/support/ResultsBundleGenerator.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/support/ResultsBundleGenerator.java:20:
public Bundle generate(Map<String, TestResult> rawResults);
public isn't necessary on interfaces as it is the default

also javadoc :-)

[jbudorick, 2015-04-07 00:54:39]

re (1): turning on that flag had no effect. I'm going to keep experimenting
because I think that, if Outstrumentation dies, everything should go down in
flames.

re (2): I couldn't resist making a bad joke with the name :) (but I figured it
wouldn't survive review).

I've addressed everything but the rename. I'm doing that in its own patchset to
maintain some useful delta for the rest of the changes.

https://codereview.chromium.org/1034053002/diff/1/build/android/pylib/remote/...
File build/android/pylib/remote/device/remote_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1034053002/diff/1/build/android/pylib/remote/...
build/android/pylib/remote/device/remote_device_instrumentation_test_run.py:40:
(os.path.abspath(test_list_file.name), None))
On 2015/03/27 16:13:30, klundberg wrote:
> Do you really need the extra parentheses in
> "(os.path.abspath(test_list_file.name), None)" inside append(...)?

Yep. _data_deps is a list of (host path, device path) tuples.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:69:
mTargetArgs.remove(EXTRA_TARGET_PACKAGE);
On 2015/04/06 17:43:54, Ted C wrote:
> why remove things?

I want Outstrumentation (% name change) to forward any arguments it doesn't use
to the child. Copying the incoming Bundle and ripping out the arguments that
Outstrumentation uses seemed to me to be the most straightforward way to do
that. I didn't (and don't) see a good way to handle e.g. looping through the key
set from the incoming bundle and adding any arguments that aren't of interest to
Outstrumentation to the target arguments beacuse all of the Bundle.put and
Bundle.get methods are type-specific and named accordingly.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:88:
testListFilePath);
On 2015/04/06 17:43:54, Ted C wrote:
> java indenting is 8 from the previous line

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:92:
while (true) {
On 2015/04/06 17:43:54, Ted C wrote:
> nit: I normally see this as:
> 
> String test;
> while ((test = testListFileReader.readLine()) != null) {
>     mTestClasses.add(test);
> }

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:126:
public void onDestroy() {
On 2015/04/06 17:43:54, Ted C wrote:
> onStop is the corollary to onStart.  So the thread above "could" in theory be
> created multiple times if the app is brought to the foreground before being
> destroyed (after being backgrounded).

Moved thread creation to onCreate and added a bool to gate thread start.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:135:
private Messenger mReportingService;
On 2015/04/06 17:43:54, Ted C wrote:
> why not declared inside of ReportingServiceConnection?
> 
> same with mServiceLock?

I'm not actually sure why I did that; I imagine that I was using them in Driver
at some point. Moved.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:166:
public void registerWithReportingService(Messenger receiver)
On 2015/04/06 17:43:54, Ted C wrote:
> javadoc all the public methods (and in ReportHandler)

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:250:
default:
On 2015/04/06 17:43:54, Ted C wrote:
> probably should fail in this case as it would indicate a programming error.

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:285:
OutstrumentationSlaveActivity.class.getName()));
On 2015/04/06 17:43:54, Ted C wrote:
> same indent comment...8 from the previous line

the troubles with jumping between languages

done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/master/Outstrumentation.java:340:
sendStatus(1, status);
On 2015/04/06 17:43:54, Ted C wrote:
> where are these status values defined (1, 0, -1)?
> 
> Maybe they should be constants.
> 
> Same with "class" and "test" since it's all repeated.
> 

I had thought they were private constants in InstrumentationTestRunner, but
they're public:
http://developer.android.com/reference/android/test/InstrumentationTestRunner...

Switching to those.

> I guess you could also have a:
> 
> private void sendTestStatus(int status, String testClass, String testMethod) {
>     Bundle statusBundle = new Bundle();
>     statusBundle.putString("class", testClass);
>     statusBundle.putString("test", testMethod);
>     sendStatus(status, statusBundle);
> }
> 
> Then they could all delegate into that (akin to your failImpl above).

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/reporter/Reporter.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/Reporter.java:78:
mLock.wait();
On 2015/04/06 17:43:54, Ted C wrote:
> what happens if the service were to be disconnected?  Would this hang forever?

> Do we expect disconnected/connected to happen multiple times?  I'm wondering
if
> we should be tracking state as well and fail quickly if something unexpected
has
> happened.
> 
> And for these waits, should you specify timeouts and fail if they don't happen
> within X multiples of a reasonable timeout?

It probably would hang. It certainly hangs when running in "normal" mode (i.e.,
without Outstrumentation) with the change I've made to
BaseInstrumentationTestRunner.

I created a Queue for pending messages if the service isn't connected. This no
longer waits on mLock at all.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java:36:
public void addFailure(Test test, AssertionFailedError e) {
On 2015/04/06 17:43:54, Ted C wrote:
> javadoc

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/outstrumentation/reporter/ReporterListener.java:46:
}
On 2015/04/06 17:43:54, Ted C wrote:
> Could it ever not be a TestCase?

I don't think so, but that's based on Android, not junit.

> 
> Should there be an else handling the error?
> 
> Maybe we should just take a class cast exception and not if check it (adding a
> findbugs annotation to ignore it's complaint)

Done.

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
File
testing/android/java/src/org/chromium/test/support/ResultsBundleGenerator.java
(right):

https://codereview.chromium.org/1034053002/diff/1/testing/android/java/src/or...
testing/android/java/src/org/chromium/test/support/ResultsBundleGenerator.java:20:
public Bundle generate(Map<String, TestResult> rawResults);
On 2015/04/06 17:43:54, Ted C wrote:
> public isn't necessary on interfaces as it is the default
> 
> also javadoc :-)

I'm noticing a theme here.

[jbudorick, 2015-04-07 15:09:22]

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:42:
public class OnDeviceInstrumentationDriver extends Instrumentation {
Renamed:
  Outstrumentation -> OnDeviceInstrumentationDriver
  OutstrumentationSlaveActivity -> OnDeviceInstrumentationPassenger
    (still a bad joke, but a more descriptive one)
  org.chromium.test.outstrumentation.master -> org.chromium.test.driver
  org.chromium.test.outstrumentation.slave -> org.chromium.test.passenger
  org.chromium.test.outstrumentation.reporter -> org.chromium.test.reporter

[Yaron, 2015-04-07 15:39:05]

Is it possible somewhere to describe why this is necessary? I get that we have
extra requirements in the form of the HTTP-server but how does appurify
necessitate this driver app? If it's an artifact of how appurify runs, do they
nor provide a support library?

https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi
File build/apk_test.gypi (right):

https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi#new...
build/apk_test.gypi:36: 'java_in_dir':
'<(DEPTH)/testing/android/java/src/org/chromium/native_test',
See my comments below. If you agree, then I think we should revert this

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:13:
<uses-sdk android:minSdkVersion="14" android:targetSdkVersion="21" />
Aurimas bumped everything to 16 now

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:15:
<application android:label="Outstrumentation">
update

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:40:
* An Instrumentation that drives instrumentation tests from outside the app.
(Hence the name.)
I think you can now drop "(Hence the name.)"

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:42:
public class OnDeviceInstrumentationDriver extends Instrumentation {
I think it would be helpful to have a mini-dd or one-pager on this that
describes the components and their interaction.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
* An Activity target for OnDeviceInstrumentationPassenger that starts the
specified
Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe I'm
just unfamiliar with the idiom/pattern

https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
File testing/android/on_device_instrumentation.gyp (right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
testing/android/on_device_instrumentation.gyp:1: 
These files need copyrights

https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
File testing/android/test_support.gyp (right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
testing/android/test_support.gyp:7: 'target_name': 'test_support_java',
There's already base_java_test_support and content_java_test_support so this is
getting confusing. Any reason not to include in base_java_test_support?

Further, in general, we don't break up a folder of java code into multiple
packages: i.e. chromium/test/driver, chromium/test/support are different
libraries. I think Chris has thoughts about making one java dir for everything
but until then I think it's clearer to separate top-level java folders as
projects and not packages within one. ISTM that you have two separable concepts:
a driver and a support library for use with that driver

[jbudorick, 2015-04-07 19:24:45]

I'll go into more detail in the one-pager, but this is necessary because of our
tests. We currently run each instrumentation test in its own invocation of the
instrumentation. This circumvents some number of issues with conflicting global
state requirements -- the one I predominantly ran into was command-line flags --
and is "acceptably slow" when testing against a local device. We drive the tests
individually in the test scripts at the moment, but doing so on Appurify would
be unacceptably slow because of the ~2 minute overhead per test run.

This accomplishes both of our goals: it's not unacceptably slow and it keeps
tests isolated from each other.

https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi
File build/apk_test.gypi (right):

https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi#new...
build/apk_test.gypi:36: 'java_in_dir':
'<(DEPTH)/testing/android/java/src/org/chromium/native_test',
On 2015/04/07 15:39:04, Yaron wrote:
> See my comments below. If you agree, then I think we should revert this

reverted + adjusted for moving testing/android/java to
testing/android/native_test/java

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:13:
<uses-sdk android:minSdkVersion="14" android:targetSdkVersion="21" />
On 2015/04/07 15:39:04, Yaron wrote:
> Aurimas bumped everything to 16 now

Done.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:15:
<application android:label="Outstrumentation">
On 2015/04/07 15:39:04, Yaron wrote:
> update

Done.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:40:
* An Instrumentation that drives instrumentation tests from outside the app.
(Hence the name.)
On 2015/04/07 15:39:04, Yaron wrote:
> I think you can now drop "(Hence the name.)"

oops, done.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:42:
public class OnDeviceInstrumentationDriver extends Instrumentation {
On 2015/04/07 15:39:04, Yaron wrote:
> I think it would be helpful to have a mini-dd or one-pager on this that
> describes the components and their interaction.

Can do. I've got it on a whiteboard right now and will post the link when I've
written it up.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
* An Activity target for OnDeviceInstrumentationPassenger that starts the
specified
On 2015/04/07 15:39:04, Yaron wrote:
> Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe I'm
> just unfamiliar with the idiom/pattern

I'm no good with names and am open to suggestions. Passenger made sense to be
because it's being driven by the Driver, but I'm certainly not sold on the name.
I'm not sure Broker is any more (or less) clear.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
File testing/android/on_device_instrumentation.gyp (right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
testing/android/on_device_instrumentation.gyp:1: 
On 2015/04/07 15:39:04, Yaron wrote:
> These files need copyrights

Done.

https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
File testing/android/test_support.gyp (right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
testing/android/test_support.gyp:7: 'target_name': 'test_support_java',
On 2015/04/07 15:39:04, Yaron wrote:
> There's already base_java_test_support and content_java_test_support so this
is
> getting confusing. Any reason not to include in base_java_test_support?
> 

This is used both in base_java_test_support and in build/apk_test.gypi because
it's used by both the gtests and instrumentation tests to dump results in a
format that Appurify understands.

> Further, in general, we don't break up a folder of java code into multiple
> packages: i.e. chromium/test/driver, chromium/test/support are different
> libraries. I think Chris has thoughts about making one java dir for everything
> but until then I think it's clearer to separate top-level java folders as
> projects and not packages within one. ISTM that you have two separable
concepts:
> a driver and a support library for use with that driver

I had thought that that was a product of gyp's quirks, but I don't have any
objections to splitting these into separate top-level java folders. Created:
  testing/android/appurify_support
  testing/android/driver
  testing/android/native_test
  testing/android/passenger
  testing/android/reporter

Moving the preexisting code to testing/android/native_test added quite a bit to
this CL. I'd be open to splitting that into a separate CL; wdyt?

[Yaron, 2015-04-07 20:08:43]

On 2015/04/07 19:24:45, jbudorick wrote:
> I'll go into more detail in the one-pager, but this is necessary because of
our
> tests. We currently run each instrumentation test in its own invocation of the
> instrumentation. This circumvents some number of issues with conflicting
global
> state requirements -- the one I predominantly ran into was command-line flags
--
> and is "acceptably slow" when testing against a local device. We drive the
tests
> individually in the test scripts at the moment, but doing so on Appurify would
> be unacceptably slow because of the ~2 minute overhead per test run.
> 
> This accomplishes both of our goals: it's not unacceptably slow and it keeps
> tests isolated from each other.


Ah, gotcha. I had wondered if we could drop this requirement regardless of
Appurify for improving test speed. I found that Android even offers
SingleLaunchActivityTestCase which would help somewhat but I worried about
isolation.
> 
> https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi
> File build/apk_test.gypi (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi#new...
> build/apk_test.gypi:36: 'java_in_dir':
> '<(DEPTH)/testing/android/java/src/org/chromium/native_test',
> On 2015/04/07 15:39:04, Yaron wrote:
> > See my comments below. If you agree, then I think we should revert this
> 
> reverted + adjusted for moving testing/android/java to
> testing/android/native_test/java
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> File testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml
> (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:13:
> <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="21" />
> On 2015/04/07 15:39:04, Yaron wrote:
> > Aurimas bumped everything to 16 now
> 
> Done.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:15:
> <application android:label="Outstrumentation">
> On 2015/04/07 15:39:04, Yaron wrote:
> > update
> 
> Done.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> File
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
> (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:40:
> * An Instrumentation that drives instrumentation tests from outside the app.
> (Hence the name.)
> On 2015/04/07 15:39:04, Yaron wrote:
> > I think you can now drop "(Hence the name.)"
> 
> oops, done.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:42:
> public class OnDeviceInstrumentationDriver extends Instrumentation {
> On 2015/04/07 15:39:04, Yaron wrote:
> > I think it would be helpful to have a mini-dd or one-pager on this that
> > describes the components and their interaction.
> 
> Can do. I've got it on a whiteboard right now and will post the link when I've
> written it up.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> File
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
> (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
> * An Activity target for OnDeviceInstrumentationPassenger that starts the
> specified
> On 2015/04/07 15:39:04, Yaron wrote:
> > Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe I'm
> > just unfamiliar with the idiom/pattern
> 
> I'm no good with names and am open to suggestions. Passenger made sense to be
> because it's being driven by the Driver, but I'm certainly not sold on the
name.
> I'm not sure Broker is any more (or less) clear.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
> File testing/android/on_device_instrumentation.gyp (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
> testing/android/on_device_instrumentation.gyp:1: 
> On 2015/04/07 15:39:04, Yaron wrote:
> > These files need copyrights
> 
> Done.
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
> File testing/android/test_support.gyp (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
> testing/android/test_support.gyp:7: 'target_name': 'test_support_java',
> On 2015/04/07 15:39:04, Yaron wrote:
> > There's already base_java_test_support and content_java_test_support so this
> is
> > getting confusing. Any reason not to include in base_java_test_support?
> > 
> 
> This is used both in base_java_test_support and in build/apk_test.gypi because
> it's used by both the gtests and instrumentation tests to dump results in a
> format that Appurify understands.
> 
> > Further, in general, we don't break up a folder of java code into multiple
> > packages: i.e. chromium/test/driver, chromium/test/support are different
> > libraries. I think Chris has thoughts about making one java dir for
everything
> > but until then I think it's clearer to separate top-level java folders as
> > projects and not packages within one. ISTM that you have two separable
> concepts:
> > a driver and a support library for use with that driver
> 
> I had thought that that was a product of gyp's quirks, but I don't have any
> objections to splitting these into separate top-level java folders. Created:
>   testing/android/appurify_support
>   testing/android/driver
>   testing/android/native_test
>   testing/android/passenger
>   testing/android/reporter
> 
> Moving the preexisting code to testing/android/native_test added quite a bit
to
> this CL. I'd be open to splitting that into a separate CL; wdyt?

sgtm

[Yaron, 2015-04-07 20:12:29]

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
File
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
(right):

https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
* An Activity target for OnDeviceInstrumentationPassenger that starts the
specified
On 2015/04/07 19:24:45, jbudorick wrote:
> On 2015/04/07 15:39:04, Yaron wrote:
> > Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe I'm
> > just unfamiliar with the idiom/pattern
> 
> I'm no good with names and am open to suggestions. Passenger made sense to be
> because it's being driven by the Driver, but I'm certainly not sold on the
name.
> I'm not sure Broker is any more (or less) clear.

Broker seemed like a decent-ish analog based on:
http://en.wikipedia.org/wiki/Broker_Pattern :)

[Yaron, 2015-04-07 20:12:58]

On 2015/04/07 20:12:29, Yaron wrote:
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> File
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
> (right):
> 
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
> * An Activity target for OnDeviceInstrumentationPassenger that starts the
> specified
> On 2015/04/07 19:24:45, jbudorick wrote:
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe
I'm
> > > just unfamiliar with the idiom/pattern
> > 
> > I'm no good with names and am open to suggestions. Passenger made sense to
be
> > because it's being driven by the Driver, but I'm certainly not sold on the
> name.
> > I'm not sure Broker is any more (or less) clear.
> 
> Broker seemed like a decent-ish analog based on:
> http://en.wikipedia.org/wiki/Broker_Pattern :)

Note that I'm sold on it as the best option - I'm not a fan of naming either

[jbudorick, 2015-04-08 15:58:22]

On 2015/04/07 20:08:43, Yaron wrote:
> On 2015/04/07 19:24:45, jbudorick wrote:
> > I'll go into more detail in the one-pager, but this is necessary because of
> our
> > tests. We currently run each instrumentation test in its own invocation of
the
> > instrumentation. This circumvents some number of issues with conflicting
> global
> > state requirements -- the one I predominantly ran into was command-line
flags
> --
> > and is "acceptably slow" when testing against a local device. We drive the
> tests
> > individually in the test scripts at the moment, but doing so on Appurify
would
> > be unacceptably slow because of the ~2 minute overhead per test run.
> > 
> > This accomplishes both of our goals: it's not unacceptably slow and it keeps
> > tests isolated from each other.
> 
> 
> Ah, gotcha. I had wondered if we could drop this requirement regardless of
> Appurify for improving test speed. I found that Android even offers
> SingleLaunchActivityTestCase which would help somewhat but I worried about
> isolation.

We can't drop it, but I don't think we have to run _each_ test in isolation. We
may be able to create small groups that don't have conflicting requirements.

> > 
> > https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi
> > File build/apk_test.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/build/apk_test.gypi#new...
> > build/apk_test.gypi:36: 'java_in_dir':
> > '<(DEPTH)/testing/android/java/src/org/chromium/native_test',
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > See my comments below. If you agree, then I think we should revert this
> > 
> > reverted + adjusted for moving testing/android/java to
> > testing/android/native_test/java
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> > File testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml
> > (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> > testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:13:
> > <uses-sdk android:minSdkVersion="14" android:targetSdkVersion="21" />
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > Aurimas bumped everything to 16 now
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> > testing/android/java/src/org/chromium/test/driver/AndroidManifest.xml:15:
> > <application android:label="Outstrumentation">
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > update
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> > File
> >
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> >
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:40:
> > * An Instrumentation that drives instrumentation tests from outside the app.
> > (Hence the name.)
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > I think you can now drop "(Hence the name.)"
> > 
> > oops, done.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> >
>
testing/android/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:42:
> > public class OnDeviceInstrumentationDriver extends Instrumentation {
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > I think it would be helpful to have a mini-dd or one-pager on this that
> > > describes the components and their interaction.
> > 
> > Can do. I've got it on a whiteboard right now and will post the link when
I've
> > written it up.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> > File
> >
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/java/sr...
> >
>
testing/android/java/src/org/chromium/test/passenger/OnDeviceInstrumentationPassenger.java:14:
> > * An Activity target for OnDeviceInstrumentationPassenger that starts the
> > specified
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > Perhaps *Broker? Passenger doesn't mean anything to me off the bat maybe
I'm
> > > just unfamiliar with the idiom/pattern
> > 
> > I'm no good with names and am open to suggestions. Passenger made sense to
be
> > because it's being driven by the Driver, but I'm certainly not sold on the
> name.
> > I'm not sure Broker is any more (or less) clear.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
> > File testing/android/on_device_instrumentation.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/on_devi...
> > testing/android/on_device_instrumentation.gyp:1: 
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > These files need copyrights
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
> > File testing/android/test_support.gyp (right):
> > 
> >
>
https://codereview.chromium.org/1034053002/diff/40001/testing/android/test_su...
> > testing/android/test_support.gyp:7: 'target_name': 'test_support_java',
> > On 2015/04/07 15:39:04, Yaron wrote:
> > > There's already base_java_test_support and content_java_test_support so
this
> > is
> > > getting confusing. Any reason not to include in base_java_test_support?
> > > 
> > 
> > This is used both in base_java_test_support and in build/apk_test.gypi
because
> > it's used by both the gtests and instrumentation tests to dump results in a
> > format that Appurify understands.
> > 
> > > Further, in general, we don't break up a folder of java code into multiple
> > > packages: i.e. chromium/test/driver, chromium/test/support are different
> > > libraries. I think Chris has thoughts about making one java dir for
> everything
> > > but until then I think it's clearer to separate top-level java folders as
> > > projects and not packages within one. ISTM that you have two separable
> > concepts:
> > > a driver and a support library for use with that driver
> > 
> > I had thought that that was a product of gyp's quirks, but I don't have any
> > objections to splitting these into separate top-level java folders. Created:
> >   testing/android/appurify_support
> >   testing/android/driver
> >   testing/android/native_test
> >   testing/android/passenger
> >   testing/android/reporter
> > 
> > Moving the preexisting code to testing/android/native_test added quite a bit
> to
> > this CL. I'd be open to splitting that into a separate CL; wdyt?
> 
> sgtm

https://codereview.chromium.org/1071573003/

[jbudorick, 2015-04-10 17:10:56]

rebased. working on the one-pager.

[jbudorick, 2015-04-17 23:09:30]

Passenger -> Broker

draft one-pager (well, 1.5 pager) here:
https://docs.google.com/a/chromium.org/document/d/1xQZvPBvzPs-MLACbNr2jdrANKn...

(currently requires @chromium.org for viewing + commenting)

[Yaron, 2015-04-18 00:22:58]

https://codereview.chromium.org/1034053002/diff/100001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1034053002/diff/100001/build/java.gypi#newcod...
build/java.gypi:104: 'java_in_dir_suffix%': '/src',
Seems unnecessary - please revert this file.

https://codereview.chromium.org/1034053002/diff/100001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1034053002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2931:
'../testing/android/on_device_instrumentation.gyp:broker_java',
This might be over-thinking it but you could have java_apk add this to deps for
is_test_apk == 1

https://codereview.chromium.org/1034053002/diff/100001/testing/android/driver...
File testing/android/driver/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1034053002/diff/100001/testing/android/driver...
testing/android/driver/java/AndroidManifest.xml:18:
android:process=":results_reporting"/>
This only has the one service, why specify android::process? Or does it run
separately from the Driver. If so, is that really desirable? I worry that the
ReportingService will get killed for some reason and we fail to collect results.

I imagine another way this could be done is with BroadcastReceivers. You could
have one here in the driver which would just listen to broadcasts of events
which the broker would supply. If you're concerned about potential crosstalk
between apps, you could have an identifier for each run that's included with the
test result. I'm not sure that it's strictly better, but does simplify the
service connection part and removes a process. wdyt?

[jbudorick, 2015-04-20 19:21:38]

I'll be updating the doc to reflect the change to BroadcastReceiver.

https://codereview.chromium.org/1034053002/diff/100001/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/1034053002/diff/100001/build/java.gypi#newcod...
build/java.gypi:104: 'java_in_dir_suffix%': '/src',
On 2015/04/18 00:22:57, Yaron wrote:
> Seems unnecessary - please revert this file.

Done.

https://codereview.chromium.org/1034053002/diff/100001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1034053002/diff/100001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2931:
'../testing/android/on_device_instrumentation.gyp:broker_java',
On 2015/04/18 00:22:58, Yaron wrote:
> This might be over-thinking it but you could have java_apk add this to deps
for
> is_test_apk == 1

I've left this as-is for now. I'm not sure it makes sense to include this in
APKs that don't use BaseInstrumentationTestRunner or a class that derives from
it.

https://codereview.chromium.org/1034053002/diff/100001/testing/android/driver...
File testing/android/driver/java/AndroidManifest.xml (right):

https://codereview.chromium.org/1034053002/diff/100001/testing/android/driver...
testing/android/driver/java/AndroidManifest.xml:18:
android:process=":results_reporting"/>
On 2015/04/18 00:22:58, Yaron wrote:
> This only has the one service, why specify android::process? Or does it run
> separately from the Driver. If so, is that really desirable? I worry that the
> ReportingService will get killed for some reason and we fail to collect
results.
> 
> I imagine another way this could be done is with BroadcastReceivers. You could
> have one here in the driver which would just listen to broadcasts of events
> which the broker would supply. If you're concerned about potential crosstalk
> between apps, you could have an identifier for each run that's included with
the
> test result. I'm not sure that it's strictly better, but does simplify the
> service connection part and removes a process. wdyt?

Switched from service to dynamically registered BroadcastReceiver. This dropped
~200 lines from this CL and is much simpler.

[Yaron, 2015-04-20 20:59:54]

https://codereview.chromium.org/1034053002/diff/120001/testing/android/driver...
File
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/120001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:219:
finishedLock.wait();
Does this timeout somewhere if the app crashes?

https://codereview.chromium.org/1034053002/diff/120001/testing/android/report...
File
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusReporter.java
(right):

https://codereview.chromium.org/1034053002/diff/120001/testing/android/report...
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusReporter.java:11:
* Reports test status to the {@link ReportingService}.
Nit: update

[jbudorick, 2015-04-21 17:28:20]

rebased, added heartbeats to detect crashed tests, updated doc

https://codereview.chromium.org/1034053002/diff/120001/testing/android/driver...
File
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/120001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:219:
finishedLock.wait();
On 2015/04/20 20:59:54, Yaron wrote:
> Does this timeout somewhere if the app crashes?

It does now.

https://codereview.chromium.org/1034053002/diff/120001/testing/android/report...
File
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusReporter.java
(right):

https://codereview.chromium.org/1034053002/diff/120001/testing/android/report...
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusReporter.java:11:
* Reports test status to the {@link ReportingService}.
On 2015/04/20 20:59:54, Yaron wrote:
> Nit: update

Done.

[Yaron, 2015-04-21 21:18:23]

lgtm

https://codereview.chromium.org/1034053002/diff/140001/testing/android/report...
File
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusListener.java
(right):

https://codereview.chromium.org/1034053002/diff/140001/testing/android/report...
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusListener.java:75:
}
mReporter.stopHeartbeat() ?

[jbudorick, 2015-04-22 00:09:31]

https://codereview.chromium.org/1034053002/diff/140001/testing/android/report...
File
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusListener.java
(right):

https://codereview.chromium.org/1034053002/diff/140001/testing/android/report...
testing/android/reporter/java/src/org/chromium/test/reporter/TestStatusListener.java:75:
}
On 2015/04/21 21:18:23, Yaron wrote:
> mReporter.stopHeartbeat() ?

Done.

[jbudorick, 2015-04-22 01:01:16]

jbudorick@chromium.org changed reviewers:
+ cjhopman@chromium.org, thestig@chromium.org, torne@chromium.org

[jbudorick, 2015-04-22 01:01:17]

adding owners:
+torne for android_webview/
+thestig for base/base.gyp
+cjhopman for build/apk_test.gypi

[Lei Zhang, 2015-04-22 01:07:16]

What about GN?

[Torne, 2015-04-22 09:40:23]

android_webview LGTM

[jbudorick, 2015-04-22 16:06:21]

On 2015/04/22 01:07:16, Lei Zhang wrote:
> What about GN?

Added.

[Lei Zhang, 2015-04-22 17:25:01]

https://codereview.chromium.org/1034053002/diff/220001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1034053002/diff/220001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2929:
'../testing/android/on_device_instrumentation.gyp:broker_java',
Why does chrome/android/BUILD.gn depend on //testing/android/driver:driver_apk
vs broker_java here?

[cjhopman, 2015-04-22 18:54:01]

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
File
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:210:
getContext().registerReceiver(r, TestStatusReceiver.INTENT_FILTER);
It's odd to me that this needs to know about and correctly use
TestStatusReceiver.INTENT_FILTER. What do you think about adding something like

void register(Context c) { ... }

to TestStatusReceiver and then make INTENT_FILTER private there?

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:229:
synchronized (statusLock) {
Does something else enforce a timeout on individual tests? IIRC, this was
handled by the python driver before.

[jbudorick, 2015-04-22 22:15:35]

https://codereview.chromium.org/1034053002/diff/220001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1034053002/diff/220001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2929:
'../testing/android/on_device_instrumentation.gyp:broker_java',
On 2015/04/22 17:25:01, Lei Zhang wrote:
> Why does chrome/android/BUILD.gn depend on //testing/android/driver:driver_apk
> vs broker_java here?

- the gn versions of chrome_shell_test_apk and content_shell_test_apk both now
depend on broker_java
- gyp doesn't have a good way of replicating gn's datadeps functionality to
require that an APK be built without pulling in its code. After talking with
cjhopman, I added a somewhat hacky gyp target that lets the three APKs that
require driver_apk do so.

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
File
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java
(right):

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:210:
getContext().registerReceiver(r, TestStatusReceiver.INTENT_FILTER);
On 2015/04/22 18:54:01, cjhopman wrote:
> It's odd to me that this needs to know about and correctly use
> TestStatusReceiver.INTENT_FILTER. What do you think about adding something
like
> 
> void register(Context c) { ... }
> 
> to TestStatusReceiver and then make INTENT_FILTER private there?

sgtm, done.

https://codereview.chromium.org/1034053002/diff/220001/testing/android/driver...
testing/android/driver/java/src/org/chromium/test/driver/OnDeviceInstrumentationDriver.java:229:
synchronized (statusLock) {
On 2015/04/22 18:54:01, cjhopman wrote:
> Does something else enforce a timeout on individual tests? IIRC, this was
> handled by the python driver before.

Not at the moment. You're correct that the timeouts are defined and enforced by
the python test runner based on annotations. I don't think the driver has the
information to handle timeouts, but I think I could implement them in
BaseInstrumentationTestRunner. I would prefer to do so in a separate CL,
especially since this won't be immediately used on the bots (and have added a
TODO to BaseInstrumentationTestRunner to that effect) -- wdyt?

[Lei Zhang, 2015-04-22 23:11:16]

base/ and chrome/ gyp/gn files lgtm

[cjhopman, 2015-04-24 01:31:54]

lgtm

https://codereview.chromium.org/1034053002/diff/260001/testing/android/on_dev...
File testing/android/on_device_instrumentation.gyp (right):

https://codereview.chromium.org/1034053002/diff/260001/testing/android/on_dev...
testing/android/on_device_instrumentation.gyp:52: 'target_name':
'require_driver_apk',
This could probably use a short comment about what it's doing

[jbudorick, 2015-04-24 19:58:47]

https://codereview.chromium.org/1034053002/diff/260001/testing/android/on_dev...
File testing/android/on_device_instrumentation.gyp (right):

https://codereview.chromium.org/1034053002/diff/260001/testing/android/on_dev...
testing/android/on_device_instrumentation.gyp:52: 'target_name':
'require_driver_apk',
On 2015/04/24 01:31:54, cjhopman wrote:
> This could probably use a short comment about what it's doing

Done.

[jbudorick, 2015-04-24 19:58:55]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-04-24 19:58:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
klundberg@chromium.org, yfriedman@chromium.org, torne@chromium.org,
thestig@chromium.org, cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/1034053002/#ps280001
(title: "cjhopman comment")

[commit-bot: I haz the power, 2015-04-24 19:59:21]

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

[commit-bot: I haz the power, 2015-04-24 22:15:39]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-24 22:17:00]

Patchset 15 (id:??) landed as
https://crrev.com/79d22ed2b74d0ff1082f6febfd3fd93c0574d130
Cr-Commit-Position: refs/heads/master@{#326900}

[Justin Donnelly, 2015-04-24 22:50:56]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/1094903008/ by jdonnelly@chromium.org.

The reason for reverting is: Broke the "Android" bot:

http://build.chromium.org/p/chromium/builders/Android/builds/38661.