Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522303002:
Enforce ordering in FilterTestNames. (Closed)
|
Created:
5 years ago by alexandermont Modified:
5 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnforce ordering in FilterTestNames.
BUG=569723
Committed: https://crrev.com/daeb80935df8b1532de2cf12879ea3d40a0615ae
Cr-Commit-Position: refs/heads/master@{#365376}
Patch Set 1 #
Total comments: 2
Patch Set 2 : make FilterTestNames return results in order #Patch Set 3 : make FilterTestNames return results in order #
Total comments: 3
Patch Set 4 : make FilterTestNames return results in order #
Total comments: 2
Patch Set 5 : make FilterTestNames output results in order #Messages
Total messages: 23 (4 generated)
alexandermont@chromium.org changed reviewers: + jbudorick@chromium.org, mikecase@chromium.org
Change the filtering code to output the results of the filter in order of the patterns that they match.
I have 1 quick question. Otherwise, this CL looks good. https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... build/util/lib/common/unittest_util.py:141: pt_tests = [test for test in all_tests What does "pt" stand for?
https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... build/util/lib/common/unittest_util.py:144: not any(fnmatch(test, p) for p in positive_patterns[:i]))] I think you could make tests a set instead of a list while you're filtering and remove this third any clause.
On 2015/12/15 at 16:32:20, mikecase wrote: > I have 1 quick question. Otherwise, this CL looks good. > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > File build/util/lib/common/unittest_util.py (right): > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > build/util/lib/common/unittest_util.py:141: pt_tests = [test for test in all_tests > What does "pt" stand for? "pt" = "pattern" In other words, this variable stores the tests that match that one particular pattern.
On 2015/12/15 at 16:35:59, jbudorick wrote: > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > File build/util/lib/common/unittest_util.py (right): > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > build/util/lib/common/unittest_util.py:144: not any(fnmatch(test, p) for p in positive_patterns[:i]))] > I think you could make tests a set instead of a list while you're filtering and remove this third any clause. Sets aren't ordered.
On 2015/12/15 18:56:42, alexandermont wrote: > On 2015/12/15 at 16:35:59, jbudorick wrote: > > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > > File build/util/lib/common/unittest_util.py (right): > > > > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > > build/util/lib/common/unittest_util.py:144: not any(fnmatch(test, p) for p in > positive_patterns[:i]))] > > I think you could make tests a set instead of a list while you're filtering > and remove this third any clause. > > Sets aren't ordered. ah, right.
On 2015/12/15 18:57:34, jbudorick wrote: > On 2015/12/15 18:56:42, alexandermont wrote: > > On 2015/12/15 at 16:35:59, jbudorick wrote: > > > > > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > > > File build/util/lib/common/unittest_util.py (right): > > > > > > > > > https://codereview.chromium.org/1522303002/diff/1/build/util/lib/common/unitt... > > > build/util/lib/common/unittest_util.py:144: not any(fnmatch(test, p) for p > in > > positive_patterns[:i]))] > > > I think you could make tests a set instead of a list while you're filtering > > and remove this third any clause. > > > > Sets aren't ordered. > > ah, right. i suppose you could keep a set alongside the list and check that rather than doing the third any(fnmatch...) clause.
Updated version of CL
lgtm w/ nit https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... build/util/lib/common/unittest_util.py:142: ptrn_tests = [test for test in all_tests nit: Would prefer if this was named pattern_tests
https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... build/util/lib/common/unittest_util.py:142: ptrn_tests = [test for test in all_tests On 2015/12/15 at 19:20:28, mikecase wrote: > nit: Would prefer if this was named pattern_tests if you do this, it makes line 144 too long
https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/40001/build/util/lib/common/u... build/util/lib/common/unittest_util.py:142: ptrn_tests = [test for test in all_tests On 2015/12/15 19:37:53, alexandermont wrote: > On 2015/12/15 at 19:20:28, mikecase wrote: > > nit: Would prefer if this was named pattern_tests > > if you do this, it makes line 144 too long I'd also prefer pattern_tests. You may need to handle line breaks a bit differently.
Changed name of variable to pattern_tests.
lgtm w/ nit https://codereview.chromium.org/1522303002/diff/60001/build/util/lib/common/u... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/60001/build/util/lib/common/u... build/util/lib/common/unittest_util.py:144: if (fnmatch.fnmatch(test, pattern) and nit: move the two ands down to the beginning of the following line.
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1522303002/#ps80001 (title: "make FilterTestNames output results in order")
https://codereview.chromium.org/1522303002/diff/60001/build/util/lib/common/u... File build/util/lib/common/unittest_util.py (right): https://codereview.chromium.org/1522303002/diff/60001/build/util/lib/common/u... build/util/lib/common/unittest_util.py:144: if (fnmatch.fnmatch(test, pattern) and On 2015/12/15 at 21:52:19, jbudorick wrote: > nit: move the two ands down to the beginning of the following line. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522303002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522303002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enforce ordering in FilterTestNames. BUG=569723 ========== to ========== Enforce ordering in FilterTestNames. BUG=569723 Committed: https://crrev.com/daeb80935df8b1532de2cf12879ea3d40a0615ae Cr-Commit-Position: refs/heads/master@{#365376} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/daeb80935df8b1532de2cf12879ea3d40a0615ae Cr-Commit-Position: refs/heads/master@{#365376}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1532463002/ by alexandermont@chromium.org. The reason for reverting is: causing test failures.
On 2015/12/16 at 00:47:44, alexandermont wrote: > A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1532463002/ by alexandermont@chromium.org. > > The reason for reverting is: causing test failures. CL has been reverted. New version of CL is at: https://codereview.chromium.org/1531013002 |
