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..858df5c0b2d3fd9de87fc4975948c99845902925 100644
--- a/base/test/launcher/test_launcher.cc
+++ b/base/test/launcher/test_launcher.cc
@@ -826,12 +826,8 @@ 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;
-  }
+  std::vector<std::string> positive_file_filter;
+  std::vector<std::string> positive_gtest_filter;
 
   if (command_line->HasSwitch(switches::kTestLauncherFilterFile)) {
     base::FilePath filter_file_path = base::MakeAbsoluteFilePath(
@@ -853,26 +849,27 @@ bool TestLauncher::Init() {
       if (filter_line[0] == '-')
         negative_test_filter_.push_back(filter_line.substr(1));
       else
-        positive_test_filter_.push_back(filter_line);
+        positive_file_filter.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_gtest_filter =
+        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_gtest_filter =
+        SplitString(filter.substr(0, dash_pos), ":", base::TRIM_WHITESPACE,
+                    base::SPLIT_WANT_ALL);
+
+    // Everything after the dash.
+    for (std::string pattern :
+         SplitString(filter.substr(dash_pos + 1), ":", base::TRIM_WHITESPACE,
+                     base::SPLIT_WANT_ALL)) {
+      negative_test_filter_.push_back(pattern);
     }
   }
 
@@ -881,6 +878,8 @@ bool TestLauncher::Init() {
     return false;
   }
 
+  CombinePositiveTestFilters(positive_gtest_filter, positive_file_filter);
+
   if (!results_tracker_.Init(*command_line)) {
     LOG(ERROR) << "Failed to initialize test results tracker.";
     return 1;
@@ -951,11 +950,44 @@ bool TestLauncher::Init() {
   return true;
 }
 
+void TestLauncher::CombinePositiveTestFilters(
+    std::vector<std::string> filter_a,
+    std::vector<std::string> filter_b) {
+  has_at_least_one_positive_filter_ = !filter_a.empty() || !filter_b.empty();
+  if (!has_at_least_one_positive_filter_) {
+    return;
+  } else {

[Dirk Pranke, 2016/12/01 20:42:56]

If you have the early return on line 958, you don't also need the else clause.

+    // If two positive filters are present, only run tests that match a pattern
+    // in both filters.
+    if (!filter_a.empty() && !filter_b.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_a = false;
+        bool found_b = false;
+        for (size_t k = 0; k < filter_a.size(); ++k) {
+          found_a = found_a || MatchPattern(test_name, filter_a[k]);
+        }
+        for (size_t k = 0; k < filter_b.size(); ++k) {
+          found_b = found_b || MatchPattern(test_name, filter_b[k]);
+        }
+        if (found_a && found_b) {
+          positive_test_filter_.push_back(test_name);
+        }
+      }
+    } else if (!filter_a.empty()) {
+      positive_test_filter_ = filter_a;
+    } else {
+      positive_test_filter_ = filter_b;
+    }
+  }
+}
+
 void TestLauncher::RunTests() {
   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,23 +1000,23 @@ 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 (has_at_least_one_positive_filter_) {

[Dirk Pranke, 2016/12/01 20:42:56]

This is a better name, thanks!

       bool found = false;
-      for (size_t k = 0; k < positive_test_filter_.size(); ++k) {
-        if (MatchPattern(test_name, positive_test_filter_[k]) ||
-            MatchPattern(test_name_no_disabled, positive_test_filter_[k])) {
+      for (auto filter : positive_test_filter_) {
+        if (MatchPattern(test_name, filter) ||
+            MatchPattern(test_name_no_disabled, filter)) {
           found = true;
           break;
         }
@@ -995,9 +1027,9 @@ void TestLauncher::RunTests() {
     }
     if (!negative_test_filter_.empty()) {
       bool excluded = false;
-      for (size_t k = 0; k < negative_test_filter_.size(); ++k) {
-        if (MatchPattern(test_name, negative_test_filter_[k]) ||
-            MatchPattern(test_name_no_disabled, negative_test_filter_[k])) {
+      for (auto filter : negative_test_filter_) {
+        if (MatchPattern(test_name, filter) ||
+            MatchPattern(test_name_no_disabled, filter)) {
           excluded = true;
           break;
         }