Refactor and add special test runner to support Espresso tests

Refactor and add special test runner to support Espresso tests

The test runner is based off previously available GoogleInstrumentationRunner to provide Activity lifecycle status for Espresso test (because every interaction in Espresso is synced).

BUG=627910

[the real yoland, 2016-07-13 18:23:56]

yolandyan@chromium.org changed reviewers:
+ jbudorick@chromium.org, mikecase@chromium.org, sgurun@chromium.org

[jbudorick, 2016-07-13 18:39:20]

https://codereview.chromium.org/2146713003/diff/1/android_webview/automated_u...
File android_webview/automated_ui_tests.isolate (right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/automated_u...
android_webview/automated_ui_tests.isolate:7:
'<(DEPTH)/android_webview/tools/automated_ui_tests/test/data/',
This shouldn't be necessary any more. We should be able to handle it via data in
gn.

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml:1: <!--
What's the point in moving all of this to apk/? This diverges from the way we
structure apks & java directories elsewhere in chromium, and I don't think we
should do so here.

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:29:
private static final long MILLIS_TO_WAIT_FOR_ACTIVITY_TO_STOP =
TimeUnit.SECONDS.toMillis(2);
nit: ACTIVITY_STOP_WAIT_INTERVAL_MS

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:30:
private static final long MILLIS_TO_POLL_FOR_ACTIVITY_STOP =
nit: ACTIVITY_STOP_POLL_INTERVAL_MS

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:42:
Log.i(TAG, "Instrumentation Started");
What's the point of this log?

(also, it's wrong; if you want to log something here, you should be explicitly
saying that it's entering onCreate. start/onStart is a different part.)

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:58:
onException(t, e);
What is this? I don't see it defined anywhere, but it looks like it should be
logging...

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:78:
Log.i(TAG, String.format("waitForActivitiesToComplete() took: %sms", endTime -
startTime));
Is this here intentionally, or was it just here for debugging?

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:87:
while (currentActivityCount > 0 && System.currentTimeMillis() < endTime) {
Can this use Criteria/CriteriaHelper?
https://codesearch.chromium.org/chromium/src/content/public/test/android/java...

This looks awfully similar...

[mikecase (-- gone --), 2016-07-13 19:38:20]

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml:1: <!--
On 2016/07/13 at 18:39:19, jbudorick wrote:
> What's the point in moving all of this to apk/? This diverges from the way we
structure apks & java directories elsewhere in chromium, and I don't think we
should do so here.

Selim suggested it would be a good idea to move it since this is how the webview
shell code is structured.

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:66:

nit: extra line

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File android_webview/tools/automated_ui_tests/test/data/alert.html (right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/test/data/alert.html:1: <html>
Doesnt look like anything uses these html files. Probably add these when you add
the tests.

[the real yoland, 2016-07-13 23:36:43]

Description was changed from

==========
Refactor and add special test runner to support Espresso tests

BUG=627910
==========

to

==========
Refactor and add special test runner to support Espresso tests

The test runner is based off previously available GoogleInstrumentationRunner to
provide Activity lifecycle status for Espresso test (because every interaction
in Espresso is synced).

BUG=627910
==========

[sgurun-gerrit only, 2016-07-14 00:26:26]

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml:1: <!--
On 2016/07/13 19:38:20, mikecase wrote:
> On 2016/07/13 at 18:39:19, jbudorick wrote:
> > What's the point in moving all of this to apk/? This diverges from the way
we
> structure apks & java directories elsewhere in chromium, and I don't think we
> should do so here.
> 
> Selim suggested it would be a good idea to move it since this is how the
webview
> shell code is structured.

yeah this is how our tools/system_webview_shell is structured. I think it is
useful to move apks one layer as I am guessing this directory will not only
include apk but also tests and other data in the future.

How are similar test tools structured in chromium?

[the real yoland, 2016-07-14 03:10:00]

https://codereview.chromium.org/2146713003/diff/1/android_webview/automated_u...
File android_webview/automated_ui_tests.isolate (right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/automated_u...
android_webview/automated_ui_tests.isolate:7:
'<(DEPTH)/android_webview/tools/automated_ui_tests/test/data/',
On 2016/07/13 at 18:39:19, jbudorick wrote:
> This shouldn't be necessary any more. We should be able to handle it via data
in gn.

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
File
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:29:
private static final long MILLIS_TO_WAIT_FOR_ACTIVITY_TO_STOP =
TimeUnit.SECONDS.toMillis(2);
On 2016/07/13 at 18:39:20, jbudorick wrote:
> nit: ACTIVITY_STOP_WAIT_INTERVAL_MS

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:30:
private static final long MILLIS_TO_POLL_FOR_ACTIVITY_STOP =
On 2016/07/13 at 18:39:19, jbudorick wrote:
> nit: ACTIVITY_STOP_POLL_INTERVAL_MS

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:42:
Log.i(TAG, "Instrumentation Started");
On 2016/07/13 at 18:39:19, jbudorick wrote:
> What's the point of this log?
> 
> (also, it's wrong; if you want to log something here, you should be explicitly
saying that it's entering onCreate. start/onStart is a different part.)

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:58:
onException(t, e);
On 2016/07/13 at 18:39:19, jbudorick wrote:
> What is this? I don't see it defined anywhere, but it looks like it should be
logging...

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:66:

On 2016/07/13 at 19:38:20, mikecase wrote:
> nit: extra line

Done

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:78:
Log.i(TAG, String.format("waitForActivitiesToComplete() took: %sms", endTime -
startTime));
On 2016/07/13 at 18:39:20, jbudorick wrote:
> Is this here intentionally, or was it just here for debugging?

I think the idea of all these logging is to show that what stage of an Espresso
interaction is on during its run, since we can't see what's going on a device on
bot.

https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:87:
while (currentActivityCount > 0 && System.currentTimeMillis() < endTime) {
On 2016/07/13 at 18:39:19, jbudorick wrote:
> Can this use Criteria/CriteriaHelper?
https://codesearch.chromium.org/chromium/src/content/public/test/android/java...
> 
> This looks awfully similar...

Hmm, true, but pollInstrumentationThread from CriteriaHelper doesn't provide Log
on how many activities that are still active.
I am pro-logging for debugging purposes.

[jbudorick, 2016-07-14 17:46:13]

On 2016/07/14 00:26:26, sgurun wrote:
>
https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
> File android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml
> (right):
> 
>
https://codereview.chromium.org/2146713003/diff/1/android_webview/tools/autom...
> android_webview/tools/automated_ui_tests/apk/java/AndroidManifest.xml:1: <!--
> On 2016/07/13 19:38:20, mikecase wrote:
> > On 2016/07/13 at 18:39:19, jbudorick wrote:
> > > What's the point in moving all of this to apk/? This diverges from the way
> we
> > structure apks & java directories elsewhere in chromium, and I don't think
we
> > should do so here.
> > 
> > Selim suggested it would be a good idea to move it since this is how the
> webview
> > shell code is structured.
> 
> yeah this is how our tools/system_webview_shell is structured. I think it is
> useful to move apks one layer as I am guessing this directory will not only
> include apk but also tests and other data in the future.

Ah, ok. If other tools in android_webview are structured this way, I'm fine with
this.

> 
> How are similar test tools structured in chromium?

We don't have anything quite like these, but the test apks, which are probably
the closest analogue, are laid out approximately how this is laid out currently
(i.e., java/ and javatests/ without apk/) but there are definitely some
inconsistencies.

[sgurun-gerrit only, 2016-07-14 20:32:20]

https://codereview.chromium.org/2146713003/diff/20001/android_webview/tools/a...
File
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2146713003/diff/20001/android_webview/tools/a...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:70:
waitForIdleSync();
why is this needed?

https://codereview.chromium.org/2146713003/diff/20001/android_webview/tools/a...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:83:
protected void waitForActivitiesToComplete() {
would force-stopping activities work instead of waiting? if so, you can use adb
shell am force-stop <package>

[aluo, 2016-07-15 23:42:12]

aluo@chromium.org changed reviewers:
+ aluo@chromium.org

[aluo, 2016-07-15 23:42:13]

https://codereview.chromium.org/2146713003/diff/20001/android_webview/tools/a...
File
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2146713003/diff/20001/android_webview/tools/a...
android_webview/tools/automated_ui_tests/apk/javatests/src/org/chromium/webview_ui_test/test/WebViewUiInstrumentationTestRunner.java:46:
ApplicationLifecycleMonitorRegistry.registerInstance(mApplicationMonitor);
Missing IntentMonitorRegistry logic (it's more involved to hook it up correctly,
see
MonitoringInstrumentation.java in android.support.test.runner)  Without it,
tests that use espresso-intents won't work.