[Android] Refactor browser test execution.

[Android] Refactor browser test execution.

This change allows us to run multiple browser tests within a single
invocation of an app. It does so by moving the test activity into its
own process, running each test individually within that activity, and
killing the process between each test.

BUG=472360
Committed: https://crrev.com/b049103fa4848e304102a24e78c6ba994130a535
Cr-Commit-Position: refs/heads/master@{#334149}

[jbudorick, 2015-06-02 04:08:17]

jbudorick@chromium.org changed reviewers:
+ cjhopman@chromium.org, jaekyun@chromium.org, tedchoc@chromium.org

[jbudorick, 2015-06-02 04:08:18]

try job times so far:
  4m2s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
  3m33s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
  3m41s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

The current median over the last 2000 runs of linux_android_rel_ng is hovering
around 8.5 minutes. [1]

[1] taken from a hacked-up version of http://pastebin.com/Sm9zFn7g, a script
which was originally used in the CY (see "Aug 14 trybots in CQ stats. Generated
using this script." in
https://sites.google.com/a/chromium.org/hackability-cy/pillar-3)

[jbudorick, 2015-06-02 04:09:10]

On 2015/06/02 at 04:08:18, jbudorick wrote:
> try job times so far:
>   4m2s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
>   3m33s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
>   3m41s in
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> 
> The current median over the last 2000 runs of linux_android_rel_ng is hovering
around 8.5 minutes. [1]
> 
> [1] taken from a hacked-up version of http://pastebin.com/Sm9zFn7g, a script
which was originally used in the CY (see "Aug 14 trybots in CQ stats. Generated
using this script." in
https://sites.google.com/a/chromium.org/hackability-cy/pillar-3)

(to be clear: all times are for content_browsertests)

[Jaekyun Seok (inactive), 2015-06-02 21:50:43]

On 2015/06/02 04:09:10, jbudorick wrote:
> On 2015/06/02 at 04:08:18, jbudorick wrote:
> > try job times so far:
> >   4m2s in
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> >   3m33s in
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> >   3m41s in
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...
> > 
> > The current median over the last 2000 runs of linux_android_rel_ng is
hovering
> around 8.5 minutes. [1]
> > 
> > [1] taken from a hacked-up version of http://pastebin.com/Sm9zFn7g, a script
> which was originally used in the CY (see "Aug 14 trybots in CQ stats.
Generated
> using this script." in
> https://sites.google.com/a/chromium.org/hackability-cy/pillar-3)
> 
> (to be clear: all times are for content_browsertests)

Great! LGTM

[jbudorick, 2015-06-04 20:24:32]

Ted, Chris, thoughts?

[Ted C, 2015-06-05 01:39:37]

lgtm

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java
(right):

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:53:

each line should have a * leading it

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

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:82:
filterFlag.append("--gtest_filter=");
I assume there is no text bound on the command line right?  Just making sure we
won't get unexpected failures for a shard with too many tests.

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java:190:
mHandler.post(new ShardEnder(mPid));
Do you need to do anything special aside from logging to mark this failure?

[jbudorick, 2015-06-08 13:10:25]

Had to make process killing synchronous to keep the instrumentation from trying
to launch NativeBrowserTestActivity into a process that's about to die, which
was responsible for the flakes in the earlier patchsets.

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java
(right):

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:53:

On 2015/06/05 at 01:39:36, Ted C wrote:
> each line should have a * leading it

done

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

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:82:
filterFlag.append("--gtest_filter=");
On 2015/06/05 at 01:39:36, Ted C wrote:
> I assume there is no text bound on the command line right?  Just making sure
we won't get unexpected failures for a shard with too many tests.

Not to my knowledge. I've never seen a failure like that, and we pass some very
large command-line flags for some of our gtest suites.

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
File
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java:190:
mHandler.post(new ShardEnder(mPid));
On 2015/06/05 at 01:39:36, Ted C wrote:
> Do you need to do anything special aside from logging to mark this failure?

In theory, no; the parser below should have been picking up the start of the
test but not the end, and marking the test as UNKNOWN as a result.

In reality, the regex wasn't quite right, and RobotiumBundleGenerator wasn't
properly handling UNKNOWNs (which only would've affected appurify). Fixed both
in the latest patchset.

[jbudorick, 2015-06-08 19:03:38]

cjhopman: ptal at build/

[jbudorick, 2015-06-09 21:09:52]

On 2015/06/08 at 19:03:38, jbudorick wrote:
> cjhopman: ptal at build/

ping

[Ted C, 2015-06-09 21:15:12]

On 2015/06/08 13:10:25, jbudorick wrote:
> Had to make process killing synchronous to keep the instrumentation from
trying
> to launch NativeBrowserTestActivity into a process that's about to die, which
> was responsible for the flakes in the earlier patchsets.
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
> File
>
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java
> (right):
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
>
testing/android/native_test/java/src/org/chromium/native_test/NativeBrowserTestActivity.java:53:
> 
> On 2015/06/05 at 01:39:36, Ted C wrote:
> > each line should have a * leading it
> 
> done
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
> File
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java
> (right):
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:82:
> filterFlag.append("--gtest_filter=");
> On 2015/06/05 at 01:39:36, Ted C wrote:
> > I assume there is no text bound on the command line right?  Just making sure
> we won't get unexpected failures for a shard with too many tests.
> 
> Not to my knowledge. I've never seen a failure like that, and we pass some
very
> large command-line flags for some of our gtest suites.
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
> File
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java
> (right):
> 
>
https://codereview.chromium.org/1165523002/diff/120001/testing/android/native...
>
testing/android/native_test/java/src/org/chromium/native_test/NativeTestInstrumentationTestRunner.java:190:
> mHandler.post(new ShardEnder(mPid));
> On 2015/06/05 at 01:39:36, Ted C wrote:
> > Do you need to do anything special aside from logging to mark this failure?
> 
> In theory, no; the parser below should have been picking up the start of the
> test but not the end, and marking the test as UNKNOWN as a result.
> 
> In reality, the regex wasn't quite right, and RobotiumBundleGenerator wasn't
> properly handling UNKNOWNs (which only would've affected appurify). Fixed both
> in the latest patchset.

the changes still lgtm

[cjhopman, 2015-06-09 22:17:25]

https://codereview.chromium.org/1165523002/diff/180001/content/shell/android/...
File
content/shell/android/browsertests_apk/src/org/chromium/content_browsertests_apk/ContentBrowserTestsActivity.java
(right):

https://codereview.chromium.org/1165523002/diff/180001/content/shell/android/...
content/shell/android/browsertests_apk/src/org/chromium/content_browsertests_apk/ContentBrowserTestsActivity.java:43:
protected void initializeBrowserProcess() {
This function seems really, really similar to the ComponentsBrowserTests one

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

https://codereview.chromium.org/1165523002/diff/180001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:43:
private static final long RUN_TESTS_DELAY_IN_MS = 100;
Why is there a delay at all (and not just post the task)? Is there something
that we need to have happen between onCreate and the task actually running? Is
there any way for us to detect that that thing happened?

Edit: After reading below, my question has already been addressed. We don't
actually use this delay anymore. Please delete.

[jbudorick, 2015-06-10 02:41:54]

https://codereview.chromium.org/1165523002/diff/180001/content/shell/android/...
File
content/shell/android/browsertests_apk/src/org/chromium/content_browsertests_apk/ContentBrowserTestsActivity.java
(right):

https://codereview.chromium.org/1165523002/diff/180001/content/shell/android/...
content/shell/android/browsertests_apk/src/org/chromium/content_browsertests_apk/ContentBrowserTestsActivity.java:43:
protected void initializeBrowserProcess() {
On 2015/06/09 at 22:17:24, cjhopman wrote:
> This function seems really, really similar to the ComponentsBrowserTests one

Yeah, that's a known issue with all of the ComponentsBrowserTests code. Moved
some things around.

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

https://codereview.chromium.org/1165523002/diff/180001/testing/android/native...
testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java:43:
private static final long RUN_TESTS_DELAY_IN_MS = 100;
On 2015/06/09 at 22:17:24, cjhopman wrote:
> Why is there a delay at all (and not just post the task)? Is there something
that we need to have happen between onCreate and the task actually running? Is
there any way for us to detect that that thing happened?
> 
> Edit: After reading below, my question has already been addressed. We don't
actually use this delay anymore. Please delete.

Done.

https://codereview.chromium.org/1165523002/diff/200001/content/shell/android/...
File
content/shell/android/browsertests/src/org/chromium/content_shell/browsertests/ContentShellBrowserTestActivity.java
(right):

https://codereview.chromium.org/1165523002/diff/200001/content/shell/android/...
content/shell/android/browsertests/src/org/chromium/content_shell/browsertests/ContentShellBrowserTestActivity.java:24:
public abstract class ContentShellBrowserTestActivity extends
NativeBrowserTestActivity {
Most of the common code between ContentBrowserTestActivity and
ComponentsBrowserTestActivity depended on content/ or ui/, so it couldn't go in
NativeBrowserTestActivity, which only depends on base/. I created this for
browser tests that run on ContentShell (as opposed to ChromeBrowserTests, if we
ever get so adventurous on Android).

[jbudorick, 2015-06-11 14:41:48]

cjhopman: ping

[cjhopman, 2015-06-11 20:25:37]

lgtm

[jbudorick, 2015-06-12 00:05:39]

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

[jbudorick, 2015-06-12 00:05:40]

+jochen for components/test/DEPS OWNERS

[jochen (gone - plz use gerrit), 2015-06-12 08:36:52]

lgtm

[jbudorick, 2015-06-12 13:01:41]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-06-12 13:01:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
jaekyun@chromium.org, tedchoc@chromium.org, cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/1165523002/#ps260001
(title: "Add content/shell/android/browsertests/ to components/test/DEPS")

[commit-bot: I haz the power, 2015-06-12 13:01:53]

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

[commit-bot: I haz the power, 2015-06-12 13:06:25]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-06-12 13:07:27]

Patchset 14 (id:??) landed as
https://crrev.com/b049103fa4848e304102a24e78c6ba994130a535
Cr-Commit-Position: refs/heads/master@{#334149}

[jbudorick, 2015-06-12 13:18:27]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1186513002/ by jbudorick@chromium.org.

The reason for reverting is: broke cronet_sample_test_apk compilation :(.