Convert Vr tests to JUnit4

Convert Vr tests to JUnit4

Convert Vr tests in chrome_public_test_vr_apk to JUnit4. For more detail,
please check src/testing/android/docs/junit4.md

BUG=640116

[the real yoland, 2017-05-03 02:07:25]

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

[commit-bot: I haz the power, 2017-05-03 02:07:43]

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-05-03 03:35:15]

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

[commit-bot: I haz the power, 2017-05-03 03:35:16]

Dry run: This issue passed the CQ dry run.

[the real yoland, 2017-05-03 16:26:05]

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

[jbudorick, 2017-05-03 16:39:11]

https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
(right):

https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:304:
/**
Would it make sense to put these in a VrTestRule rather than moving them into
WebVrTest? Given that they're in VrTestBase at the moment, I imagine the idea
was that they might be useful in other tests.

[bsheedy, 2017-05-03 16:41:21]

On 2017/05/03 16:39:11, jbudorick wrote:
>
https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
> (right):
> 
>
https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:304:
> /**
> Would it make sense to put these in a VrTestRule rather than moving them into
> WebVrTest? Given that they're in VrTestBase at the moment, I imagine the idea
> was that they might be useful in other tests.

This is exactly right. They were recently moved OUT of WebVR test for this exact
purpose, as WebVR will need to be tested in conjunction with the VR browser (see
https://codereview.chromium.org/2851313002/) in addition to being tested by
itself like it is now.

[the real yoland, 2017-05-03 16:51:33]

On 2017/05/03 at 16:41:21, bsheedy wrote:
> On 2017/05/03 16:39:11, jbudorick wrote:
> >
https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
> > File
> >
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java
> > (right):
> > 
> >
https://codereview.chromium.org/2857583005/diff/1/chrome/android/javatests/sr...
> >
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:304:
> > /**
> > Would it make sense to put these in a VrTestRule rather than moving them
into
> > WebVrTest? Given that they're in VrTestBase at the moment, I imagine the
idea
> > was that they might be useful in other tests.
> 
> This is exactly right. They were recently moved OUT of WebVR test for this
exact purpose, as WebVR will need to be tested in conjunction with the VR
browser (see https://codereview.chromium.org/2851313002/) in addition to being
tested by itself like it is now.

I see, I will put it in its separate rule there.

[the real yoland, 2017-05-09 02:36:25]

Patchset #2 (id:20001) has been deleted

[the real yoland, 2017-05-09 23:33:14]

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

[the real yoland, 2017-05-09 23:33:21]

Done

[commit-bot: I haz the power, 2017-05-09 23:35:51]

Dry run: CQ is trying da patch.

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

[jbudorick, 2017-05-09 23:52:41]

https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java
(right):

https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:51:
public class VrTestRule extends ChromeTabbedActivityTestRule {
Given how little this class interacts with its superclass, I'm surprised by the
choice to extend it. Why not just have the tests use both a
ChromeTabbedActivityTestRule for the normal activity things and a VrTestRule for
the VR-specific things?

[bsheedy, 2017-05-10 00:00:38]

On 2017/05/09 23:52:41, jbudorick wrote:
>
https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java
> (right):
> 
>
https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:51:
> public class VrTestRule extends ChromeTabbedActivityTestRule {
> Given how little this class interacts with its superclass, I'm surprised by
the
> choice to extend it. Why not just have the tests use both a
> ChromeTabbedActivityTestRule for the normal activity things and a VrTestRule
for
> the VR-specific things?

LGTM w/ John's suggestion - I imagine it extends it since it's a direct port of
VrTestBase to VrTestRule, and VrTestBase originally extended
ChromeTabbedActivityTestBase.

[commit-bot: I haz the power, 2017-05-10 00:16:34]

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

[commit-bot: I haz the power, 2017-05-10 00:16:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[the real yoland, 2017-05-10 00:56:45]

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

[the real yoland, 2017-05-10 00:56:51]

https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java
(right):

https://codereview.chromium.org/2857583005/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:51:
public class VrTestRule extends ChromeTabbedActivityTestRule {
On 2017/05/09 at 23:52:41, jbudorick wrote:
> Given how little this class interacts with its superclass, I'm surprised by
the choice to extend it. Why not just have the tests use both a
ChromeTabbedActivityTestRule for the normal activity things and a VrTestRule for
the VR-specific things?

Done

[commit-bot: I haz the power, 2017-05-10 00:57:34]

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-05-10 02:59:33]

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

[commit-bot: I haz the power, 2017-05-10 02:59:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[the real yoland, 2017-05-10 18:24:48]

yolandyan@chromium.org changed reviewers:
+ bshe@chromium.org, dtrainor@chromium.org

[the real yoland, 2017-05-10 18:24:48]

+dtrainor for owner stamp on chrome/android/BUILD.gn

+bshe for owner stamp on chrome/android/javatests/*/vr_shell/*

[bsheedy, 2017-05-10 18:28:47]

bsheedy@chromium.org changed reviewers:
+ mthiesse@chromium.org
- bshe@chromium.org

[bsheedy, 2017-05-10 18:28:48]

-bshe@ since he's on paternity leave

+mthiesse@ to replace him

[jbudorick, 2017-05-10 18:31:13]

https://codereview.chromium.org/2857583005/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java
(right):

https://codereview.chromium.org/2857583005/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:52:
public class VrTestRule extends ChromeActivityTestRule<ChromeTabbedActivity> {
This ... isn't much of a change. All of the Activity stuff is still being routed
through VrTestRule.

I'll follow up with you offline.

[David Trainor- moved to gerrit, 2017-05-10 20:15:56]

chrome/android/BUILD.gn lgtm

[mthiesse, 2017-05-10 21:18:06]

lgtm once jbudorick is happy

[the real yoland, 2017-05-10 22:07:40]

Changed to VrTestRule to not inherit any ActivityTestRule

[jbudorick, 2017-05-10 22:14:12]

lgtm

[the real yoland, 2017-05-11 19:14:32]

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

[commit-bot: I haz the power, 2017-05-11 19:15:34]

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-05-11 20:34:29]

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

[commit-bot: I haz the power, 2017-05-11 20:34:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bsheedy, 2017-05-11 23:08:53]

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

[commit-bot: I haz the power, 2017-05-11 23:10:27]

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-05-12 01:36:02]

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

[commit-bot: I haz the power, 2017-05-12 01:36:03]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[bsheedy, 2017-05-12 17:02:12]

In addition to the numerous null pointer exceptions that are happening now, the
way you refactored VrTestRule doesn't work as intended anymore.

Right now, you're passing in a CTA instance and getting the active tab's
WebContents if necessary - in some cases, we explicitly need the WebContents of
a tab other than the currently active one (e.g. in
WebVrTest#testPoseDataUnfocusedTab), which is why we passed in the WebContents
object before.