Android gtest runner: Add --enable-concurrent-adb flag

Android gtest runner: Add --enable-concurrent-adb flag

Flag is disabled by default as it's somewhat experimental.

This brings the time to the start of tests from 3 seconds -> 2 seconds.

BUG=540857
Committed: https://crrev.com/8bcb52eb6468d4eae7461d93bb1c750e495b687d
Cr-Commit-Position: refs/heads/master@{#355135}

[agrieve, 2015-10-19 20:56:28]

Description was changed from

==========
Android gtest runner: Add --enable-concurrent-adb flag

And enable it by default for run_${suite}_incremental scripts

This brings the time to the start of tests from 3 seconds -> 2 seconds.

BUG=540857
==========

to

==========
Android gtest runner: Add --enable-concurrent-adb flag

And enable it by default for run_${suite}_incremental scripts

This brings the time to the start of tests from 3 seconds -> 2 seconds.

BUG=540857
==========

[agrieve, 2015-10-19 20:56:28]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org

[agrieve, 2015-10-19 20:56:34]

On 2015/10/19 20:56:28, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

[jbudorick, 2015-10-19 21:01:28]

https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
(install_apk, push_test_data, init_tool_and_start_servers)
This scares me, and I'd prefer we not do it. While adb can handle multiple
connections to the same device, I'm pretty sure it doesn't always behave in such
situations. I don't think a 1s gain is worth the potential flakes.

[agrieve, 2015-10-19 21:21:06]

On 2015/10/19 21:01:28, jbudorick wrote:
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> File build/android/pylib/local/device/local_device_gtest_run.py (right):
> 
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
> (install_apk, push_test_data, init_tool_and_start_servers)
> This scares me, and I'd prefer we not do it. While adb can handle multiple
> connections to the same device, I'm pretty sure it doesn't always behave in
such
> situations. I don't think a 1s gain is worth the potential flakes.

I agree this is too scary to enable for bots, but for local testing it's on by
default for incremental installs and it seems to be working fine so far.

[jbudorick, 2015-10-19 23:49:31]

On 2015/10/19 21:21:06, agrieve wrote:
> On 2015/10/19 21:01:28, jbudorick wrote:
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > 
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
> > (install_apk, push_test_data, init_tool_and_start_servers)
> > This scares me, and I'd prefer we not do it. While adb can handle multiple
> > connections to the same device, I'm pretty sure it doesn't always behave in
> such
> > situations. I don't think a 1s gain is worth the potential flakes.
> 
> I agree this is too scary to enable for bots, but for local testing it's on by
> default for incremental installs and it seems to be working fine so far.

I would expect this to usually work. I'm worried about how often it fails, what
the failure modes look like, how easy they are to diagnose, and above all, how
frustrating they'll be to devs.

[agrieve, 2015-10-20 01:23:18]

On 2015/10/19 23:49:31, jbudorick wrote:
> On 2015/10/19 21:21:06, agrieve wrote:
> > On 2015/10/19 21:01:28, jbudorick wrote:
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
> > > (install_apk, push_test_data, init_tool_and_start_servers)
> > > This scares me, and I'd prefer we not do it. While adb can handle multiple
> > > connections to the same device, I'm pretty sure it doesn't always behave
in
> > such
> > > situations. I don't think a 1s gain is worth the potential flakes.
> > 
> > I agree this is too scary to enable for bots, but for local testing it's on
by
> > default for incremental installs and it seems to be working fine so far.
> 
> I would expect this to usually work. I'm worried about how often it fails,
what
> the failure modes look like, how easy they are to diagnose, and above all, how
> frustrating they'll be to devs.

Fair enough. How about I switch the flag off by default? That way I can enable
it manually for a while to give it more testing.

Might make sense to hook it up to the --no-threading flag of
incremental_install, but that would slow down things too much I think.

[jbudorick, 2015-10-20 13:03:06]

On 2015/10/20 01:23:18, agrieve wrote:
> On 2015/10/19 23:49:31, jbudorick wrote:
> > On 2015/10/19 21:21:06, agrieve wrote:
> > > On 2015/10/19 21:01:28, jbudorick wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
> > > > (install_apk, push_test_data, init_tool_and_start_servers)
> > > > This scares me, and I'd prefer we not do it. While adb can handle
multiple
> > > > connections to the same device, I'm pretty sure it doesn't always behave
> in
> > > such
> > > > situations. I don't think a 1s gain is worth the potential flakes.
> > > 
> > > I agree this is too scary to enable for bots, but for local testing it's
on
> by
> > > default for incremental installs and it seems to be working fine so far.
> > 
> > I would expect this to usually work. I'm worried about how often it fails,
> what
> > the failure modes look like, how easy they are to diagnose, and above all,
how
> > frustrating they'll be to devs.
> 
> Fair enough. How about I switch the flag off by default? That way I can enable
> it manually for a while to give it more testing.
> 
> Might make sense to hook it up to the --no-threading flag of
> incremental_install, but that would slow down things too much I think.

[jbudorick, 2015-10-20 13:03:06]

On 2015/10/20 01:23:18, agrieve wrote:
> On 2015/10/19 23:49:31, jbudorick wrote:
> > On 2015/10/19 21:21:06, agrieve wrote:
> > > On 2015/10/19 21:01:28, jbudorick wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > File build/android/pylib/local/device/local_device_gtest_run.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > build/android/pylib/local/device/local_device_gtest_run.py:257: steps =
> > > > (install_apk, push_test_data, init_tool_and_start_servers)
> > > > This scares me, and I'd prefer we not do it. While adb can handle
multiple
> > > > connections to the same device, I'm pretty sure it doesn't always behave
> in
> > > such
> > > > situations. I don't think a 1s gain is worth the potential flakes.
> > > 
> > > I agree this is too scary to enable for bots, but for local testing it's
on
> by
> > > default for incremental installs and it seems to be working fine so far.
> > 
> > I would expect this to usually work. I'm worried about how often it fails,
> what
> > the failure modes look like, how easy they are to diagnose, and above all,
how
> > frustrating they'll be to devs.
> 
> Fair enough. How about I switch the flag off by default? That way I can enable
> it manually for a while to give it more testing.
> 
> Might make sense to hook it up to the --no-threading flag of
> incremental_install, but that would slow down things too much I think.

[jbudorick, 2015-10-20 13:04:04]

On 2015/10/20 13:03:06, jbudorick wrote:

Double empty review, hooray.

> On 2015/10/20 01:23:18, agrieve wrote:
> > On 2015/10/19 23:49:31, jbudorick wrote:
> > > On 2015/10/19 21:21:06, agrieve wrote:
> > > > On 2015/10/19 21:01:28, jbudorick wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > > File build/android/pylib/local/device/local_device_gtest_run.py
(right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > > build/android/pylib/local/device/local_device_gtest_run.py:257: steps
=
> > > > > (install_apk, push_test_data, init_tool_and_start_servers)
> > > > > This scares me, and I'd prefer we not do it. While adb can handle
> multiple
> > > > > connections to the same device, I'm pretty sure it doesn't always
behave
> > in
> > > > such
> > > > > situations. I don't think a 1s gain is worth the potential flakes.
> > > > 
> > > > I agree this is too scary to enable for bots, but for local testing it's
> on
> > by
> > > > default for incremental installs and it seems to be working fine so far.
> > > 
> > > I would expect this to usually work. I'm worried about how often it fails,
> > what
> > > the failure modes look like, how easy they are to diagnose, and above all,
> how
> > > frustrating they'll be to devs.
> > 
> > Fair enough. How about I switch the flag off by default? That way I can
enable
> > it manually for a while to give it more testing.

I'd be ok with this.

> > 
> > Might make sense to hook it up to the --no-threading flag of
> > incremental_install, but that would slow down things too much I think.

[agrieve, 2015-10-20 17:34:09]

Description was changed from

==========
Android gtest runner: Add --enable-concurrent-adb flag

And enable it by default for run_${suite}_incremental scripts

This brings the time to the start of tests from 3 seconds -> 2 seconds.

BUG=540857
==========

to

==========
Android gtest runner: Add --enable-concurrent-adb flag

Flag is disabled by default as it's somewhat experimental.

This brings the time to the start of tests from 3 seconds -> 2 seconds.

BUG=540857
==========

[agrieve, 2015-10-20 17:34:27]

On 2015/10/20 13:04:04, jbudorick wrote:
> On 2015/10/20 13:03:06, jbudorick wrote:
> 
> Double empty review, hooray.
> 
> > On 2015/10/20 01:23:18, agrieve wrote:
> > > On 2015/10/19 23:49:31, jbudorick wrote:
> > > > On 2015/10/19 21:21:06, agrieve wrote:
> > > > > On 2015/10/19 21:01:28, jbudorick wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > > > File build/android/pylib/local/device/local_device_gtest_run.py
> (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1414093002/diff/1/build/android/pylib/local/d...
> > > > > > build/android/pylib/local/device/local_device_gtest_run.py:257:
steps
> =
> > > > > > (install_apk, push_test_data, init_tool_and_start_servers)
> > > > > > This scares me, and I'd prefer we not do it. While adb can handle
> > multiple
> > > > > > connections to the same device, I'm pretty sure it doesn't always
> behave
> > > in
> > > > > such
> > > > > > situations. I don't think a 1s gain is worth the potential flakes.
> > > > > 
> > > > > I agree this is too scary to enable for bots, but for local testing
it's
> > on
> > > by
> > > > > default for incremental installs and it seems to be working fine so
far.
> > > > 
> > > > I would expect this to usually work. I'm worried about how often it
fails,
> > > what
> > > > the failure modes look like, how easy they are to diagnose, and above
all,
> > how
> > > > frustrating they'll be to devs.
> > > 
> > > Fair enough. How about I switch the flag off by default? That way I can
> enable
> > > it manually for a while to give it more testing.
> 
> I'd be ok with this.
Done
> 
> > > 
> > > Might make sense to hook it up to the --no-threading flag of
> > > incremental_install, but that would slow down things too much I think.

[jbudorick, 2015-10-20 17:35:32]

lgtm

[agrieve, 2015-10-20 18:24:43]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-10-20 18:27:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414093002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414093002/20001

[commit-bot: I haz the power, 2015-10-20 19:38:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-20 19:39:19]

Patchset 2 (id:??) landed as
https://crrev.com/8bcb52eb6468d4eae7461d93bb1c750e495b687d
Cr-Commit-Position: refs/heads/master@{#355135}