Convert chrome compositor test's InstrumentationTestCases

Convert chrome compositor test's InstrumentationTestCases

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

[the real yoland, 2017-03-20 23:01:09]

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

[commit-bot: I haz the power, 2017-03-20 23:01:38]

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

[the real yoland, 2017-03-21 00:51:39]

The CQ bit was unchecked by yolandyan@chromium.org

[the real yoland, 2017-03-21 00:52:06]

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

[the real yoland, 2017-03-21 00:52:23]

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

[commit-bot: I haz the power, 2017-03-21 00:53:08]

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-03-21 01:03:38]

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

[commit-bot: I haz the power, 2017-03-21 01:03:39]

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

[the real yoland, 2017-03-21 23:59:28]

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

[commit-bot: I haz the power, 2017-03-21 23:59:57]

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-03-22 00:34:04]

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

[commit-bot: I haz the power, 2017-03-22 00:34:05]

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

[the real yoland, 2017-03-22 01:17:19]

Patchset #3 (id:40001) has been deleted

[the real yoland, 2017-03-22 01:17:24]

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

[commit-bot: I haz the power, 2017-03-22 01:18:02]

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

[the real yoland, 2017-03-22 02:10:16]

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

[commit-bot: I haz the power, 2017-03-22 02:11:16]

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

[the real yoland, 2017-03-22 05:24:29]

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

[commit-bot: I haz the power, 2017-03-22 05:25:08]

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-03-22 07:48:42]

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

[commit-bot: I haz the power, 2017-03-22 07:48:43]

Dry run: This issue passed the CQ dry run.

[the real yoland, 2017-03-22 15:39:58]

This CL is ready

[jbudorick, 2017-03-22 15:58:07]

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:67:
mTargetContext = targetContext;
This should not store the target context. RestrictionSkipCheck already does so.

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:82:
mDaydreamApi = vrClassesBuilder.createVrDaydreamApi(mTargetContext);
Perhaps we should expose that context via a method in RestrictionSkipCheck,
though.

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java:20:
*  A custom runner for JUnit4 tests for chrome java tests
nit: A custom runner for //chrome JUnit4 tests.

[Ted C, 2017-03-22 16:32:51]

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:36:
public UiThreadTestRule mRule = new UiThreadTestRule();
I'm confused why you need to add this here.  I thought this was only as a
replacement for android.test.UiThreadTest.

This also changes @Before and @After to run on the UI thread where previously it
was running on the instrumentation thread.

This seems like a behavioral change and I'm surprised to see that.

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelEventFilterTest.java:66:
public UiThreadTestRule mRule = new UiThreadTestRule();
Same comment

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManagerTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelManagerTest.java:33:
public UiThreadTestRule mRule = new UiThreadTestRule();
Same (applies to all further usage if there are any)

[the real yoland, 2017-03-22 17:43:33]

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:36:
public UiThreadTestRule mRule = new UiThreadTestRule();
On 2017/03/22 at 16:32:51, Ted C wrote:
> I'm confused why you need to add this here.  I thought this was only as a
replacement for android.test.UiThreadTest.
> 
> This also changes @Before and @After to run on the UI thread where previously
it was running on the instrumentation thread.
> 
> This seems like a behavioral change and I'm surprised to see that.

Currently, to make the @UiThreadTest annotation effective, UiThreadTestRule must
be added in the test suite. But UiThreadTestRule without the @UiThreadTest
doesn't do anything. UiThreadTestRule only makes test method annotated with
@UiThreadTest.

The next version of android test support library will support @UiThreadTest
without UiThreadTestRule, at that time, we can delete these UiThreadTestRules if
they are not used in test class

There is a CL to update this part of documentation:
https://codereview.chromium.org/2749353002

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:67:
mTargetContext = targetContext;
On 2017/03/22 at 15:58:07, jbudorick wrote:
> This should not store the target context. RestrictionSkipCheck already does
so.

Done

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:82:
mDaydreamApi = vrClassesBuilder.createVrDaydreamApi(mTargetContext);
On 2017/03/22 at 15:58:07, jbudorick wrote:
> Perhaps we should expose that context via a method in RestrictionSkipCheck,
though.

Done

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeJUnit4ClassRunner.java:20:
*  A custom runner for JUnit4 tests for chrome java tests
On 2017/03/22 at 15:58:07, jbudorick wrote:
> nit: A custom runner for //chrome JUnit4 tests.

Done

[Ted C, 2017-03-22 17:54:35]

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:36:
public UiThreadTestRule mRule = new UiThreadTestRule();
On 2017/03/22 17:43:33, the real yoland wrote:
> On 2017/03/22 at 16:32:51, Ted C wrote:
> > I'm confused why you need to add this here.  I thought this was only as a
> replacement for android.test.UiThreadTest.
> > 
> > This also changes @Before and @After to run on the UI thread where
previously
> it was running on the instrumentation thread.
> > 
> > This seems like a behavioral change and I'm surprised to see that.
> 
> Currently, to make the @UiThreadTest annotation effective, UiThreadTestRule
must
> be added in the test suite. But UiThreadTestRule without the @UiThreadTest
> doesn't do anything. UiThreadTestRule only makes test method annotated with
> @UiThreadTest.
> 
> The next version of android test support library will support @UiThreadTest
> without UiThreadTestRule, at that time, we can delete these UiThreadTestRules
if
> they are not used in test class
> 
> There is a CL to update this part of documentation:
> https://codereview.chromium.org/2749353002

Yes, but why did we need to add UiThreadTest at all?

Did these tests fail?  What exactly failed?  Looking at the documentation
you linked, I suspect there was some dependency on a handler/looper that
was previously on the instrumentation thread.  What was that?

[the real yoland, 2017-03-22 18:30:06]

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java
(right):

https://codereview.chromium.org/2760153002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBaseTest.java:36:
public UiThreadTestRule mRule = new UiThreadTestRule();
On 2017/03/22 at 17:54:35, Ted C wrote:
> On 2017/03/22 17:43:33, the real yoland wrote:
> > On 2017/03/22 at 16:32:51, Ted C wrote:
> > > I'm confused why you need to add this here.  I thought this was only as a
> > replacement for android.test.UiThreadTest.
> > > 
> > > This also changes @Before and @After to run on the UI thread where
previously
> > it was running on the instrumentation thread.
> > > 
> > > This seems like a behavioral change and I'm surprised to see that.
> > 
> > Currently, to make the @UiThreadTest annotation effective, UiThreadTestRule
must
> > be added in the test suite. But UiThreadTestRule without the @UiThreadTest
> > doesn't do anything. UiThreadTestRule only makes test method annotated with
> > @UiThreadTest.
> > 
> > The next version of android test support library will support @UiThreadTest
> > without UiThreadTestRule, at that time, we can delete these
UiThreadTestRules if
> > they are not used in test class
> > 
> > There is a CL to update this part of documentation:
> > https://codereview.chromium.org/2749353002
> 
> Yes, but why did we need to add UiThreadTest at all?
> 
> Did these tests fail?  What exactly failed?  Looking at the documentation
> you linked, I suspect there was some dependency on a handler/looper that
> was previously on the instrumentation thread.  What was that?

Yes it does fail, it is usually because test would run something that requires
handler (e.g. create a new Activity).

Unlike Junit3's instrumentation runner, JUnit4 test runner prevent handler from
created. So the solution would be running
test on UiThread

In this specific case, it fails because in setUp stage, when creating
MockOverlayPanel, a GestureEventFilter is created, which require a new handler
(https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...)

Since during real usage case, GestureEventFilter is probably created on Ui
thread. In test, it should be on ui thread as well.

[Ted C, 2017-03-23 03:36:01]

lgtm...thank you for the explanation

[the real yoland, 2017-03-23 04:04:43]

On 2017/03/23 at 03:36:01, tedchoc wrote:
> lgtm...thank you for the explanation

np!

[the real yoland, 2017-03-23 04:04:47]

The CQ bit was checked by yolandyan@chromium.org

[commit-bot: I haz the power, 2017-03-23 04:05:03]

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-03-23 04:10:49]

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

[commit-bot: I haz the power, 2017-03-23 04:10:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbudorick, 2017-03-23 04:37:24]

On 2017/03/23 04:10:50, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

lgtm

[the real yoland, 2017-03-23 16:20:21]

The CQ bit was checked by yolandyan@chromium.org

[commit-bot: I haz the power, 2017-03-23 16:20:50]

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-03-23 16:41:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490286021210830,
"parent_rev": "6935a6b5c4cac7b654a002b7c80c133d458bfe39", "commit_rev":
"a524a80c71ca1aa177b1a714fc0ec0c618d1aa15"}

[commit-bot: I haz the power, 2017-03-23 16:42:24]

Description was changed from

==========
Convert chrome compositor test's InstrumentationTestCases

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

to

==========
Convert chrome compositor test's InstrumentationTestCases

BUG=640116

Review-Url: https://codereview.chromium.org/2760153002
Cr-Commit-Position: refs/heads/master@{#459116}
Committed:
https://chromium.googlesource.com/chromium/src/+/a524a80c71ca1aa177b1a714fc0e...
==========

[commit-bot: I haz the power, 2017-03-23 16:42:25]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a524a80c71ca1aa177b1a714fc0e...