Fix flaky testWebView on Android

Fix flaky testWebView on Android

BUG=452266
Committed: https://crrev.com/db0e8ddf9c524249b40504e5de3ec36076aa6ce0
Cr-Commit-Position: refs/heads/master@{#313414}

[Zhen Wang, 2015-01-27 18:41:55]

zhenw@chromium.org changed reviewers:
+ chrishenry@google.com, nednguyen@google.com

[Zhen Wang, 2015-01-27 18:41:56]

ptal

[chrishenry, 2015-01-27 18:45:07]

https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/android_app_unittest.py (right):

https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/android_app_unittest.py:30:
@decorators.Disabled('android') # crbug.com/452266
Would the right fix be simply adding a None check to the AndroidApp code?
Looking briefly at the bug, that looks to be the case. In that case, you should
just add a None check there.

[nednguyen, 2015-01-27 18:45:10]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-01-27 18:45:11]

lgtm

[commit-bot: I haz the power, 2015-01-27 18:46:27]

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

[nednguyen, 2015-01-27 18:48:07]

https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/android_app_unittest.py (right):

https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/android_app_unittest.py:30:
@decorators.Disabled('android') # crbug.com/452266
Just realize that this disable decorator magic may not work in this case.

[nednguyen, 2015-01-27 18:48:12]

The CQ bit was unchecked by nednguyen@google.com

[nednguyen, 2015-01-27 18:53:12]

On 2015/01/27 18:48:07, nednguyen wrote:
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/android_app_unittest.py (right):
> 
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/android_app_unittest.py:30:
> @decorators.Disabled('android') # crbug.com/452266
> Just realize that this disable decorator magic may not work in this case.

I am pretty sure that it doesn't work in this case. i.e: this test will run as
long as you plugin the android device, unlike the normal browser test case which
you have to specify --browser=.... You may want to use @Disable() completely.

[nednguyen, 2015-01-27 18:53:13]

On 2015/01/27 18:48:07, nednguyen wrote:
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/android_app_unittest.py (right):
> 
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/android_app_unittest.py:30:
> @decorators.Disabled('android') # crbug.com/452266
> Just realize that this disable decorator magic may not work in this case.

I am pretty sure that it doesn't work in this case. i.e: this test will run as
long as you plugin the android device, unlike the normal browser test case which
you have to specify --browser=.... You may want to use @Disable() completely.

[Zhen Wang, 2015-01-27 19:02:51]

On 2015/01/27 18:53:13, nednguyen wrote:
> On 2015/01/27 18:48:07, nednguyen wrote:
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > File tools/telemetry/telemetry/core/android_app_unittest.py (right):
> > 
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > tools/telemetry/telemetry/core/android_app_unittest.py:30:
> > @decorators.Disabled('android') # crbug.com/452266
> > Just realize that this disable decorator magic may not work in this case.
> 
> I am pretty sure that it doesn't work in this case. i.e: this test will run as
> long as you plugin the android device, unlike the normal browser test case
which
> you have to specify --browser=.... You may want to use @Disable() completely.

Will @Disable work? I use:

@decorators.Disabled

But when I use command "tools/telemetry/run_tests --browser=android-chrome-shell
AndroidAppTest" for local testing. The test still run.

[nednguyen, 2015-01-27 19:08:39]

On 2015/01/27 19:02:51, Zhen Wang wrote:
> On 2015/01/27 18:53:13, nednguyen wrote:
> > On 2015/01/27 18:48:07, nednguyen wrote:
> > >
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > > File tools/telemetry/telemetry/core/android_app_unittest.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > > tools/telemetry/telemetry/core/android_app_unittest.py:30:
> > > @decorators.Disabled('android') # crbug.com/452266
> > > Just realize that this disable decorator magic may not work in this case.
> > 
> > I am pretty sure that it doesn't work in this case. i.e: this test will run
as
> > long as you plugin the android device, unlike the normal browser test case
> which
> > you have to specify --browser=.... You may want to use @Disable()
completely.
> 
> Will @Disable work? I use:
> 
> @decorators.Disabled
> 
> But when I use command "tools/telemetry/run_tests
--browser=android-chrome-shell
> AndroidAppTest" for local testing. The test still run.

Hmh, probably just does the None check that Chris suggests then.

[chrishenry, 2015-01-27 19:09:16]

On 2015/01/27 19:02:51, Zhen Wang wrote:
> On 2015/01/27 18:53:13, nednguyen wrote:
> > On 2015/01/27 18:48:07, nednguyen wrote:
> > >
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > > File tools/telemetry/telemetry/core/android_app_unittest.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/858093003/diff/1/tools/telemetry/telemetry/co...
> > > tools/telemetry/telemetry/core/android_app_unittest.py:30:
> > > @decorators.Disabled('android') # crbug.com/452266
> > > Just realize that this disable decorator magic may not work in this case.
> > 
> > I am pretty sure that it doesn't work in this case. i.e: this test will run
as
> > long as you plugin the android device, unlike the normal browser test case
> which
> > you have to specify --browser=.... You may want to use @Disable()
completely.
> 
> Will @Disable work? I use:
> 
> @decorators.Disabled
> 
> But when I use command "tools/telemetry/run_tests
--browser=android-chrome-shell
> AndroidAppTest" for local testing. The test still run.

Have you tried changing this:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

to:
  def _IsAppReady(self):
    if self._is_app_ready_predicate is None:
      return True
    return self._is_app_ready_predicate(self.app)

Would this fix the test?

[Zhen Wang, 2015-01-27 19:16:13]

None check fix the problem (locally tested). But I am not sure if it is the
right thing to do in your context. If it is ok. I will just fix it in that way.

[chrishenry, 2015-01-27 19:22:57]

On 2015/01/27 19:16:13, Zhen Wang wrote:
> None check fix the problem (locally tested). But I am not sure if it is the
> right thing to do in your context. If it is ok. I will just fix it in that
way.

It is the right solution as ctor accepts None as default value, so it should do
the reasonable thing in that case, not raising exception. An alternative would
be to set the field in ctor to a function that returns True if the value is
None. Either way works...

When you're at it, please add ctor pydoc as to what happen if
is_app_ready_predicate is not passed to the ctore.

[Zhen Wang, 2015-01-27 21:21:40]

I add the pydoc now. ptal

[nednguyen, 2015-01-27 21:25:54]

https://codereview.chromium.org/858093003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/858093003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:24:
AndroidAppBackend is ready by default, i.e., _IsAppReady() returns True.
This document should be moved to android_platform.LaunchAndroidApplication
method, which is a public API.

[Zhen Wang, 2015-01-27 21:31:34]

On 2015/01/27 21:25:54, nednguyen wrote:
>
https://codereview.chromium.org/858093003/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):
> 
>
https://codereview.chromium.org/858093003/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/android_app_backend.py:24:
> AndroidAppBackend is ready by default, i.e., _IsAppReady() returns True.
> This document should be moved to android_platform.LaunchAndroidApplication
> method, which is a public API.

Ah, right. Fixed

[nednguyen, 2015-01-27 21:32:45]

lgtm

[Zhen Wang, 2015-01-27 22:10:45]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2015-01-27 22:11:41]

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

[commit-bot: I haz the power, 2015-01-27 22:20:22]

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

[commit-bot: I haz the power, 2015-01-27 22:20:23]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[nednguyen, 2015-01-27 22:20:41]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-01-27 22:21:15]

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

[commit-bot: I haz the power, 2015-01-27 22:21:54]

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

[commit-bot: I haz the power, 2015-01-27 22:21:56]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Zhen Wang, 2015-01-28 00:23:52]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2015-01-28 00:24:48]

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

[commit-bot: I haz the power, 2015-01-28 00:26:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-28 00:27:23]

Patchset 4 (id:??) landed as
https://crrev.com/db0e8ddf9c524249b40504e5de3ec36076aa6ce0
Cr-Commit-Position: refs/heads/master@{#313414}