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)) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

If we're extracting code, I'd suggest this flag-related code to get extracted as
well. It could be a separate method or combined with CombineTestFilters.
Suggested name could be (Get|Extract|Compute)TestFilters.

Also see if that way we can reduce number of member variables. It'd be more
maintainable to store intermediary state in local variables.

[katthomas, 2016/12/01 00:07:00]

I agree it would be better if that parsing was extracted to a helper method, but
I think that refactor should be separate from this change. 

     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));
+        negative_test_filter_file_.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) {
+    positive_test_filter_gtest_ =
+        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_test_filter_gtest_ =
+        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 test_name :
+         SplitString(filter.substr(dash_pos + 1), ":", base::TRIM_WHITESPACE,
+                     base::SPLIT_WANT_ALL)) {
+      negative_test_filter_gtest_.push_back(test_name);

[Paweł Hajdan Jr., 2016/11/30 12:41:31]

Since negative_test_filter_gtest_ doesn't collide with anything else, we should
be able to just assign result of SplitString instead of looping.

[katthomas, 2016/12/01 00:07:00]

Done.

     }
   }
 
   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 58 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::CombineTestFilters() {
+  bool has_positive_filter = !positive_test_filter_file_.empty() ||
+                             !positive_test_filter_gtest_.empty();
+  if (has_positive_filter) {
+    // If two positive filters are present, only run tests that match a pattern
+    // in both filters.
+    if (!positive_test_filter_file_.empty() &&
+        !positive_test_filter_gtest_.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_gtest = false;
+        bool found_file = false;
+        for (size_t k = 0; k < positive_test_filter_gtest_.size(); ++k) {
+          found_gtest = MatchPattern(test_name, positive_test_filter_gtest_[k]);

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

This assignment overwrites any previous result. A test will likely match one of
the patterns, but not all of them.

Shouldn't found_* be OR-ed with the previous value?

[katthomas, 2016/12/01 00:07:00]

Oof, I didn't catch this because I wasn't as thorough with my test cases
yesterday. Thanks for catching.

+        }
+        for (size_t k = 0; k < positive_test_filter_file_.size(); ++k) {
+          found_file = MatchPattern(test_name, positive_test_filter_file_[k]);
+        }
+        if (found_gtest && found_file) {
+          positive_test_filter_.push_back(test_name);
+        }
+      }
+    } else if (!positive_test_filter_file_.empty()) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

I'm not sure if this case-based code is the simplest way to express.

How about we extract a method which checks if a test matches filters, and call
that in place of code near "// Skip the test that doesn't match the filter (if
given)." comment?

[katthomas, 2016/12/01 00:07:00]

Didn't totally follow your advice here, but got the point that this has gotten a
little gross.

+      positive_test_filter_ = positive_test_filter_file_;
+    } else {
+      positive_test_filter_ = positive_test_filter_gtest_;
+    }
+  }
+  negative_test_filter_.reserve(negative_test_filter_gtest_.size() +
+                                negative_test_filter_file_.size());
+  negative_test_filter_.insert(negative_test_filter_.end(),
+                               negative_test_filter_gtest_.begin(),
+                               negative_test_filter_gtest_.end());
+  negative_test_filter_.insert(negative_test_filter_.end(),
+                               negative_test_filter_file_.begin(),
+                               negative_test_filter_file_.end());
+}
+
 void TestLauncher::RunTests() {
+  TestLauncher::CombineTestFilters();

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

nit: Is the "TestLauncher::" prefix needed here?

[katthomas, 2016/12/01 00:07:00]

Done.

+
   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 (!positive_test_filter_.empty() || has_positive_filter) {

[Paweł Hajdan Jr., 2016/11/30 12:41:32]

Why do we need a separate "has_positive_filter" variable?

[katthomas, 2016/12/01 00:07:00]

Because there is the possibility that there both flags are provided with
positive filters, and there is no intersection.

       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)
@@ skipping 174 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