[Android WebViewShell] Make WebViewLayoutTest runnable with test_runner.py

[Android WebViewShell] Make WebViewLayoutTest runnable with test_runner.py

Make it possible to run the instrumentation tests in WebViewLayoutTest using
build/android/test_runner.py.

This patch adds an .isolate file, modifies the gyp target settings and uses
the standard paths for testing on the device. Also the apk now performs any
necessary copying (of js-test.js) itself, making it possible to run the
tests both locally and on the bots using the .isolate file.

BUG=497861
Committed: https://crrev.com/139cb05fe71b72e67776c85b9d0743fd61904ebe
Cr-Commit-Position: refs/heads/master@{#337040}

[timvolodine, 2015-06-26 14:38:18]

timvolodine@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2015-06-29 12:51:13]

Generally LGTM but I don't know a lot about the test runner/isolate/etc system.
You maybe should get someone more familiar with that to review it. DO we have
any other selfinstrumenting APKs?

[timvolodine, 2015-06-29 16:53:50]

On 2015/06/29 12:51:13, Torne wrote:
> Generally LGTM but I don't know a lot about the test runner/isolate/etc
system.
> You maybe should get someone more familiar with that to review it. DO we have
> any other selfinstrumenting APKs?

Looks like NativeTestInstrumentationTestRunner also instruments itself, i.e. the
runner is in the target package.
Usually the instrumentation apks have the appendix "Test", e.g.
ContentShellTest, but in this case it does not seem really necessary as the
whole AndroidWebViewShell.apk is meant for testing. Maybe we could split it off
at some point into AndroidWebViewShellTest.apk which would exclude some of the
java classes.

[timvolodine, 2015-06-29 17:00:52]

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

[timvolodine, 2015-06-29 17:00:53]

+tedchoc@: could you please verify the sanity of this patch? (or add somebody
who can?)

[Ted C, 2015-06-29 17:06:24]

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

[Ted C, 2015-06-29 17:06:24]

jbudorick -- he's going to be the best person for all your test infrastructure
needs

[jbudorick, 2015-06-29 17:49:05]

We don't have any other self-instrumenting APKs that run instrumentation tests
-- as the name (hopefully) implies, NativeTestInstrumentationTestRunner is for
running native tests. That said, I think this is fine.

lgtm w/ one question

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
This suite failed locally for me because this file doesn't seem to exist in the
repository, unlike basic-logging.html in testSimple.

[timvolodine, 2015-06-29 18:06:31]

thanks John, reply below,

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 17:49:05, jbudorick wrote:
> This suite failed locally for me because this file doesn't seem to exist in
the
> repository, unlike basic-logging.html in testSimple.

correct, the global-interface-listing.html (and the corresponding js file) are
actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The idea is
to actually reuse the blink layout tests, but with different expectations.

The actual recipe here https://codereview.chromium.org/1215703004/ does the
copying before running test_runner with isolate. Please feel free to comment
there as well as this is my first recipe implementation ;)

[jbudorick, 2015-06-29 18:11:20]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 at 18:06:31, timvolodine wrote:
> On 2015/06/29 17:49:05, jbudorick wrote:
> > This suite failed locally for me because this file doesn't seem to exist in
the
> > repository, unlike basic-logging.html in testSimple.
> 
> correct, the global-interface-listing.html (and the corresponding js file) are
actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The idea is
to actually reuse the blink layout tests, but with different expectations.

In that case, both files should be listed in the isolate file above, and the
corresponding copy logic in the recipe should be removed.

> 
> The actual recipe here https://codereview.chromium.org/1215703004/ does the
copying before running test_runner with isolate. Please feel free to comment
there as well as this is my first recipe implementation ;)

[timvolodine, 2015-06-29 18:19:32]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 18:11:19, jbudorick wrote:
> On 2015/06/29 at 18:06:31, timvolodine wrote:
> > On 2015/06/29 17:49:05, jbudorick wrote:
> > > This suite failed locally for me because this file doesn't seem to exist
in
> the
> > > repository, unlike basic-logging.html in testSimple.
> > 
> > correct, the global-interface-listing.html (and the corresponding js file)
are
> actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The idea is
> to actually reuse the blink layout tests, but with different expectations.
> 
> In that case, both files should be listed in the isolate file above, and the
> corresponding copy logic in the recipe should be removed.

that won't work unfortunately as all the test files need to be in one directory
(i.e. android_webview/tools/WebView/test/..) because they reference a common
js-test.js file which is different for webview (as opposed to the one in blink)

> 
> > 
> > The actual recipe here https://codereview.chromium.org/1215703004/ does the
> copying before running test_runner with isolate. Please feel free to comment
> there as well as this is my first recipe implementation ;)
>

[jbudorick, 2015-06-29 18:35:21]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 at 18:19:32, timvolodine wrote:
> On 2015/06/29 18:11:19, jbudorick wrote:
> > On 2015/06/29 at 18:06:31, timvolodine wrote:
> > > On 2015/06/29 17:49:05, jbudorick wrote:
> > > > This suite failed locally for me because this file doesn't seem to exist
in
> > the
> > > > repository, unlike basic-logging.html in testSimple.
> > > 
> > > correct, the global-interface-listing.html (and the corresponding js file)
are
> > actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The idea
is
> > to actually reuse the blink layout tests, but with different expectations.
> > 
> > In that case, both files should be listed in the isolate file above, and the
> > corresponding copy logic in the recipe should be removed.
> 
> that won't work unfortunately as all the test files need to be in one
directory (i.e. android_webview/tools/WebView/test/..) because they reference a
common js-test.js file which is different for webview (as opposed to the one in
blink)

If we need our own js-test.js, why don't we have our own
global-interface-listing.html that references it?

I'm primarily concerned that, as is, this test fails if not run by a bot. That's
not a state we want to be in.

> 
> > 
> > > 
> > > The actual recipe here https://codereview.chromium.org/1215703004/ does
the
> > copying before running test_runner with isolate. Please feel free to comment
> > there as well as this is my first recipe implementation ;)
> >

[timvolodine, 2015-06-29 18:46:31]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 18:35:21, jbudorick wrote:
> On 2015/06/29 at 18:19:32, timvolodine wrote:
> > On 2015/06/29 18:11:19, jbudorick wrote:
> > > On 2015/06/29 at 18:06:31, timvolodine wrote:
> > > > On 2015/06/29 17:49:05, jbudorick wrote:
> > > > > This suite failed locally for me because this file doesn't seem to
exist
> in
> > > the
> > > > > repository, unlike basic-logging.html in testSimple.
> > > > 
> > > > correct, the global-interface-listing.html (and the corresponding js
file)
> are
> > > actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The
idea
> is
> > > to actually reuse the blink layout tests, but with different expectations.
> > > 
> > > In that case, both files should be listed in the isolate file above, and
the
> > > corresponding copy logic in the recipe should be removed.
> > 
> > that won't work unfortunately as all the test files need to be in one
> directory (i.e. android_webview/tools/WebView/test/..) because they reference
a
> common js-test.js file which is different for webview (as opposed to the one
in
> blink)
> 
> If we need our own js-test.js, why don't we have our own
> global-interface-listing.html that references it?
> 
> I'm primarily concerned that, as is, this test fails if not run by a bot.
That's
> not a state we want to be in.
> 

because the blink html files + what they reference except js-test.js should be
the same, so if somebody changes a layout test in blink it will automatically
'propagate' to webview.

For running the tests locally there is a WebViewShell/test/test_runner.sh
script. Now that I think about it it would probably need a small modification to
run with this patch, but it should run fine with the trunk checkout.

> > 
> > > 
> > > > 
> > > > The actual recipe here https://codereview.chromium.org/1215703004/ does
> the
> > > copying before running test_runner with isolate. Please feel free to
comment
> > > there as well as this is my first recipe implementation ;)
> > >
>

[jbudorick, 2015-06-29 18:55:24]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
On 2015/06/29 at 18:46:31, timvolodine wrote:
> On 2015/06/29 18:35:21, jbudorick wrote:
> > On 2015/06/29 at 18:19:32, timvolodine wrote:
> > > On 2015/06/29 18:11:19, jbudorick wrote:
> > > > On 2015/06/29 at 18:06:31, timvolodine wrote:
> > > > > On 2015/06/29 17:49:05, jbudorick wrote:
> > > > > > This suite failed locally for me because this file doesn't seem to
exist
> > in
> > > > the
> > > > > > repository, unlike basic-logging.html in testSimple.
> > > > > 
> > > > > correct, the global-interface-listing.html (and the corresponding js
file)
> > are
> > > > actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The
idea
> > is
> > > > to actually reuse the blink layout tests, but with different
expectations.
> > > > 
> > > > In that case, both files should be listed in the isolate file above, and
the
> > > > corresponding copy logic in the recipe should be removed.
> > > 
> > > that won't work unfortunately as all the test files need to be in one
> > directory (i.e. android_webview/tools/WebView/test/..) because they
reference a
> > common js-test.js file which is different for webview (as opposed to the one
in
> > blink)
> > 
> > If we need our own js-test.js, why don't we have our own
> > global-interface-listing.html that references it?
> > 
> > I'm primarily concerned that, as is, this test fails if not run by a bot.
That's
> > not a state we want to be in.
> > 
> 
> because the blink html files + what they reference except js-test.js should be
the same, so if somebody changes a layout test in blink it will automatically
'propagate' to webview.
> 

Then what about listing the blink files in the .isolate, then having this test
(or setUp, or something inside the APK) copy them to the necessary locations?

> For running the tests locally there is a WebViewShell/test/test_runner.sh
script. Now that I think about it it would probably need a small modification to
run with this patch, but it should run fine with the trunk checkout.
> 

I don't like the idea of having bots run tests differently than devs.

> > > 
> > > > 
> > > > > 
> > > > > The actual recipe here https://codereview.chromium.org/1215703004/
does
> > the
> > > > copying before running test_runner with isolate. Please feel free to
comment
> > > > there as well as this is my first recipe implementation ;)
> > > >
> >

[Torne, 2015-06-30 11:25:16]

On 2015/06/29 18:55:24, jbudorick wrote:
>
https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
> File
>
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
> (right):
> 
>
https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
>
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
> runTest("webexposed/global-interface-listing.html",
> On 2015/06/29 at 18:46:31, timvolodine wrote:
> > On 2015/06/29 18:35:21, jbudorick wrote:
> > > On 2015/06/29 at 18:19:32, timvolodine wrote:
> > > > On 2015/06/29 18:11:19, jbudorick wrote:
> > > > > On 2015/06/29 at 18:06:31, timvolodine wrote:
> > > > > > On 2015/06/29 17:49:05, jbudorick wrote:
> > > > > > > This suite failed locally for me because this file doesn't seem to
> exist
> > > in
> > > > > the
> > > > > > > repository, unlike basic-logging.html in testSimple.
> > > > > > 
> > > > > > correct, the global-interface-listing.html (and the corresponding js
> file)
> > > are
> > > > > actually copied from blink, i.e. third_party/WebKit/LayoutTests/.. The
> idea
> > > is
> > > > > to actually reuse the blink layout tests, but with different
> expectations.
> > > > > 
> > > > > In that case, both files should be listed in the isolate file above,
and
> the
> > > > > corresponding copy logic in the recipe should be removed.
> > > > 
> > > > that won't work unfortunately as all the test files need to be in one
> > > directory (i.e. android_webview/tools/WebView/test/..) because they
> reference a
> > > common js-test.js file which is different for webview (as opposed to the
one
> in
> > > blink)
> > > 
> > > If we need our own js-test.js, why don't we have our own
> > > global-interface-listing.html that references it?
> > > 
> > > I'm primarily concerned that, as is, this test fails if not run by a bot.
> That's
> > > not a state we want to be in.
> > > 
> > 
> > because the blink html files + what they reference except js-test.js should
be
> the same, so if somebody changes a layout test in blink it will automatically
> 'propagate' to webview.
> > 
> 
> Then what about listing the blink files in the .isolate, then having this test
> (or setUp, or something inside the APK) copy them to the necessary locations?
> 
> > For running the tests locally there is a WebViewShell/test/test_runner.sh
> script. Now that I think about it it would probably need a small modification
to
> run with this patch, but it should run fine with the trunk checkout.
> > 
> 
> I don't like the idea of having bots run tests differently than devs.

Yeah, we shouldn't have a special test runner script for local testing, this
should be deleted once it's possible to run the tests with the standard test
runner.

> > > > 
> > > > > 
> > > > > > 
> > > > > > The actual recipe here https://codereview.chromium.org/1215703004/
> does
> > > the
> > > > > copying before running test_runner with isolate. Please feel free to
> comment
> > > > > there as well as this is my first recipe implementation ;)
> > > > >
> > >

[timvolodine, 2015-06-30 17:39:45]

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
File
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/1215733002/diff/40001/android_webview/tools/W...
android_webview/tools/WebViewShell/src/org/chromium/webview_shell/WebViewLayoutTest.java:66:
runTest("webexposed/global-interface-listing.html",
> Then what about listing the blink files in the .isolate, then having this test
> (or setUp, or something inside the APK) copy them to the necessary locations?
> 

ok, thanks for the suggestion John! I've updated the patch to do minimal copying
in the apk (i.e. only js-test.js needs copy). That way the recipe doesn't need
to copy anything and the required files are listed in the .isolate.

[timvolodine, 2015-07-01 16:34:46]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-07-01 16:34:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1215733002/#ps80002
(title: "no-find-copies")

[commit-bot: I haz the power, 2015-07-01 16:35:28]

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

[commit-bot: I haz the power, 2015-07-01 16:39:51]

Committed patchset #6 (id:80002)

[commit-bot: I haz the power, 2015-07-01 16:40:46]

Patchset 6 (id:??) landed as
https://crrev.com/139cb05fe71b72e67776c85b9d0743fd61904ebe
Cr-Commit-Position: refs/heads/master@{#337040}