Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555633002:
[Android] Support gtest_filter + gtest_also_run_disabled_tests (Closed)
|
Created:
4 years ago by shenghuazhang Modified:
4 years ago Reviewers:
jbudorick Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Support gtest_filter + gtest_also_run_disabled_tests
Support gtest_filter + gtest_also_run_disabled_tests in the same way
that //base/test/launcher does.
See https://bugs.chromium.org/p/chromium/issues/detail?id=658892 for
details.
BUG=669632
Committed: https://crrev.com/61757ce0ebb1aef762564a649b2266d846f24adf
Cr-Commit-Position: refs/heads/master@{#437383}
Patch Set 1 #
Total comments: 8
Patch Set 2 : John comment #
Total comments: 5
Patch Set 3 : patterns nit #Patch Set 4 : Replace 'DISABLED_' in same way #
Messages
Total messages: 16 (5 generated)
shenghuazhang@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:230: def TestNameWithoutDisabledPrefix(test_name): This should have tests. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:239: disabled_prefixes = ['DISABLED_', 'FLAKY_'] compile DISABLED_ and FLAKY_ into regexes at module scope, e.g. _DISABLED_RE = re.compile('DISABLED_') and then use them in this function. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:241: if re.match(r'%s' % dp, test_name): I think you should be able to do this with one or two re.sub calls & no re.match or re.find calls. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:449: for test in test_list: While this appears to work, it looks like it'll be pretty inefficient w/ this loop deprefixing each test in each iteration of the outer loop + the 'test not in filtered_test_list' check. Can you tune it a bit?
https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:230: def TestNameWithoutDisabledPrefix(test_name): On 2016/12/06 03:22:05, jbudorick wrote: > This should have tests. Done. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:239: disabled_prefixes = ['DISABLED_', 'FLAKY_'] On 2016/12/06 03:22:05, jbudorick wrote: > compile DISABLED_ and FLAKY_ into regexes at module scope, e.g. > > _DISABLED_RE = re.compile('DISABLED_') > > and then use them in this function. Done. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:241: if re.match(r'%s' % dp, test_name): On 2016/12/06 03:22:05, jbudorick wrote: > I think you should be able to do this with one or two re.sub calls & no re.match > or re.find calls. Done. https://codereview.chromium.org/2555633002/diff/1/build/android/pylib/gtest/g... build/android/pylib/gtest/gtest_test_instance.py:449: for test in test_list: On 2016/12/06 03:22:05, jbudorick wrote: > While this appears to work, it looks like it'll be pretty inefficient w/ this > loop deprefixing each test in each iteration of the outer loop + the 'test not > in filtered_test_list' check. Can you tune it a bit? Refactor the code by escape the outer gtest_filter_strings loop. It will loop the rest tests only excluding filtered_test_list.
https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:244: test_name = re.sub(r'\.%s' % dp.pattern, '.', test_name) nit: you should be able to replace these two lines with: test_name = dp.sub('', test_name) https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:451: for test in out_filtered_test_list: 1) this is much better :) 2) to be consistent w/ the filtering above, what if you did: test_names_no_disabled = [ TestNameWithoutDisabledPrefix(t) for t in out_filtered_test_list] filtered_test_list.extend( unittest_util.FilterTestNames(test_names_no_disabled, self._gtest_filter) (I *think* that should work the same way.)
https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:244: test_name = re.sub(r'\.%s' % dp.pattern, '.', test_name) On 2016/12/08 01:32:57, jbudorick wrote: > nit: you should be able to replace these two lines with: > > test_name = dp.sub('', test_name) lgtm w/ this nit https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:451: for test in out_filtered_test_list: On 2016/12/08 01:32:57, jbudorick wrote: > 1) this is much better :) > 2) to be consistent w/ the filtering above, what if you did: > > test_names_no_disabled = [ > TestNameWithoutDisabledPrefix(t) > for t in out_filtered_test_list] > filtered_test_list.extend( > unittest_util.FilterTestNames(test_names_no_disabled, self._gtest_filter) > > (I *think* that should work the same way.) > spoke offline, this is fine.
Nit change the patterns. Please review it again. https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... File build/android/pylib/gtest/gtest_test_instance.py (right): https://codereview.chromium.org/2555633002/diff/20001/build/android/pylib/gte... build/android/pylib/gtest/gtest_test_instance.py:244: test_name = re.sub(r'\.%s' % dp.pattern, '.', test_name) On 2016/12/08 01:38:01, jbudorick wrote: > On 2016/12/08 01:32:57, jbudorick wrote: > > nit: you should be able to replace these two lines with: > > > > test_name = dp.sub('', test_name) > > lgtm w/ this nit Refactor this by 2 pattern groups. - _RE_DISABLED = re.compile(r'^DISABLED_') _RE_FLAKY = re.compile(r'^FLAKY_') Replace the pattern with ''. -_RE_DOT_DISABLED = re.compile(r'\.DISABLED_') _RE_DOT_FLAKY = re.compile(r'\.FLAKY_') Replace the pattern with '.'
nit change
nit change
The CQ bit was checked by shenghuazhang@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/2555633002/#ps60001 (title: "Replace 'DISABLED_' in same way")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481230192650640,
"parent_rev": "c55763d5c51d62183793375483b23a2f5e5b215f", "commit_rev":
"e850e9301dd87fbad19f92a2e3de05c72d3ee5eb"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Support gtest_filter + gtest_also_run_disabled_tests Support gtest_filter + gtest_also_run_disabled_tests in the same way that //base/test/launcher does. See https://bugs.chromium.org/p/chromium/issues/detail?id=658892 for details. BUG=669632 ========== to ========== [Android] Support gtest_filter + gtest_also_run_disabled_tests Support gtest_filter + gtest_also_run_disabled_tests in the same way that //base/test/launcher does. See https://bugs.chromium.org/p/chromium/issues/detail?id=658892 for details. BUG=669632 Committed: https://crrev.com/61757ce0ebb1aef762564a649b2266d846f24adf Cr-Commit-Position: refs/heads/master@{#437383} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/61757ce0ebb1aef762564a649b2266d846f24adf Cr-Commit-Position: refs/heads/master@{#437383} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
