Make test filters play nice

base/test/launcher/test_launcher.cc

Index: base/test/launcher/test_launcher.cc
diff --git a/base/test/launcher/test_launcher.cc b/base/test/launcher/test_launcher.cc
index 8528b1d5fd906be59fcb11b9648981ff6996eff2..f6e07f31f0dd7fa5e7a57411999a4b90bbdc56f6 100644
--- a/base/test/launcher/test_launcher.cc
+++ b/base/test/launcher/test_launcher.cc
@@ -826,13 +826,6 @@ bool TestLauncher::Init() {
     worker_thread_->Start();
   }
 
-  if (command_line->HasSwitch(switches::kTestLauncherFilterFile) &&
-      command_line->HasSwitch(kGTestFilterFlag)) {
-    LOG(ERROR) << "Only one of --test-launcher-filter-file and --gtest_filter "
-               << "at a time is allowed.";
-    return false;
-  }
-
   if (command_line->HasSwitch(switches::kTestLauncherFilterFile)) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

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.

[katthomas, 2016/12/01 00:07:00]

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. 

     base::FilePath filter_file_path = base::MakeAbsoluteFilePath(
         command_line->GetSwitchValuePath(switches::kTestLauncherFilterFile));
@@ -851,28 +844,29 @@ bool TestLauncher::Init() {
         continue;
 
       if (filter_line[0] == '-')
-        negative_test_filter_.push_back(filter_line.substr(1));
+        negative_test_filter_file_.push_back(filter_line.substr(1));
       else
-        positive_test_filter_.push_back(filter_line);
+        positive_test_filter_file_.push_back(filter_line);
     }
+  }
+  // Split --gtest_filter at '-', if there is one, to separate into
+  // positive filter and negative filter portions.
+  std::string filter = command_line->GetSwitchValueASCII(kGTestFilterFlag);
+  size_t dash_pos = filter.find('-');
+  if (dash_pos == std::string::npos) {
+    positive_test_filter_gtest_ =
+        SplitString(filter, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   } else {
-    // Split --gtest_filter at '-', if there is one, to separate into
-    // positive filter and negative filter portions.
-    std::string filter = command_line->GetSwitchValueASCII(kGTestFilterFlag);
-    size_t dash_pos = filter.find('-');
-    if (dash_pos == std::string::npos) {
-      positive_test_filter_ = SplitString(
-          filter, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-    } else {
-      // Everything up to the dash.
-      positive_test_filter_ = SplitString(
-          filter.substr(0, dash_pos), ":", base::TRIM_WHITESPACE,
-          base::SPLIT_WANT_ALL);
-
-      // Everything after the dash.
-      negative_test_filter_ = SplitString(
-          filter.substr(dash_pos + 1), ":", base::TRIM_WHITESPACE,
-          base::SPLIT_WANT_ALL);
+    // Everything up to the dash.
+    positive_test_filter_gtest_ =
+        SplitString(filter.substr(0, dash_pos), ":", base::TRIM_WHITESPACE,
+                    base::SPLIT_WANT_ALL);
+
+    // Everything after the dash.
+    for (std::string test_name :
+         SplitString(filter.substr(dash_pos + 1), ":", base::TRIM_WHITESPACE,
+                     base::SPLIT_WANT_ALL)) {
+      negative_test_filter_gtest_.push_back(test_name);

[Paweł Hajdan Jr., 2016/11/30 12:41:31]

Since negative_test_filter_gtest_ doesn't collide with anything else, we should
be able to just assign result of SplitString instead of looping.

[katthomas, 2016/12/01 00:07:00]

Done.

     }
   }
 
@@ -951,11 +945,52 @@ bool TestLauncher::Init() {
   return true;
 }
 
+void TestLauncher::CombineTestFilters() {
+  bool has_positive_filter = !positive_test_filter_file_.empty() ||
+                             !positive_test_filter_gtest_.empty();
+  if (has_positive_filter) {
+    // If two positive filters are present, only run tests that match a pattern
+    // in both filters.
+    if (!positive_test_filter_file_.empty() &&
+        !positive_test_filter_gtest_.empty()) {
+      for (size_t i = 0; i < tests_.size(); i++) {
+        std::string test_name =
+            FormatFullTestName(tests_[i].test_case_name, tests_[i].test_name);
+        bool found_gtest = false;
+        bool found_file = false;
+        for (size_t k = 0; k < positive_test_filter_gtest_.size(); ++k) {
+          found_gtest = MatchPattern(test_name, positive_test_filter_gtest_[k]);

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

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?

[katthomas, 2016/12/01 00:07:00]

Oof, I didn't catch this because I wasn't as thorough with my test cases
yesterday. Thanks for catching.

+        }
+        for (size_t k = 0; k < positive_test_filter_file_.size(); ++k) {
+          found_file = MatchPattern(test_name, positive_test_filter_file_[k]);
+        }
+        if (found_gtest && found_file) {
+          positive_test_filter_.push_back(test_name);
+        }
+      }
+    } else if (!positive_test_filter_file_.empty()) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

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?

[katthomas, 2016/12/01 00:07:00]

Didn't totally follow your advice here, but got the point that this has gotten a
little gross.

+      positive_test_filter_ = positive_test_filter_file_;
+    } else {
+      positive_test_filter_ = positive_test_filter_gtest_;
+    }
+  }
+  negative_test_filter_.reserve(negative_test_filter_gtest_.size() +
+                                negative_test_filter_file_.size());
+  negative_test_filter_.insert(negative_test_filter_.end(),
+                               negative_test_filter_gtest_.begin(),
+                               negative_test_filter_gtest_.end());
+  negative_test_filter_.insert(negative_test_filter_.end(),
+                               negative_test_filter_file_.begin(),
+                               negative_test_filter_file_.end());
+}
+
 void TestLauncher::RunTests() {
+  TestLauncher::CombineTestFilters();

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

nit: Is the "TestLauncher::" prefix needed here?

[katthomas, 2016/12/01 00:07:00]

Done.

+
   std::vector<std::string> test_names;
   for (size_t i = 0; i < tests_.size(); i++) {
-    std::string test_name = FormatFullTestName(
-        tests_[i].test_case_name, tests_[i].test_name);
+    std::string test_name =
+        FormatFullTestName(tests_[i].test_case_name, tests_[i].test_name);
 
     results_tracker_.AddTest(test_name, tests_[i].file, tests_[i].line);
 
@@ -968,19 +1003,19 @@ void TestLauncher::RunTests() {
         continue;
     }
 
-    if (!launcher_delegate_->ShouldRunTest(
-        tests_[i].test_case_name, tests_[i].test_name)) {
+    if (!launcher_delegate_->ShouldRunTest(tests_[i].test_case_name,
+                                           tests_[i].test_name)) {
       continue;
     }
 
     // Count tests in the binary, before we apply filter and sharding.
     test_found_count_++;
 
-    std::string test_name_no_disabled = TestNameWithoutDisabledPrefix(
-        test_name);
+    std::string test_name_no_disabled =
+        TestNameWithoutDisabledPrefix(test_name);
 
     // Skip the test that doesn't match the filter (if given).
-    if (!positive_test_filter_.empty()) {
+    if (!positive_test_filter_.empty() || has_positive_filter) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

Why do we need a separate "has_positive_filter" variable?

[katthomas, 2016/12/01 00:07:00]

Because there is the possibility that there both flags are provided with
positive filters, and there is no intersection.

       bool found = false;
       for (size_t k = 0; k < positive_test_filter_.size(); ++k) {
         if (MatchPattern(test_name, positive_test_filter_[k]) ||