Convert WebView Layout test to JUnit4

Convert WebView Layout test to JUnit4

Since Android N came out, previous JUnit3 instrumentation tests are now deprecated.
We are now gradually converting all the instrumentation tests to JUnit4 style.
For details on this migration, please check the bug.

BUG=640116
Review-Url: https://codereview.chromium.org/2633733002
Cr-Commit-Position: refs/heads/master@{#451362}
Committed: https://chromium.googlesource.com/chromium/src/+/9dfd7fc55c1a6803577c5a1dc38a6e144139f101

[the real yoland, 2017-01-13 19:47:22]

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

[jbudorick, 2017-01-13 20:32:19]

I'm so excited.

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:64:
private boolean mIsRebaseLine;
nit: just mRebaseline

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java
(right):

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewThreadTest.java:38:
public ActivityTestRule<WebViewThreadTestActivity> mActivtyTestRule =
mActivityTestRule

[the real yoland, 2017-01-23 17:36:23]

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/1/android_webview/tools/syste...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:64:
private boolean mIsRebaseLine;
On 2017/01/13 at 20:32:19, jbudorick wrote:
> nit: just mRebaseline

Done

[mikecase (-- gone --), 2017-02-01 17:56:48]

This all lgtm. 2 small fixes.

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
File android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml
(right):

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml:36:
junit4="true"/>
update to chromium_junit4 like you mentioned.

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:86:
private boolean isRebaseline() {
delete this function:

https://cs.chromium.org/chromium/src/android_webview/tools/system_webview_she...

[the real yoland, 2017-02-01 18:52:50]

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

[the real yoland, 2017-02-01 18:52:51]

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
File android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml
(right):

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml:36:
junit4="true"/>
On 2017/02/01 at 17:56:48, mikecase wrote:
> update to chromium_junit4 like you mentioned.

Done

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/40001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:86:
private boolean isRebaseline() {
On 2017/02/01 at 17:56:48, mikecase wrote:
> delete this function:
> 
>
https://cs.chromium.org/chromium/src/android_webview/tools/system_webview_she...

Done

[timvolodine, 2017-02-16 23:37:24]

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml:35:
android:label="Android JUnit 4 runner"
why not keep the label?

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:98:
@Test
are the @Test required? we already have @MediumTest below, so looks a bit weird
having it twice..

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:135:
@DisabledTest(message = "crbug.com/683153")
don't think this should be here?

[the real yoland, 2017-02-17 01:15:14]

Ty for the review, updated

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/AndroidManifest.xml:35:
android:label="Android JUnit 4 runner"
On 2017/02/16 at 23:37:24, timvolodine wrote:
> why not keep the label?

Done

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:98:
@Test
On 2017/02/16 at 23:37:24, timvolodine wrote:
> are the @Test required? we already have @MediumTest below, so looks a bit
weird having it twice..

ya, unfortunately JUnit4 requires `@Test` annotation (we can override the test
runner to look for @Small/Medium/LargeTest annotations, but it would not align
with the other android instrumentation tests style, also, @Test provides useful
functionalities like `@Test(expect=Exception)`)

Since we only use the @Small/Medium/LargeTest annotations to identify how long
it takes for the test to run, we might in the future get rid of it and just use
@Test(timeout = SMALL_TEST)

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:135:
@DisabledTest(message = "crbug.com/683153")
On 2017/02/16 at 23:37:24, timvolodine wrote:
> don't think this should be here?

Done

[the real yoland, 2017-02-17 01:25:26]

Rebased
https://codereview.chromium.org/2633733002/diff2/80001:120001/android_webview...

[timvolodine, 2017-02-17 13:58:26]

thanks, lgtm (with one question below)

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:98:
@Test
On 2017/02/17 01:15:13, the real yoland wrote:
> On 2017/02/16 at 23:37:24, timvolodine wrote:
> > are the @Test required? we already have @MediumTest below, so looks a bit
> weird having it twice..
> 
> ya, unfortunately JUnit4 requires `@Test` annotation (we can override the test
> runner to look for @Small/Medium/LargeTest annotations, but it would not align
> with the other android instrumentation tests style, also, @Test provides
useful
> functionalities like `@Test(expect=Exception)`)
> 
> Since we only use the @Small/Medium/LargeTest annotations to identify how long
> it takes for the test to run, we might in the future get rid of it and just
use
> @Test(timeout = SMALL_TEST)

I see. I assume the @MediumTest etc annotation is still taken into account when
running these tests right?

[timvolodine, 2017-02-17 14:00:30]

also, maybe add a bit more description (which is currently empty) e.g.
explaining why (and how)

[the real yoland, 2017-02-17 16:51:16]

Description was changed from

==========
Convert WebView Layout test to JUnit4

BUG=640116
==========

to

==========
Convert WebView Layout test to JUnit4

Since Android N came out, previous JUnit3 instrumentation tests are now
deprecated.
We are now gradually converting all the instrumentation tests to JUnit4 style.
For details on this migration, please check the bug.

BUG=640116
==========

[the real yoland, 2017-02-17 16:51:24]

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

[the real yoland, 2017-02-17 16:51:29]

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
File
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java
(right):

https://codereview.chromium.org/2633733002/diff/80001/android_webview/tools/s...
android_webview/tools/system_webview_shell/layout_tests/src/org/chromium/webview_shell/test/WebViewLayoutTest.java:98:
@Test
On 2017/02/17 at 13:58:26, timvolodine wrote:
> On 2017/02/17 01:15:13, the real yoland wrote:
> > On 2017/02/16 at 23:37:24, timvolodine wrote:
> > > are the @Test required? we already have @MediumTest below, so looks a bit
> > weird having it twice..
> > 
> > ya, unfortunately JUnit4 requires `@Test` annotation (we can override the
test
> > runner to look for @Small/Medium/LargeTest annotations, but it would not
align
> > with the other android instrumentation tests style, also, @Test provides
useful
> > functionalities like `@Test(expect=Exception)`)
> > 
> > Since we only use the @Small/Medium/LargeTest annotations to identify how
long
> > it takes for the test to run, we might in the future get rid of it and just
use
> > @Test(timeout = SMALL_TEST)
> 
> I see. I assume the @MediumTest etc annotation is still taken into account
when running these tests right?

They are taken account on the host python side instead of java test runner.

[commit-bot: I haz the power, 2017-02-17 16:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 18:04:05]

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

[commit-bot: I haz the power, 2017-02-17 18:04:06]

Dry run: This issue passed the CQ dry run.

[the real yoland, 2017-02-17 19:24:28]

The CQ bit was checked by yolandyan@chromium.org

[the real yoland, 2017-02-17 19:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mikecase@chromium.org
Link to the patchset: https://codereview.chromium.org/2633733002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-17 19:25:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 19:59:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487359468032970,
"parent_rev": "f903348fa83ee0e48eb45a1b850bcd16ab230392", "commit_rev":
"9dfd7fc55c1a6803577c5a1dc38a6e144139f101"}

[commit-bot: I haz the power, 2017-02-17 19:59:36]

Description was changed from

==========
Convert WebView Layout test to JUnit4

Since Android N came out, previous JUnit3 instrumentation tests are now
deprecated.
We are now gradually converting all the instrumentation tests to JUnit4 style.
For details on this migration, please check the bug.

BUG=640116
==========

to

==========
Convert WebView Layout test to JUnit4

Since Android N came out, previous JUnit3 instrumentation tests are now
deprecated.
We are now gradually converting all the instrumentation tests to JUnit4 style.
For details on this migration, please check the bug.

BUG=640116

Review-Url: https://codereview.chromium.org/2633733002
Cr-Commit-Position: refs/heads/master@{#451362}
Committed:
https://chromium.googlesource.com/chromium/src/+/9dfd7fc55c1a6803577c5a1dc38a...
==========

[commit-bot: I haz the power, 2017-02-17 19:59:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/9dfd7fc55c1a6803577c5a1dc38a...