Reenable TabStripTest#testScrollingStripStackerScrollsToSelectedTab()

Reenable TabStripTest#testScrollingStripStackerScrollsToSelectedTab()

Poll for the selected tab to be visible rather than asserting after
waiting for idle sync. The application may be idle before the tab
is fully visible.

BUG=632843
Committed: https://crrev.com/812c3d355853b0f9be40f1fbde02604a0a1410d7
Cr-Commit-Position: refs/heads/master@{#411243}

[Theresa, 2016-08-10 17:18:47]

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

[Theresa, 2016-08-10 17:19:30]

twellington@chromium.org changed reviewers:
+ changwan@chromium.org

[Theresa, 2016-08-10 17:19:31]

ptal

[commit-bot: I haz the power, 2016-08-10 17:19:54]

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, 2016-08-10 18:29:27]

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

[commit-bot: I haz the power, 2016-08-10 18:29:28]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-08-11 01:27:35]

Thanks for re-enabling this. I've left some inline comments.

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:657:
getInstrumentation().waitForIdleSync();
This line is not needed.

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:660:
CriteriaHelper.pollUiThread(new Criteria(){
Per email discussion about test retry on failure, it would be better to use
CallbackHelper than polling on UI thread.

How about something like the following instead?

final CallbackHelper visibilityCallback = new CallbackHelper();

ThreadUtils.runOnUiThread(new Runnable() {
  @Override
  public void run() {
    tab.addObserver(new LayoutTab.Observer() {
      @Override
      public onVisibilityChange(boolean isVisible) {
        if (isVisible) {
          visibilityCallback.notifyCalled();
          tab.removeObserver(this);
        }
      }
    });
    TabModelUtils.setIndex(getActivity().getCurrentTabModel(), 0);
  }
});

visibilityCallback.waitForCallback(0);

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:1049:
private void assertTabVisibility(boolean shouldBeVisible, StripLayoutTab
tabView) {
I just realized that it is awkward to call tabView.isVisible() from test thread.
Could you fix it? There are also other places where we naively call UI methods
from the test thread, which I think is a technical debt from long time ago. I'll
leave the rest to your judgment.

[Theresa, 2016-08-11 01:41:08]

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

[Theresa, 2016-08-11 01:41:43]

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:657:
getInstrumentation().waitForIdleSync();
On 2016/08/11 01:27:35, Changwan Ryu wrote:
> This line is not needed.

Done.

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:660:
CriteriaHelper.pollUiThread(new Criteria(){
On 2016/08/11 01:27:35, Changwan Ryu wrote:
> Per email discussion about test retry on failure, it would be better to use
> CallbackHelper than polling on UI thread.
> 
> How about something like the following instead?
> 
> final CallbackHelper visibilityCallback = new CallbackHelper();
> 
> ThreadUtils.runOnUiThread(new Runnable() {
>   @Override
>   public void run() {
>     tab.addObserver(new LayoutTab.Observer() {
>       @Override
>       public onVisibilityChange(boolean isVisible) {
>         if (isVisible) {
>           visibilityCallback.notifyCalled();
>           tab.removeObserver(this);
>         }
>       }
>     });
>     TabModelUtils.setIndex(getActivity().getCurrentTabModel(), 0);
>   }
> });
> 
> visibilityCallback.waitForCallback(0);

I added a TODO as discussed offline since StripLayoutTab doesn't currently have
an observer interface.

https://codereview.chromium.org/2236603002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:1049:
private void assertTabVisibility(boolean shouldBeVisible, StripLayoutTab
tabView) {
On 2016/08/11 01:27:35, Changwan Ryu wrote:
> I just realized that it is awkward to call tabView.isVisible() from test
thread.
> Could you fix it? There are also other places where we naively call UI methods
> from the test thread, which I think is a technical debt from long time ago.
I'll
> leave the rest to your judgment.

Done.

[commit-bot: I haz the power, 2016-08-11 01:42:08]

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

[Changwan Ryu, 2016-08-11 01:46:59]

https://codereview.chromium.org/2236603002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/2236603002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:1056:
assertEquals(
I haven't tested this enough but it's always better to have assertions in the
test thread not UI thread (otherwise, test may end up passing while UI thread
checks it later...). How about something like

assertEquals(shouldBeVisible, ThreadUtils.runOnUiThreadBlocking(new
Callable<Boolean>(...));

[Theresa, 2016-08-11 02:09:05]

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

[Theresa, 2016-08-11 02:09:22]

https://codereview.chromium.org/2236603002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java
(right):

https://codereview.chromium.org/2236603002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:1056:
assertEquals(
On 2016/08/11 01:46:59, Changwan Ryu wrote:
> I haven't tested this enough but it's always better to have assertions in the
> test thread not UI thread (otherwise, test may end up passing while UI thread
> checks it later...). How about something like
> 
> assertEquals(shouldBeVisible, ThreadUtils.runOnUiThreadBlocking(new
> Callable<Boolean>(...));

Done.

[commit-bot: I haz the power, 2016-08-11 02:09:43]

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

[Changwan Ryu, 2016-08-11 02:11:11]

lgtm

[Theresa, 2016-08-11 02:11:39]

The CQ bit was unchecked by twellington@chromium.org

[Theresa, 2016-08-11 02:11:39]

The CQ bit was checked by twellington@chromium.org

[commit-bot: I haz the power, 2016-08-11 02:12:02]

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, 2016-08-11 03:06:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-11 03:07:49]

Description was changed from

==========
Reenable TabStripTest#testScrollingStripStackerScrollsToSelectedTab()

Poll for the selected tab to be visible rather than asserting after
waiting for idle sync. The application may be idle before the tab
is fully visible.

BUG=632843
==========

to

==========
Reenable TabStripTest#testScrollingStripStackerScrollsToSelectedTab()

Poll for the selected tab to be visible rather than asserting after
waiting for idle sync. The application may be idle before the tab
is fully visible.

BUG=632843

Committed: https://crrev.com/812c3d355853b0f9be40f1fbde02604a0a1410d7
Cr-Commit-Position: refs/heads/master@{#411243}
==========

[commit-bot: I haz the power, 2016-08-11 03:07:50]

Patchset 5 (id:??) landed as
https://crrev.com/812c3d355853b0f9be40f1fbde02604a0a1410d7
Cr-Commit-Position: refs/heads/master@{#411243}