Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2515573003:
Make test filters play nice (Closed)
DescriptionMake test filters play nice
We want to be able to use --test-launcher-filter-file
AND --gtest_filter.
A test is run if and only if:
A) positive_patterns_from_gtest_filter are empty,
or the test matches at least one pattern from
positive_patterns_from_gtest_filter
AND
B) positive_patterns_from_test_launcher_filter_file are empty,
or the test matches at least one pattern from
positive_patterns_from_test_launcher_filter_file
AND
C) the test doesn't match ANY negative_patterns_from_gtest_filter
AND
D) the test doesn't match ANY
negative_patterns_from_test_launcher_filter_file
R=dpranke@chromium.org
BUG=587527
Committed: https://crrev.com/8857248be5dd8829d99b4505dc21a85d232e0db5
Cr-Commit-Position: refs/heads/master@{#435680}
Patch Set 1 #Patch Set 2 : Make test filters play nice #
Total comments: 11
Patch Set 3 : Make test filters play nice #
Total comments: 14
Patch Set 4 : Make test filters play nice #
Total comments: 6
Patch Set 5 : Make test filters play nice #
Total comments: 2
Messages
Total messages: 26 (9 generated)
The CQ bit was checked by katthomas@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + phajdan.jr@chromium.org
I don't think this is implementing what the description says (and the
description is saying what I think we want).
In particular, if I do:
gn gen out/Release
ninja -C out/Release gn_unittests
echo "Visibililty.*" > filter
out/Release/gn_unittests --test-launcher-filter-filter=filters
--gtest_filter=Visibility.Public
You'll run 3 tests (the same as Visibility.*) and not just Visibility.Public.
i.e., you're getting the union of the positive filters, not the intersection.
I believe we want the union of the negative filters and the intersection of the
positive filters (at least, that's my interpretation of
https://bugs.chromium.org/p/chromium/issues/detail?id=587527#c34 ).
Yup. One way to do that could be to have another set of variables (positive and negative) for filter from file. Then when we check, require that _both_ positive filters match, and _none_ of the negative ones match.
Oh I see, I misread the description. Thanks! On Tue, Nov 22, 2016 at 3:20 AM <phajdan.jr@chromium.org> wrote: > Yup. One way to do that could be to have another set of variables > (positive and > negative) for filter from file. > > Then when we check, require that _both_ positive filters match, and _none_ > of > the negative ones match. > > https://codereview.chromium.org/2515573003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL
https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.cc:857: for (std::string test_name : SplitString(filter, ":", base::TRIM_WHITESPACE, nit: Why do we need the loops for the positive filter where previously this was just an assignment? Negative filter indeed needs a loop, since we're possibly adding the results from the file. Although since file code already has a loop, and flag code doesn't have to, how about putting the flag code first and keep assignments in it? https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.cc:953: // Compute the intersection of the gtest and filter file positive filters Could you explain more reasoning behind this? It seems simpler to just literally check that flag filter matches (or is empty), AND file filter matches (or is empty). Where it gets tricky is e.g. one specifying A.* and the other A.B . These are different strings, and should result in just A.B being run. I'm not sure why the code sometimes refers to some of positive_test_filter_file_, positive_test_filter_gtest_, positive_test_filter_ but not others, which doesn't seem to be consistent. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.h (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:184: std::vector<std::string> positive_test_filter_gtest_; nit: I'd suggest "flag", "cmdline", or similar suffix. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:186: std::vector<std::string> positive_test_filter_; Do we need this old variable? If yes, why?
https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.cc:953: // Compute the intersection of the gtest and filter file positive filters I'm interpreting "Could you explain more" as "write a much longer comment", and I agree with that. I would also move all of 953-1027 out into a separate function that computes and returns the list of tests to run (but doesn't run them). As written here, this routine is doing too many (i.e., more than 1) things. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.h (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:187: std::vector<std::string> negative_test_filter_; Even though you only need a single negative_test_filter_, it might be easier to follow the code if you had two of them, to mirror the positive filters.
PTAL https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.cc:857: for (std::string test_name : SplitString(filter, ":", base::TRIM_WHITESPACE, Oops, should have changed that back. Thanks for catching that. On 2016/11/28 12:56:14, Paweł Hajdan Jr. wrote: > nit: Why do we need the loops for the positive filter where previously this was > just an assignment? > > Negative filter indeed needs a loop, since we're possibly adding the results > from the file. Although since file code already has a loop, and flag code > doesn't have to, how about putting the flag code first and keep assignments in > it? https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.cc:953: // Compute the intersection of the gtest and filter file positive filters On 2016/11/29 02:07:15, Dirk Pranke wrote: > I'm interpreting "Could you explain more" as "write a much longer comment", and > I agree with that. > > I would also move all of 953-1027 out into a separate function that computes and > returns the list of tests to run (but doesn't run them). As written here, this > routine is doing too many (i.e., more than 1) things. > Oh right, I didn't realize you could match patterns. Of course this makes sense. Thank you. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... File base/test/launcher/test_launcher.h (right): https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:184: std::vector<std::string> positive_test_filter_gtest_; On 2016/11/28 12:56:14, Paweł Hajdan Jr. wrote: > nit: I'd suggest "flag", "cmdline", or similar suffix. Acknowledged. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:186: std::vector<std::string> positive_test_filter_; On 2016/11/28 12:56:14, Paweł Hajdan Jr. wrote: > Do we need this old variable? If yes, why? Yes, it is still used. https://codereview.chromium.org/2515573003/diff/20001/base/test/launcher/test... base/test/launcher/test_launcher.h:187: std::vector<std::string> negative_test_filter_; On 2016/11/29 02:07:15, Dirk Pranke wrote: > Even though you only need a single negative_test_filter_, it might be easier to > follow the code if you had two of them, to mirror the positive filters. Done.
https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:829: if (command_line->HasSwitch(switches::kTestLauncherFilterFile)) { If we're extracting code, I'd suggest this flag-related code to get extracted as well. It could be a separate method or combined with CombineTestFilters. Suggested name could be (Get|Extract|Compute)TestFilters. Also see if that way we can reduce number of member variables. It'd be more maintainable to store intermediary state in local variables. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:869: negative_test_filter_gtest_.push_back(test_name); Since negative_test_filter_gtest_ doesn't collide with anything else, we should be able to just assign result of SplitString instead of looping. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:962: found_gtest = MatchPattern(test_name, positive_test_filter_gtest_[k]); This assignment overwrites any previous result. A test will likely match one of the patterns, but not all of them. Shouldn't found_* be OR-ed with the previous value? https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:971: } else if (!positive_test_filter_file_.empty()) { I'm not sure if this case-based code is the simplest way to express. How about we extract a method which checks if a test matches filters, and call that in place of code near "// Skip the test that doesn't match the filter (if given)." comment? https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:988: TestLauncher::CombineTestFilters(); nit: Is the "TestLauncher::" prefix needed here? https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1018: if (!positive_test_filter_.empty() || has_positive_filter) { Why do we need a separate "has_positive_filter" variable? https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... File base/test/launcher/test_launcher.h (right): https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.h:186: bool has_positive_filter; nit: All member variables should end with "_". By the way, do we really need this variable?
PTAL https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:829: if (command_line->HasSwitch(switches::kTestLauncherFilterFile)) { On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > If we're extracting code, I'd suggest this flag-related code to get extracted as > well. It could be a separate method or combined with CombineTestFilters. > Suggested name could be (Get|Extract|Compute)TestFilters. > > Also see if that way we can reduce number of member variables. It'd be more > maintainable to store intermediary state in local variables. I agree it would be better if that parsing was extracted to a helper method, but I think that refactor should be separate from this change. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:869: negative_test_filter_gtest_.push_back(test_name); On 2016/11/30 12:41:31, Paweł Hajdan Jr. wrote: > Since negative_test_filter_gtest_ doesn't collide with anything else, we should > be able to just assign result of SplitString instead of looping. Done. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:962: found_gtest = MatchPattern(test_name, positive_test_filter_gtest_[k]); On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > This assignment overwrites any previous result. A test will likely match one of > the patterns, but not all of them. > > Shouldn't found_* be OR-ed with the previous value? Oof, I didn't catch this because I wasn't as thorough with my test cases yesterday. Thanks for catching. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:971: } else if (!positive_test_filter_file_.empty()) { On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > I'm not sure if this case-based code is the simplest way to express. > > How about we extract a method which checks if a test matches filters, and call > that in place of code near "// Skip the test that doesn't match the filter (if > given)." comment? Didn't totally follow your advice here, but got the point that this has gotten a little gross. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:988: TestLauncher::CombineTestFilters(); On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > nit: Is the "TestLauncher::" prefix needed here? Done. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1018: if (!positive_test_filter_.empty() || has_positive_filter) { On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > Why do we need a separate "has_positive_filter" variable? Because there is the possibility that there both flags are provided with positive filters, and there is no intersection. https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... File base/test/launcher/test_launcher.h (right): https://codereview.chromium.org/2515573003/diff/40001/base/test/launcher/test... base/test/launcher/test_launcher.h:186: bool has_positive_filter; On 2016/11/30 12:41:32, Paweł Hajdan Jr. wrote: > nit: All member variables should end with "_". > > By the way, do we really need this variable? We don't _need_ it, but I think it makes the code more readable and easier to understand.
I mostly defer to Paweł here, but I think you're pretty close! https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1015: for (size_t k = 0; k < positive_test_filter_.size(); ++k) { The code is easier to read as a range-based loop, I think, e.g.: for (auto filter: postive_test_filter_) { if (MatchPattern(test_name, filter) || MatchPattern(test_name_no_disabled, filter)) { ... } Also, I'm not sure that has_positive_filter_ really is easier to follow than (!postive_gtest_filter_.empty() || !postive_file_filter_.empty()). I found myself wondering how has_positive_filter was defined and how it was different from !positive_test_filter_.empty(), and I think spelling it out helps. https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1028: for (size_t k = 0; k < negative_test_filter_.size(); ++k) { same comment about the range-based loop.
LGTM w/comment Thanks for the patch! https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:957: if (has_positive_filter_) { nit: Instead of big nested block, return early: if (!has_positive_filter_) return;
https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:957: if (has_positive_filter_) { On 2016/12/01 17:50:43, Paweł Hajdan Jr. wrote: > nit: Instead of big nested block, return early: > > if (!has_positive_filter_) > return; Done. https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1015: for (size_t k = 0; k < positive_test_filter_.size(); ++k) { On 2016/12/01 01:40:00, Dirk Pranke wrote: > The code is easier to read as a range-based loop, I think, e.g.: > > for (auto filter: postive_test_filter_) { > if (MatchPattern(test_name, filter) || > MatchPattern(test_name_no_disabled, filter)) { > ... > } > > Also, I'm not sure that has_positive_filter_ really is easier to follow than > (!postive_gtest_filter_.empty() || !postive_file_filter_.empty()). > > I found myself wondering how has_positive_filter was defined and how > it was different from !positive_test_filter_.empty(), > and I think spelling it out helps. Ah, good point. Well, those aren't member variables anymore, so perhaps this longer but more descriptive name will do? Also, I didn't know about `auto`, so thanks for suggesting that! https://codereview.chromium.org/2515573003/diff/60001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1028: for (size_t k = 0; k < negative_test_filter_.size(); ++k) { On 2016/12/01 01:40:00, Dirk Pranke wrote: > same comment about the range-based loop. Done.
The CQ bit was checked by katthomas@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2515573003/#ps80001 (title: "Make test filters play nice")
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": 80001, "attempt_start_ts": 1480619223388870,
"parent_rev": "3379d7b889a6139f52aef801c5e28337704248c3", "commit_rev":
"5a6c56539f6aab2b2e6662930ee5543aa57224e7"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make test filters play nice We want to be able to use --test-launcher-filter-file AND --gtest_filter. A test is run if and only if: A) positive_patterns_from_gtest_filter are empty, or the test matches at least one pattern from positive_patterns_from_gtest_filter AND B) positive_patterns_from_test_launcher_filter_file are empty, or the test matches at least one pattern from positive_patterns_from_test_launcher_filter_file AND C) the test doesn't match ANY negative_patterns_from_gtest_filter AND D) the test doesn't match ANY negative_patterns_from_test_launcher_filter_file R=dpranke@chromium.org BUG=587527 ========== to ========== Make test filters play nice We want to be able to use --test-launcher-filter-file AND --gtest_filter. A test is run if and only if: A) positive_patterns_from_gtest_filter are empty, or the test matches at least one pattern from positive_patterns_from_gtest_filter AND B) positive_patterns_from_test_launcher_filter_file are empty, or the test matches at least one pattern from positive_patterns_from_test_launcher_filter_file AND C) the test doesn't match ANY negative_patterns_from_gtest_filter AND D) the test doesn't match ANY negative_patterns_from_test_launcher_filter_file R=dpranke@chromium.org BUG=587527 Committed: https://crrev.com/8857248be5dd8829d99b4505dc21a85d232e0db5 Cr-Commit-Position: refs/heads/master@{#435680} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8857248be5dd8829d99b4505dc21a85d232e0db5 Cr-Commit-Position: refs/heads/master@{#435680}
Message was sent while issue was closed.
https://codereview.chromium.org/2515573003/diff/80001/base/test/launcher/test... File base/test/launcher/test_launcher.cc (right): https://codereview.chromium.org/2515573003/diff/80001/base/test/launcher/test... base/test/launcher/test_launcher.cc:959: } else { If you have the early return on line 958, you don't also need the else clause. https://codereview.chromium.org/2515573003/diff/80001/base/test/launcher/test... base/test/launcher/test_launcher.cc:1015: if (has_at_least_one_positive_filter_) { This is a better name, thanks! |
