Android gtest runner: Only query device for test list when necessary

Android gtest runner: Only query device for test list when necessary

When --gtest_filter is used and contains a list of tests without
wildcards, it is not necessary to query the device for the list of
tests.

This speeds up local development where it's comment to run just a single
test at a time (saves 3 seconds).

BUG=540857
Committed: https://crrev.com/065d7462c301d58b12415c7591e5647ab766fe56
Cr-Commit-Position: refs/heads/master@{#353901}

[agrieve, 2015-10-08 02:58:55]

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

[agrieve, 2015-10-08 02:59:00]

On 2015/10/08 02:58:55, agrieve wrote:
> mailto:agrieve@chromium.org changed reviewers:
> + mailto:jbudorick@chromium.org

[agrieve, 2015-10-08 02:59:22]

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

[commit-bot: I haz the power, 2015-10-08 02:59:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393223002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393223002/1

[commit-bot: I haz the power, 2015-10-08 05:19:23]

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

[commit-bot: I haz the power, 2015-10-08 05:19:23]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2015-10-08 15:01:46]

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

https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:56: def
_ExtractTestsFromFilter(gtest_filter):
It's hard for me to describe why, but this whole mechanism feels like a code
smell. I think I see what you're trying to do, but I'm not sure this is the
right way to accomplish it.

https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:259: # list since
test output is not retrieve until after the test completes.
Why do you say that it still makes sense to retrieve the test list with one
device, but not if the filter matches some set of heuristics?

also, nit: retrieved

[agrieve, 2015-10-08 15:59:47]

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

https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:56: def
_ExtractTestsFromFilter(gtest_filter):
On 2015/10/08 15:01:46, jbudorick wrote:
> It's hard for me to describe why, but this whole mechanism feels like a code
> smell. I think I see what you're trying to do, but I'm not sure this is the
> right way to accomplish it.

Yeah, I had at first just cached the test list like instrumentation tests do,
but then realized the cache would be invalidated each time you re-built the
test, which is pretty much every time when you're trying to fix / debug a test.

I think it'd be fine to remove the .* case since that's the opinionated one, but
I have already used it to run a set of failing tests before so wanted to add it
in.

https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_gtest_run.py:259: # list since
test output is not retrieve until after the test completes.
On 2015/10/08 15:01:46, jbudorick wrote:
> Why do you say that it still makes sense to retrieve the test list with one
> device, but not if the filter matches some set of heuristics?
> 
> also, nit: retrieved

Clarified (but not 100% sure it's even the case, so please tell me if it's
wrong)

[agrieve, 2015-10-13 15:23:30]

On 2015/10/08 15:59:47, agrieve wrote:
>
https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
> File build/android/pylib/local/device/local_device_gtest_run.py (right):
> 
>
https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
> build/android/pylib/local/device/local_device_gtest_run.py:56: def
> _ExtractTestsFromFilter(gtest_filter):
> On 2015/10/08 15:01:46, jbudorick wrote:
> > It's hard for me to describe why, but this whole mechanism feels like a code
> > smell. I think I see what you're trying to do, but I'm not sure this is the
> > right way to accomplish it.
> 
> Yeah, I had at first just cached the test list like instrumentation tests do,
> but then realized the cache would be invalidated each time you re-built the
> test, which is pretty much every time when you're trying to fix / debug a
test.
> 
> I think it'd be fine to remove the .* case since that's the opinionated one,
but
> I have already used it to run a set of failing tests before so wanted to add
it
> in.
> 
>
https://codereview.chromium.org/1393223002/diff/1/build/android/pylib/local/d...
> build/android/pylib/local/device/local_device_gtest_run.py:259: # list since
> test output is not retrieve until after the test completes.
> On 2015/10/08 15:01:46, jbudorick wrote:
> > Why do you say that it still makes sense to retrieve the test list with one
> > device, but not if the filter matches some set of heuristics?
> > 
> > also, nit: retrieved
> 
> Clarified (but not 100% sure it's even the case, so please tell me if it's
> wrong)

Any more thoughts on this? This change was actually the largest speed-up of the
things I tried.

[jbudorick, 2015-10-13 17:17:48]

lgtm w/ nit

https://codereview.chromium.org/1393223002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393223002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:258: # When the list
of tests to run is given via command-line, skip querying the
As written, this is structured in a way that implies that this extraction isn't
a very specialized case, which is a bit backwards.

As for an actionable nit: this can go before list_tests, since that isn't used
if we are able to extract a list of tests from the filter.

[agrieve, 2015-10-13 20:13:27]

https://codereview.chromium.org/1393223002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_gtest_run.py (right):

https://codereview.chromium.org/1393223002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_gtest_run.py:258: # When the list
of tests to run is given via command-line, skip querying the
On 2015/10/13 17:17:48, jbudorick wrote:
> As written, this is structured in a way that implies that this extraction
isn't
> a very specialized case, which is a bit backwards.
> 
> As for an actionable nit: this can go before list_tests, since that isn't used
> if we are able to extract a list of tests from the filter.

Done (and updated comments to be a bit more special-casey.

[agrieve, 2015-10-13 20:16:39]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-10-13 20:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1393223002/#ps80001
(title: "y")

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

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

[commit-bot: I haz the power, 2015-10-13 23:23:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-13 23:24:39]

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