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..668ab57864f18f99cf39ef62e755c9eb57dca943 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)) {
     base::FilePath filter_file_path = base::MakeAbsoluteFilePath(
         command_line->GetSwitchValuePath(switches::kTestLauncherFilterFile));
@@ -853,26 +846,31 @@ 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_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) {
+    for (std::string test_name : SplitString(filter, ":", base::TRIM_WHITESPACE,

[Paweł Hajdan Jr., 2016/11/28 12:56:14]

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?

[katthomas, 2016/11/29 21:38:26]

Oops, should have changed that back. Thanks for catching that.

On 2016/11/28 12:56:14, Paweł Hajdan Jr. wrote:

+                                             base::SPLIT_WANT_ALL)) {
+      positive_test_filter_gtest_.push_back(test_name);
     }
   } 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.
+    for (std::string test_name :
+         SplitString(filter.substr(0, dash_pos), ":", base::TRIM_WHITESPACE,
+                     base::SPLIT_WANT_ALL)) {
+      positive_test_filter_gtest_.push_back(test_name);
+    }
+
+    // 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_.push_back(test_name);
     }
   }
 
@@ -952,65 +950,81 @@ bool TestLauncher::Init() {
 }
 
 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);
-
-    results_tracker_.AddTest(test_name, tests_[i].file, tests_[i].line);
+  // Compute the intersection of the gtest and filter file positive filters

[Paweł Hajdan Jr., 2016/11/28 12:56:13]

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.

[Dirk Pranke, 2016/11/29 02:07:15]

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.

[katthomas, 2016/11/29 21:38:26]

Oh right, I didn't realize you could match patterns. Of course this makes sense.
Thank you.

+  std::sort(positive_test_filter_file_.begin(),
+            positive_test_filter_file_.end());
+  std::sort(positive_test_filter_gtest_.begin(),
+            positive_test_filter_gtest_.end());
+  std::set_intersection(
+      positive_test_filter_file_.begin(), positive_test_filter_file_.end(),
+      positive_test_filter_gtest_.begin(), positive_test_filter_gtest_.end(),
+      std::back_inserter(positive_test_filter_));
 
-    const CommandLine* command_line = CommandLine::ForCurrentProcess();
-    if (test_name.find("DISABLED") != std::string::npos) {
-      results_tracker_.AddDisabledTest(test_name);
+  std::vector<std::string> test_names;
+  bool has_positive_filter = !positive_test_filter_file_.empty() ||
+                             !positive_test_filter_gtest_.empty();
+  if (!has_positive_filter || !positive_test_filter_.empty() ||
+      !negative_test_filter_.empty()) {
+    for (size_t i = 0; i < tests_.size(); i++) {
+      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;
+      }
 
-      // Skip disabled tests unless explicitly requested.
-      if (!command_line->HasSwitch(kGTestRunDisabledTestsFlag))
+      if (!launcher_delegate_->ShouldRunTest(tests_[i].test_case_name,
+                                             tests_[i].test_name)) {
         continue;
-    }
-
-    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);
+      }
 
-    // Skip the test that doesn't match the filter (if given).
-    if (!positive_test_filter_.empty()) {
-      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])) {
-          found = true;
-          break;
+      // Count tests in the binary, before we apply filter and sharding.
+      test_found_count_++;
+
+      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()) {
+        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])) {
+            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) {
-        if (MatchPattern(test_name, negative_test_filter_[k]) ||
-            MatchPattern(test_name_no_disabled, negative_test_filter_[k])) {
-          excluded = true;
-          break;
+        if (!found)
+          continue;
+      }
+      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])) {
+            excluded = true;
+            break;
+          }
         }
+
+        if (excluded)
+          continue;
       }
 
-      if (excluded)
+      if (Hash(test_name) % total_shards_ !=
+          static_cast<uint32_t>(shard_index_))
         continue;
-    }
-
-    if (Hash(test_name) % total_shards_ != static_cast<uint32_t>(shard_index_))
-      continue;
 
-    test_names.push_back(test_name);
+      test_names.push_back(test_name);
+    }
   }
 
   // Save an early test summary in case the launcher crashes or gets killed.