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..11ab0995d9ee126f86c9426ed327b7f7ef7de0f4 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,42 @@ bool TestLauncher::Init() {
   return true;
 }
 
+void TestLauncher::CombinePositiveTestFilters(
+    std::vector<std::string> filter_a,
+    std::vector<std::string> filter_b) {
+  has_positive_filter_ = !filter_a.empty() || !filter_b.empty();
+  if (has_positive_filter_) {

[Paweł Hajdan Jr., 2016/12/01 17:50:43]

nit: Instead of big nested block, return early:

if (!has_positive_filter_)
  return;

[katthomas, 2016/12/01 19:06:52]

Done.

+    // 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,19 +998,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 (has_positive_filter_) {
       bool found = false;
       for (size_t k = 0; k < positive_test_filter_.size(); ++k) {

[Dirk Pranke, 2016/12/01 01:40:00]

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.

[katthomas, 2016/12/01 19:06:52]

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!

         if (MatchPattern(test_name, positive_test_filter_[k]) ||