Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 2 months ago by agrieve Modified:
5 years, 2 months ago Reviewers:
jbudorick CC:
chromium-reviews, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@gtest-faster-7 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid 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}
Patch Set 1 #
Total comments: 4
Patch Set 2 : reword comment #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : y #
Created: 5 years, 2 months ago
Depends on Patchset: Messages
Total messages: 16 (5 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
On 2015/10/08 02:58:55, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:jbudorick@chromium.org
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
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)
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.
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.
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.
The CQ bit was checked by agrieve@chromium.org
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")
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
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/065d7462c301d58b12415c7591e5647ab766fe56 Cr-Commit-Position: refs/heads/master@{#353901} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
