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;
-  }
-
   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_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 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.
-      negative_test_filter_ = SplitString(
-          filter.substr(dash_pos + 1), ":", 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_.push_back(test_name);
     }
   }
 
   if (!launcher_delegate_->GetTests(&tests_)) {
     LOG(ERROR) << "Failed to get list of tests.";
     return false;
   }
 
   if (!results_tracker_.Init(*command_line)) {
     LOG(ERROR) << "Failed to initialize test results tracker.";
@@ skipping 59 matching lines @@
 #endif
 
 #if defined(ARCH_CPU_64_BITS)
   results_tracker_.AddGlobalTag("CPU_64_BITS");
 #endif
 
   return true;
 }
 
 void TestLauncher::RunTests() {
+  // 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_));
+
   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);
+  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);
+      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);
+      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)) {
-      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;
-        }
+        // Skip disabled tests unless explicitly requested.
+        if (!command_line->HasSwitch(kGTestRunDisabledTestsFlag))
+          continue;
       }
 
-      if (!found)
+      if (!launcher_delegate_->ShouldRunTest(tests_[i].test_case_name,
+                                             tests_[i].test_name)) {
         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)
+      // 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 (excluded)
+          continue;
+      }
+
+      if (Hash(test_name) % total_shards_ !=
+          static_cast<uint32_t>(shard_index_))
         continue;
+
+      test_names.push_back(test_name);
     }
-
-    if (Hash(test_name) % total_shards_ != static_cast<uint32_t>(shard_index_))
-      continue;
-
-    test_names.push_back(test_name);
   }
 
   // Save an early test summary in case the launcher crashes or gets killed.
   MaybeSaveSummaryAsJSON({"EARLY_SUMMARY", kUnreliableResultsTag});
 
   test_started_count_ = launcher_delegate_->RunTests(this, test_names);
 
   if (test_started_count_ == 0) {
     fprintf(stdout, "0 tests run\n");
     fflush(stdout);
@@ skipping 144 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