GTTF: Move core parts of test_launcher down to base.

content/public/test/test_launcher.cc

Index: content/public/test/test_launcher.cc
diff --git a/content/public/test/test_launcher.cc b/content/public/test/test_launcher.cc
index 891d62c8776cd7a38255aa10529cb94fc2f838f5..b717eb05b0cd11fb62fbf1881901b436bb91ed68 100644
--- a/content/public/test/test_launcher.cc
+++ b/content/public/test/test_launcher.cc
@@ -19,6 +19,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_launcher.h"
 #include "base/test/test_suite.h"
 #include "base/test/test_timeouts.h"
 #include "base/time.h"
@@ -58,249 +59,13 @@ const char kManualTestPrefix[] = "MANUAL_";
 TestLauncherDelegate* g_launcher_delegate;
 }
 
-// The environment variable name for the total number of test shards.
-const char kTestTotalShards[] = "GTEST_TOTAL_SHARDS";
-// The environment variable name for the test shard index.
-const char kTestShardIndex[] = "GTEST_SHARD_INDEX";
-
-// The default output file for XML output.
-const base::FilePath::CharType kDefaultOutputFile[] = FILE_PATH_LITERAL(
-    "test_detail.xml");
-
-// Quit test execution after this number of tests has timed out.
-const int kMaxTimeouts = 5;  // 45s timeout * (5 + 1) = 270s max run time.
-
 namespace {
 
-// Parses the environment variable var as an Int32.  If it is unset, returns
-// default_val.  If it is set, unsets it then converts it to Int32 before
-// returning it.  If unsetting or converting to an Int32 fails, print an
-// error and exit with failure.
-int32 Int32FromEnvOrDie(const char* const var, int32 default_val) {
-  scoped_ptr<base::Environment> env(base::Environment::Create());
-  std::string str_val;
-  int32 result;
-  if (!env->GetVar(var, &str_val))
-    return default_val;
-  if (!env->UnSetVar(var)) {
-    LOG(ERROR) << "Invalid environment: we could not unset " << var << ".\n";
-    exit(EXIT_FAILURE);
-  }
-  if (!base::StringToInt(str_val, &result)) {
-    LOG(ERROR) << "Invalid environment: " << var << " is not an integer.\n";
-    exit(EXIT_FAILURE);
-  }
-  return result;
-}
-
-// Checks whether sharding is enabled by examining the relevant
-// environment variable values.  If the variables are present,
-// but inconsistent (i.e., shard_index >= total_shards), prints
-// an error and exits.
-bool ShouldShard(int32* total_shards, int32* shard_index) {
-  *total_shards = Int32FromEnvOrDie(kTestTotalShards, -1);
-  *shard_index = Int32FromEnvOrDie(kTestShardIndex, -1);
-
-  if (*total_shards == -1 && *shard_index == -1) {
-    return false;
-  } else if (*total_shards == -1 && *shard_index != -1) {
-    LOG(ERROR) << "Invalid environment variables: you have "
-               << kTestShardIndex << " = " << *shard_index
-               << ", but have left " << kTestTotalShards << " unset.\n";
-    exit(EXIT_FAILURE);
-  } else if (*total_shards != -1 && *shard_index == -1) {
-    LOG(ERROR) << "Invalid environment variables: you have "
-               << kTestTotalShards << " = " << *total_shards
-               << ", but have left " << kTestShardIndex << " unset.\n";
-    exit(EXIT_FAILURE);
-  } else if (*shard_index < 0 || *shard_index >= *total_shards) {
-    LOG(ERROR) << "Invalid environment variables: we require 0 <= "
-               << kTestShardIndex << " < " << kTestTotalShards
-               << ", but you have " << kTestShardIndex << "=" << *shard_index
-               << ", " << kTestTotalShards << "=" << *total_shards << ".\n";
-    exit(EXIT_FAILURE);
-  }
-
-  return *total_shards > 1;
-}
-
-// Given the total number of shards, the shard index, and the test id, returns
-// true iff the test should be run on this shard.  The test id is some arbitrary
-// but unique non-negative integer assigned by this launcher to each test
-// method.  Assumes that 0 <= shard_index < total_shards, which is first
-// verified in ShouldShard().
-bool ShouldRunTestOnShard(int total_shards, int shard_index, int test_id) {
-  return (test_id % total_shards) == shard_index;
-}
-
-// A helper class to output results.
-// Note: as currently XML is the only supported format by gtest, we don't
-// check output format (e.g. "xml:" prefix) here and output an XML file
-// unconditionally.
-// Note: we don't output per-test-case or total summary info like
-// total failed_test_count, disabled_test_count, elapsed_time and so on.
-// Only each test (testcase element in the XML) will have the correct
-// failed/disabled/elapsed_time information. Each test won't include
-// detailed failure messages either.
-class ResultsPrinter {
- public:
-  explicit ResultsPrinter(const CommandLine& command_line);
-  ~ResultsPrinter();
-  void OnTestCaseStart(const char* name, int test_count) const;
-  void OnTestCaseEnd() const;
-
-  void OnTestEnd(const char* name, const char* case_name, bool run,
-                 bool failed, bool failure_ignored, double elapsed_time) const;
- private:
-  FILE* out_;
-
-  DISALLOW_COPY_AND_ASSIGN(ResultsPrinter);
-};
-
-ResultsPrinter::ResultsPrinter(const CommandLine& command_line) : out_(NULL) {
-  if (!command_line.HasSwitch(kGTestOutputFlag))
-    return;
-  std::string flag = command_line.GetSwitchValueASCII(kGTestOutputFlag);
-  size_t colon_pos = flag.find(':');
-  base::FilePath path;
-  if (colon_pos != std::string::npos) {
-    base::FilePath flag_path =
-        command_line.GetSwitchValuePath(kGTestOutputFlag);
-    base::FilePath::StringType path_string = flag_path.value();
-    path = base::FilePath(path_string.substr(colon_pos + 1));
-    // If the given path ends with '/', consider it is a directory.
-    // Note: This does NOT check that a directory (or file) actually exists
-    // (the behavior is same as what gtest does).
-    if (path.EndsWithSeparator()) {
-      base::FilePath executable = command_line.GetProgram().BaseName();
-      path = path.Append(executable.ReplaceExtension(
-          base::FilePath::StringType(FILE_PATH_LITERAL("xml"))));
-    }
-  }
-  if (path.value().empty())
-    path = base::FilePath(kDefaultOutputFile);
-  base::FilePath dir_name = path.DirName();
-  if (!file_util::DirectoryExists(dir_name)) {
-    LOG(WARNING) << "The output directory does not exist. "
-                 << "Creating the directory: " << dir_name.value();
-    // Create the directory if necessary (because the gtest does the same).
-    file_util::CreateDirectory(dir_name);
-  }
-  out_ = file_util::OpenFile(path, "w");
-  if (!out_) {
-    LOG(ERROR) << "Cannot open output file: "
-               << path.value() << ".";
-    return;
-  }
-  fprintf(out_, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n");
-  fprintf(out_, "<testsuites name=\"AllTests\" tests=\"\" failures=\"\""
-          " disabled=\"\" errors=\"\" time=\"\">\n");
-}
-
-ResultsPrinter::~ResultsPrinter() {
-  if (!out_)
-    return;
-  fprintf(out_, "</testsuites>\n");
-  fclose(out_);
-}
-
-void ResultsPrinter::OnTestCaseStart(const char* name, int test_count) const {
-  if (!out_)
-    return;
-  fprintf(out_, "  <testsuite name=\"%s\" tests=\"%d\" failures=\"\""
-          " disabled=\"\" errors=\"\" time=\"\">\n", name, test_count);
-}
-
-void ResultsPrinter::OnTestCaseEnd() const {
-  if (!out_)
-    return;
-  fprintf(out_, "  </testsuite>\n");
-}
-
-void ResultsPrinter::OnTestEnd(const char* name,
-                               const char* case_name,
-                               bool run,
-                               bool failed,
-                               bool failure_ignored,
-                               double elapsed_time) const {
-  if (!out_)
-    return;
-  fprintf(out_, "    <testcase name=\"%s\" status=\"%s\" time=\"%.3f\""
-          " classname=\"%s\"",
-          name, run ? "run" : "notrun", elapsed_time / 1000.0, case_name);
-  if (!failed) {
-    fprintf(out_, " />\n");
-    return;
-  }
-  fprintf(out_, ">\n");
-  fprintf(out_, "      <failure message=\"\" type=\"\"%s></failure>\n",
-          failure_ignored ? " ignored=\"true\"" : "");
-  fprintf(out_, "    </testcase>\n");
-}
-
-class TestCasePrinterHelper {
- public:
-  TestCasePrinterHelper(const ResultsPrinter& printer,
-                        const char* name,
-                        int total_test_count)
-      : printer_(printer) {
-    printer_.OnTestCaseStart(name, total_test_count);
-  }
-  ~TestCasePrinterHelper() {
-    printer_.OnTestCaseEnd();
-  }
- private:
-  const ResultsPrinter& printer_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestCasePrinterHelper);
-};
-
-// For a basic pattern matching for gtest_filter options.  (Copied from
-// gtest.cc, see the comment below and http://crbug.com/44497)
-bool PatternMatchesString(const char* pattern, const char* str) {
-  switch (*pattern) {
-    case '\0':
-    case ':':  // Either ':' or '\0' marks the end of the pattern.
-      return *str == '\0';
-    case '?':  // Matches any single character.
-      return *str != '\0' && PatternMatchesString(pattern + 1, str + 1);
-    case '*':  // Matches any string (possibly empty) of characters.
-      return (*str != '\0' && PatternMatchesString(pattern, str + 1)) ||
-          PatternMatchesString(pattern + 1, str);
-    default:  // Non-special character.  Matches itself.
-      return *pattern == *str &&
-          PatternMatchesString(pattern + 1, str + 1);
-  }
-}
-
-// TODO(phajdan.jr): Avoid duplicating gtest code. (http://crbug.com/44497)
-// For basic pattern matching for gtest_filter options.  (Copied from
-// gtest.cc)
-bool MatchesFilter(const std::string& name, const std::string& filter) {
-  const char *cur_pattern = filter.c_str();
-  for (;;) {
-    if (PatternMatchesString(cur_pattern, name.c_str())) {
-      return true;
-    }
-
-    // Finds the next pattern in the filter.
-    cur_pattern = strchr(cur_pattern, ':');
-
-    // Returns if no more pattern can be found.
-    if (cur_pattern == NULL) {
-      return false;
-    }
-
-    // Skips the pattern separater (the ':' character).
-    cur_pattern++;
-  }
-}
-
-int RunTestInternal(const testing::TestCase* test_case,
-                    const std::string& test_name,
-                    CommandLine* command_line,
-                    base::TimeDelta default_timeout,
-                    bool* was_timeout) {
+int DoRunTestInternal(const testing::TestCase* test_case,
+                      const std::string& test_name,
+                      CommandLine* command_line,
+                      base::TimeDelta default_timeout,
+                      bool* was_timeout) {
   if (test_case) {
     std::string pre_test_name = test_name;
     std::string replace_string = std::string(".") + kPreTestPrefix;
@@ -311,8 +76,11 @@ int RunTestInternal(const testing::TestCase* test_case,
       cur_test_name.append(".");
       cur_test_name.append(test_info->name());
       if (cur_test_name == pre_test_name) {
-        int exit_code = RunTestInternal(test_case, pre_test_name, command_line,
-                                        default_timeout, was_timeout);
+        int exit_code = DoRunTestInternal(test_case,
+                                          pre_test_name,
+                                          command_line,
+                                          default_timeout,
+                                          was_timeout);
         if (exit_code != 0)
           return exit_code;
       }
@@ -384,11 +152,11 @@ int RunTestInternal(const testing::TestCase* test_case,
 
 // Runs test specified by |test_name| in a child process,
 // and returns the exit code.
-int RunTest(TestLauncherDelegate* launcher_delegate,
-            const testing::TestCase* test_case,
-            const std::string& test_name,
-            base::TimeDelta default_timeout,
-            bool* was_timeout) {
+int DoRunTest(TestLauncherDelegate* launcher_delegate,
+              const testing::TestCase* test_case,
+              const std::string& test_name,
+              base::TimeDelta default_timeout,
+              bool* was_timeout) {
   if (was_timeout)
     *was_timeout = false;
 
@@ -429,149 +197,10 @@ int RunTest(TestLauncherDelegate* launcher_delegate,
     return -1;
   }
 
-  return RunTestInternal(
+  return DoRunTestInternal(
       test_case, test_name, &new_cmd_line, default_timeout, was_timeout);
 }
 
-bool RunTests(TestLauncherDelegate* launcher_delegate,
-              bool should_shard,
-              int total_shards,
-              int shard_index) {
-  const CommandLine* command_line = CommandLine::ForCurrentProcess();
-
-  DCHECK(!command_line->HasSwitch(kGTestListTestsFlag));
-
-  testing::UnitTest* const unit_test = testing::UnitTest::GetInstance();
-
-  std::string filter = command_line->GetSwitchValueASCII(kGTestFilterFlag);
-
-  // Split --gtest_filter at '-', if there is one, to separate into
-  // positive filter and negative filter portions.
-  std::string positive_filter = filter;
-  std::string negative_filter;
-  size_t dash_pos = filter.find('-');
-  if (dash_pos != std::string::npos) {
-    positive_filter = filter.substr(0, dash_pos);  // Everything up to the dash.
-    negative_filter = filter.substr(dash_pos + 1); // Everything after the dash.
-  }
-
-  int num_runnable_tests = 0;
-  int test_run_count = 0;
-  int timeout_count = 0;
-  std::vector<std::string> failed_tests;
-  std::set<std::string> ignored_tests;
-
-  ResultsPrinter printer(*command_line);
-  for (int i = 0; i < unit_test->total_test_case_count(); ++i) {
-    const testing::TestCase* test_case = unit_test->GetTestCase(i);
-    TestCasePrinterHelper helper(printer, test_case->name(),
-                                 test_case->total_test_count());
-    for (int j = 0; j < test_case->total_test_count(); ++j) {
-      const testing::TestInfo* test_info = test_case->GetTestInfo(j);
-      std::string test_name = test_info->test_case_name();
-      test_name.append(".");
-      test_name.append(test_info->name());
-
-      // Skip our special test so it's not run twice. That confuses the log
-      // parser.
-      if (test_name == launcher_delegate->GetEmptyTestName())
-        continue;
-
-      // Skip disabled tests.
-      if (test_name.find("DISABLED") != std::string::npos &&
-          !command_line->HasSwitch(kGTestRunDisabledTestsFlag)) {
-        printer.OnTestEnd(test_info->name(), test_case->name(),
-                          false, false, false, 0);
-        continue;
-      }
-
-      if (StartsWithASCII(test_info->name(), kPreTestPrefix, true))
-        continue;
-
-      if (StartsWithASCII(test_info->name(), kManualTestPrefix, true) &&
-          !command_line->HasSwitch(kRunManualTestsFlag)) {
-        continue;
-      }
-
-      // Skip the test that doesn't match the filter string (if given).
-      if ((!positive_filter.empty() &&
-           !MatchesFilter(test_name, positive_filter)) ||
-          MatchesFilter(test_name, negative_filter)) {
-        printer.OnTestEnd(test_info->name(), test_case->name(),
-                          false, false, false, 0);
-        continue;
-      }
-
-      // Decide if this test should be run.
-      bool should_run = true;
-      if (should_shard) {
-        should_run = ShouldRunTestOnShard(total_shards, shard_index,
-                                          num_runnable_tests);
-      }
-      num_runnable_tests += 1;
-      // If sharding is enabled and the test should not be run, skip it.
-      if (!should_run) {
-        continue;
-      }
-
-      base::TimeTicks start_time = base::TimeTicks::Now();
-      ++test_run_count;
-      bool was_timeout = false;
-      int exit_code = RunTest(launcher_delegate,
-                              test_case,
-                              test_name,
-                              TestTimeouts::action_max_timeout(),
-                              &was_timeout);
-      if (exit_code == 0) {
-        // Test passed.
-        printer.OnTestEnd(
-            test_info->name(), test_case->name(), true, false,
-            false,
-            (base::TimeTicks::Now() - start_time).InMillisecondsF());
-      } else {
-        failed_tests.push_back(test_name);
-
-        bool ignore_failure = false;
-        printer.OnTestEnd(
-            test_info->name(), test_case->name(), true, true,
-            ignore_failure,
-            (base::TimeTicks::Now() - start_time).InMillisecondsF());
-        if (ignore_failure)
-          ignored_tests.insert(test_name);
-
-        if (was_timeout)
-          ++timeout_count;
-      }
-
-      if (timeout_count > kMaxTimeouts) {
-        printf("More than %d timeouts, aborting test case\n", kMaxTimeouts);
-        break;
-      }
-    }
-    if (timeout_count > kMaxTimeouts) {
-      printf("More than %d timeouts, aborting test\n", kMaxTimeouts);
-      break;
-    }
-  }
-
-  printf("%d test%s run\n", test_run_count, test_run_count > 1 ? "s" : "");
-  printf("%d test%s failed (%d ignored)\n",
-         static_cast<int>(failed_tests.size()),
-         failed_tests.size() != 1 ? "s" : "",
-         static_cast<int>(ignored_tests.size()));
-  if (failed_tests.size() == ignored_tests.size())
-    return true;
-
-  printf("Failing tests:\n");
-  for (std::vector<std::string>::const_iterator iter = failed_tests.begin();
-       iter != failed_tests.end(); ++iter) {
-    bool is_ignored = ignored_tests.find(*iter) != ignored_tests.end();
-    printf("%s%s\n", iter->c_str(), is_ignored ? " (ignored)" : "");
-  }
-
-  return false;
-}
-
 void PrintUsage() {
   fprintf(stdout,
       "Runs tests using the gtest framework, each test being run in its own\n"
@@ -587,6 +216,57 @@ void PrintUsage() {
       "    Shows the gtest help message.\n");
 }
 
+class WrapperTestLauncherDelegate : public base::TestLauncherDelegate {

[jam, 2013/06/13 20:43:31]

need documentation

[Paweł Hajdan Jr., 2013/06/13 22:13:06]

Done.

+ public:
+  WrapperTestLauncherDelegate(content::TestLauncherDelegate* launcher_delegate)

[jam, 2013/06/13 20:43:31]

nit: here and below, no "content::"

explicit

[Paweł Hajdan Jr., 2013/06/13 22:13:06]

Here is the error I get when I do that:

../../content/public/test/test_launcher.cc: In member function ‘virtual bool
content::{anonymous}::WrapperTestLauncherDelegate::ShouldRunTest(const
testing::TestCase*, const testing::TestInfo*)’:
../../content/public/test/test_launcher.cc:246:40: error: ‘class
base::TestLauncherDelegate’ has no member named ‘GetEmptyTestName’
../../content/public/test/test_launcher.cc: In member function ‘virtual bool
content::{anonymous}::WrapperTestLauncherDelegate::RunTest(const
testing::TestCase*, const testing::TestInfo*)’:
../../content/public/test/test_launcher.cc:268:33: error: cannot convert
‘base::TestLauncherDelegate*’ to ‘content::TestLauncherDelegate*’ for argument
‘1’ to ‘int content::{anonymous}::DoRunTest(content::TestLauncherDelegate*,
const testing::TestCase*, const string&, base::TimeDelta, bool*)’
../../content/public/test/test_launcher.cc: In function ‘int
content::LaunchTests(content::TestLauncherDelegate*, int, char**)’:
../../content/public/test/test_launcher.cc:393:57: error: no matching function
for call to
‘content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(content::TestLauncherDelegate*&)’
../../content/public/test/test_launcher.cc:393:57: note: candidates are:
../../content/public/test/test_launcher.cc:236:3: note:
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(const
content::{anonymous}::WrapperTestLauncherDelegate&)
../../content/public/test/test_launcher.cc:236:3: note:   no known conversion
for argument 1 from ‘content::TestLauncherDelegate*’ to ‘const
content::{anonymous}::WrapperTestLauncherDelegate&’
../../content/public/test/test_launcher.cc:223:3: note:
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(base::TestLauncherDelegate*)
../../content/public/test/test_launcher.cc:223:3: note:   no known conversion
for argument 1 from ‘content::TestLauncherDelegate*’ to
‘base::TestLauncherDelegate*’

Given I'm no C++ expert, there could be something I don't see that would fix it
while not saying content:: explicitly.

Having content::TestLauncherDelegate and base::TestLauncherDelegate certainly
has potential to confuse things. I could rename them, e.g.
BaseTestLauncherDelegate or ContentTestLauncherDelegate, or something else.
Done.

[jam, 2013/06/14 16:16:50]

ah I see, it's because it derives from a parent class that's also called that.
usually you don't need to specify the namespace of a parent class when referring
to it.

ignore my comment then. I think it would be redundant to have base::Base and
content::Content

+      : launcher_delegate_(launcher_delegate) {
+  }
+
+  // base::TestLauncherDelegate:
+  virtual bool ShouldRunTest(const testing::TestCase* test_case,
+                             const testing::TestInfo* test_info) OVERRIDE;
+  virtual bool RunTest(const testing::TestCase* test_case,
+                       const testing::TestInfo* test_info) OVERRIDE;
+
+ private:
+  content::TestLauncherDelegate* launcher_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(WrapperTestLauncherDelegate);
+};
+
+bool WrapperTestLauncherDelegate::ShouldRunTest(
+    const testing::TestCase* test_case,
+    const testing::TestInfo* test_info) {
+  // Skip our special test so it's not run twice. That confuses the log
+  // parser.
+  std::string test_name =
+      std::string(test_case->name()) + "." + test_info->name();
+  if (test_name == launcher_delegate_->GetEmptyTestName())
+    return false;
+
+  if (StartsWithASCII(test_info->name(), kPreTestPrefix, true))
+    return false;
+
+  if (StartsWithASCII(test_info->name(), kManualTestPrefix, true) &&
+      !CommandLine::ForCurrentProcess()->HasSwitch(kRunManualTestsFlag)) {
+    return false;
+  }
+
+  return true;
+}
+
+bool WrapperTestLauncherDelegate::RunTest(const testing::TestCase* test_case,
+                                          const testing::TestInfo* test_info) {
+  std::string test_name =
+      std::string(test_case->name()) + "." + test_info->name();
+  int exit_code = DoRunTest(launcher_delegate_,
+                            test_case,
+                            test_name,
+                            TestTimeouts::action_max_timeout(),
+                            NULL);
+  return exit_code == 0;
+}
+
 }  // namespace
 
 // The following is kept for historical reasons (so people that are used to
@@ -609,8 +289,6 @@ const char kRunManualTestsFlag[] = "run-manual";
 
 const char kSingleProcessTestsFlag[]   = "single_process";
 
-const char kWarmupFlag[] = "warmup";
-
 
 TestLauncherDelegate::~TestLauncherDelegate() {
 }
@@ -677,12 +355,6 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
   if (ShouldRunContentMain())
     return RunContentMain(argc, argv, launcher_delegate);
 
-  base::AtExitManager at_exit;
-
-  int32 total_shards;
-  int32 shard_index;
-  bool should_shard = ShouldShard(&total_shards, &shard_index);
-
   fprintf(stdout,
       "Starting tests...\n"
       "IMPORTANT DEBUGGING NOTE: each test is run inside its own process.\n"
@@ -692,9 +364,9 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
       "--single-process (to do the above, and also run Chrome in single-"
       "process mode).\n");
 
+  base::AtExitManager at_exit;
   testing::InitGoogleTest(&argc, argv);
   TestTimeouts::Initialize();
-  int exit_code = 0;
 
   std::string empty_test = launcher_delegate->GetEmptyTestName();
   if (!empty_test.empty()) {
@@ -703,41 +375,21 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
     // timeout causing flaky test failures. Use an empty test that only starts
     // and closes a browser with a long timeout to avoid those problems.
     // NOTE: We don't do this when specifying a filter because this slows down
-    // the common case of running one test locally, and also on trybots when
-    // sharding as this one test runs ~200 times and wastes a few minutes.
-    bool warmup = command_line->HasSwitch(kWarmupFlag);

[jam, 2013/06/13 20:43:31]

where did the warmup/empty test code go?

please enumerate any other changes to the moved code

[Paweł Hajdan Jr., 2013/06/13 22:13:06]

Empty test code is still in the same place - I've used cs.chromium.org to
determine that --warmup flag is not used.
One change to note is the new version is not counting timeouts. I can restore it
if you have a strong opinion about it. I was opposing the introduction of
counting timeouts in its current form, and I think removing it is the right
thing (my future changes in this area will address this better).

There is also a slight change in behavior of the empty test handling
(ShouldShard is not checked), done for simplicity of the interface. If you have
a strong opinion, I can add it back (headers in base/test would change) - I
think it's the right thing to do though.

[jam, 2013/06/14 16:16:50]

--warmup is in sharding_supervisor_old.py. afaik we don't use that file anymore
because we use run_test_cases.py.

That means that we don't do the warmup path anymore? i.e. because on the bots
that we added this to, they all do sharding. So that second code path to run the
empty test isn't run.

I wonder if we suddenly don't need the empty test case optimization anymore?
what changed?
You should separate refactoring from behavior change. Not counting timeout is
not a trivial thing, and in the future, please call this out in the cl
description as it's hard to see this when reviewing large chunks of moved code.
Until there's another way of addressing this issue, the timeout counting code
seems needed. Otherwise someone who sends a change which causes chrome to hang
will take a long time to run on all try bots.
From the comment of the code that you deleted:
"and also on trybots when sharding as this one test runs ~200 times and wastes a
few minutes."

Making test runs a few minutes slower is a regression.

+    // the common case of running one test locally.
     bool has_filter = command_line->HasSwitch(kGTestFilterFlag);
-    if (warmup || (!should_shard && !has_filter)) {
-      exit_code = RunTest(launcher_delegate,
-                          NULL,
-                          empty_test,
-                          TestTimeouts::large_test_timeout(),
-                          NULL);
-      if (exit_code != 0 || warmup)
+    if (!has_filter) {
+      int exit_code = DoRunTest(launcher_delegate,
+                                NULL,
+                                empty_test,
+                                TestTimeouts::large_test_timeout(),
+                                NULL);
+      if (exit_code != 0)
         return exit_code;
     }
   }
 
-  int cycles = 1;
-  if (command_line->HasSwitch(kGTestRepeatFlag)) {
-    base::StringToInt(command_line->GetSwitchValueASCII(kGTestRepeatFlag),
-                      &cycles);
-  }
-
-  while (cycles != 0) {
-    if (!RunTests(launcher_delegate,
-                  should_shard,
-                  total_shards,
-                  shard_index)) {
-      exit_code = 1;
-      break;
-    }
-
-    // Special value "-1" means "repeat indefinitely".
-    if (cycles != -1)
-      cycles--;
-  }
-  return exit_code;
+  WrapperTestLauncherDelegate delegate(launcher_delegate);
+  return base::LaunchTests(&delegate, argc, argv);
 }
 
 TestLauncherDelegate* GetCurrentTestLauncherDelegate() {