Make test filters play nice

base/test/launcher/test_launcher.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/test/launcher/test_launcher.h"
 
 #include <memory>
 
 #include "base/at_exit.h"
 #include "base/bind.h"
@@ skipping 808 matching lines @@
   fprintf(stdout, "Using %" PRIuS " parallel jobs.\n", parallel_jobs_);
   fflush(stdout);
   if (parallel_jobs_ > 1U) {
     worker_pool_owner_ = MakeUnique<SequencedWorkerPoolOwner>(
         parallel_jobs_, "test_launcher");
   } else {
     worker_thread_ = MakeUnique<Thread>("test_launcher");
     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(
         command_line->GetSwitchValuePath(switches::kTestLauncherFilterFile));
     std::string filter;
     if (!ReadFileToString(filter_file_path, &filter)) {
       LOG(ERROR) << "Failed to read the filter file.";
       return false;
     }
 
     // Parse the file contents (see //testing/buildbot/filters/README.md
     // for file syntax and other info).
     std::vector<std::string> filter_lines = SplitString(
         filter, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
     for (const std::string& filter_line : filter_lines) {
       if (filter_line.empty() || filter_line[0] == '#')
         continue;
 
       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 up to the dash.
+    positive_gtest_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 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);
     }
   }
 
   if (!launcher_delegate_->GetTests(&tests_)) {
     LOG(ERROR) << "Failed to get list of tests.";
     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;
   }
 
 #if defined(NDEBUG)
   results_tracker_.AddGlobalTag("MODE_RELEASE");
 #else
   results_tracker_.AddGlobalTag("MODE_DEBUG");
 #endif
@@ skipping 50 matching lines @@
   results_tracker_.AddGlobalTag("CPU_32_BITS");
 #endif
 
 #if defined(ARCH_CPU_64_BITS)
   results_tracker_.AddGlobalTag("CPU_64_BITS");
 #endif
 
   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);
 
     const CommandLine* command_line = CommandLine::ForCurrentProcess();
     if (test_name.find("DISABLED") != std::string::npos) {
       results_tracker_.AddDisabledTest(test_name);
 
       // Skip disabled tests unless explicitly requested.
       if (!command_line->HasSwitch(kGTestRunDisabledTestsFlag))
         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]) ||
             MatchPattern(test_name_no_disabled, positive_test_filter_[k])) {
           found = true;
           break;
         }
       }
 
       if (!found)
         continue;
     }
     if (!negative_test_filter_.empty()) {
       bool excluded = false;
       for (size_t k = 0; k < negative_test_filter_.size(); ++k) {

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

same comment about the range-based loop.

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

Done.

         if (MatchPattern(test_name, negative_test_filter_[k]) ||
             MatchPattern(test_name_no_disabled, negative_test_filter_[k])) {
           excluded = true;
           break;
         }
       }
 
       if (excluded)
         continue;
     }
@@ skipping 159 matching lines @@
   }
 
   std::string snippet(full_output.substr(run_pos));
   if (end_pos != std::string::npos)
     snippet = full_output.substr(run_pos, end_pos - run_pos);
 
   return snippet;
 }
 
 }  // namespace base