GTTF: Flip test launcher defaults (developer mode is now default)

chromium-dev discussions:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ewfTWVUiYGw/uTlP2mOZlmAJ
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Mw1hrGwnUpw/pYVzC3o_SDQJ

BUG=236893
R=maruel@chromium.org, sky@chromium.org

Review URL: https://codereview.chromium.org/59623015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237448 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 74 insertions, 50 deletions

diff --git a/base/test/launcher/test_launcher.cc b/base/test/launcher/test_launcher.cc
index ada3c1f..e5387be 100644
--- a/base/test/launcher/test_launcher.cc
+++ b/base/test/launcher/test_launcher.cc
@@ -178,6 +178,16 @@ bool UnsetEnvironmentVariableIfExists(const std::string& name) {
   return env->UnSetVar(name.c_str());
 }
 
+// Returns true if bot mode has been requested, i.e. defaults optimized
+// for continuous integration bots. This way developers don't have to remember
+// special command-line flags.
+bool BotModeEnabled() {
+  scoped_ptr<Environment> env(Environment::Create());
+  return CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kTestLauncherBotMode) ||
+      env->HasVar("CHROMIUM_TEST_LAUNCHER_BOT_MODE");
+}
+
 // 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) {
@@ -231,6 +241,7 @@ void RunCallback(
 void DoLaunchChildTestProcess(
     const CommandLine& command_line,
     base::TimeDelta timeout,
+    bool redirect_stdio,
     scoped_refptr<MessageLoopProxy> message_loop_proxy,
     const TestLauncher::LaunchChildGTestProcessCallback& callback) {
   TimeTicks start_time = TimeTicks::Now();
@@ -243,8 +254,7 @@ void DoLaunchChildTestProcess(
 #if defined(OS_WIN)
   win::ScopedHandle handle;
 
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherDeveloperMode)) {
+  if (redirect_stdio) {
     // Make the file handle inheritable by the child.
     SECURITY_ATTRIBUTES sa_attr;
     sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);
@@ -270,8 +280,7 @@ void DoLaunchChildTestProcess(
   base::FileHandleMappingVector fds_mapping;
   file_util::ScopedFD output_file_fd_closer;
 
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherDeveloperMode)) {
+  if (redirect_stdio) {
     int output_file_fd = open(output_file.value().c_str(), O_RDWR);
     CHECK_GE(output_file_fd, 0);
 
@@ -287,8 +296,7 @@ void DoLaunchChildTestProcess(
   int exit_code = LaunchChildTestProcessWithOptions(
       command_line, options, timeout, &was_timeout);
 
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherDeveloperMode)) {
+  if (redirect_stdio) {
 #if defined(OS_WIN)
   FlushFileBuffers(handle.Get());
   handle.Close();
@@ -340,27 +348,32 @@ TestLauncher::TestLauncher(TestLauncherDelegate* launcher_delegate,
       test_success_count_(0),
       test_broken_count_(0),
       retry_count_(0),
-      retry_limit_(3),
+      retry_limit_(0),
       run_result_(true),
       watchdog_timer_(FROM_HERE,
                       TimeDelta::FromSeconds(kOutputTimeoutSeconds),
                       this,
-                      &TestLauncher::OnOutputTimeout) {
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kTestLauncherDeveloperMode)) {
-    parallel_jobs = 1;
-    retry_limit_ = 0;
+                      &TestLauncher::OnOutputTimeout),
+      parallel_jobs_(parallel_jobs) {
+  if (BotModeEnabled()) {
     fprintf(stdout,
-            "Forcing serial test execution in developer mode.\n"
-            "Disabling test retries in developer mode.\n");
+            "Enabling defaults optimized for continuous integration bots.\n");
     fflush(stdout);
+
+    // Enable test retries by default for bots. This can be still overridden
+    // from command line using --test-launcher-retry-limit flag.
+    retry_limit_ = 3;
+  } else {
+    // Default to serial test execution if not running on a bot. This makes it
+    // possible to disable stdio redirection and can still be overridden with
+    // --test-launcher-jobs flag.
+    parallel_jobs_ = 1;
   }
-  worker_pool_owner_.reset(
-      new SequencedWorkerPoolOwner(parallel_jobs, "test_launcher"));
 }
 
 TestLauncher::~TestLauncher() {
-  worker_pool_owner_->pool()->Shutdown();
+  if (worker_pool_owner_)
+    worker_pool_owner_->pool()->Shutdown();
 }
 
 bool TestLauncher::Run(int argc, char** argv) {
@@ -418,11 +431,17 @@ void TestLauncher::LaunchChildGTestProcess(
   CommandLine new_command_line(
       PrepareCommandLineForGTest(command_line, wrapper));
 
+  // When running in parallel mode we need to redirect stdio to avoid mixed-up
+  // output. We also always redirect on the bots to get the test output into
+  // JSON summary.
+  bool redirect_stdio = (parallel_jobs_ > 1) || BotModeEnabled();
+
   worker_pool_owner_->pool()->PostWorkerTask(
       FROM_HERE,
       Bind(&DoLaunchChildTestProcess,
            new_command_line,
            timeout,
+           redirect_stdio,
            MessageLoopProxy::current(),
            Bind(&TestLauncher::OnLaunchTestProcessFinished,
                 Unretained(this),
@@ -653,6 +672,23 @@ bool TestLauncher::Init() {
     retry_limit_ = retry_limit;
   }
 
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kTestLauncherJobs)) {
+    int jobs = -1;
+    if (!StringToInt(CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+                         switches::kTestLauncherJobs), &jobs) ||
+        jobs < 0) {
+      LOG(ERROR) << "Invalid value for " << switches::kTestLauncherJobs;
+      return false;
+    }
+
+    parallel_jobs_ = jobs;
+  }
+  fprintf(stdout, "Using %" PRIuS " parallel jobs.\n", parallel_jobs_);
+  fflush(stdout);
+  worker_pool_owner_.reset(
+      new SequencedWorkerPoolOwner(parallel_jobs_, "test_launcher"));
+
   // Split --gtest_filter at '-', if there is one, to separate into
   // positive filter and negative filter portions.
   std::string filter = command_line->GetSwitchValueASCII(kGTestFilterFlag);
diff --git a/base/test/launcher/test_launcher.h b/base/test/launcher/test_launcher.h
index 1ff1c05..3907c75 100644
--- a/base/test/launcher/test_launcher.h
+++ b/base/test/launcher/test_launcher.h
@@ -181,6 +181,9 @@ class TestLauncher {
   // Watchdog timer to make sure we do not go without output for too long.
   DelayTimer<TestLauncher> watchdog_timer_;
 
+  // Number of jobs to run in parallel.
+  size_t parallel_jobs_;
+
   // Worker pool used to launch processes in parallel.
   scoped_ptr<SequencedWorkerPoolOwner> worker_pool_owner_;
 
diff --git a/base/test/launcher/unit_test_launcher.cc b/base/test/launcher/unit_test_launcher.cc
index c5dbc4d..8a57d17 100644
--- a/base/test/launcher/unit_test_launcher.cc
+++ b/base/test/launcher/unit_test_launcher.cc
@@ -464,26 +464,21 @@ int LaunchUnitTests(int argc,
   testing::InitGoogleTest(&argc, argv);
   TestTimeouts::Initialize();
 
-  int jobs = SysInfo::NumberOfProcessors();
-  if (!GetSwitchValueAsInt(switches::kTestLauncherJobs, &jobs))
-    return 1;
-
   int batch_limit = kDefaultTestBatchLimit;
   if (!GetSwitchValueAsInt(switches::kTestLauncherBatchLimit, &batch_limit))
     return 1;
 
   fprintf(stdout,
-          "Starting tests (using %d parallel jobs)...\n"
           "IMPORTANT DEBUGGING NOTE: batches of tests are run inside their\n"
           "own process. For debugging a test inside a debugger, use the\n"
           "--gtest_filter=<your_test_name> flag along with\n"
-          "--single-process-tests.\n", jobs);
+          "--single-process-tests.\n");
   fflush(stdout);
 
   MessageLoopForIO message_loop;
 
   base::UnitTestLauncherDelegate delegate(batch_limit);
-  base::TestLauncher launcher(&delegate, jobs);
+  base::TestLauncher launcher(&delegate, SysInfo::NumberOfProcessors());
   bool success = launcher.Run(argc, argv);
 
   fprintf(stdout,
diff --git a/base/test/test_switches.cc b/base/test/test_switches.cc
index ff8f315..aaea6b4 100644
--- a/base/test/test_switches.cc
+++ b/base/test/test_switches.cc
@@ -11,10 +11,10 @@ const char switches::kTestLargeTimeout[] = "test-large-timeout";
 // Maximum number of tests to run in a single batch.
 const char switches::kTestLauncherBatchLimit[] = "test-launcher-batch-limit";
 
-// Behaves in a more friendly way to local debugging, e.g. serial execution,
-// no redirection of stdio, etc.
-const char switches::kTestLauncherDeveloperMode[] =
-    "test-launcher-developer-mode";
+// Sets defaults desirable for the continuous integration bots, e.g. parallel
+// test execution and test retries.
+const char switches::kTestLauncherBotMode[] =
+    "test-launcher-bot-mode";
 
 // Number of parallel test launcher jobs.
 const char switches::kTestLauncherJobs[] = "test-launcher-jobs";
diff --git a/base/test/test_switches.h b/base/test/test_switches.h
index b9cb0a5..fb2525a 100644
--- a/base/test/test_switches.h
+++ b/base/test/test_switches.h
@@ -11,7 +11,7 @@ namespace switches {
 // alongside the definition of their values in the .cc file.
 extern const char kTestLargeTimeout[];
 extern const char kTestLauncherBatchLimit[];
-extern const char kTestLauncherDeveloperMode[];
+extern const char kTestLauncherBotMode[];
 extern const char kTestLauncherJobs[];
 extern const char kTestLauncherOutput[];
 extern const char kTestLauncherRetryLimit[];
diff --git a/chrome/browser_tests.isolate b/chrome/browser_tests.isolate
index 8d590c6..ff06a4b 100644
--- a/chrome/browser_tests.isolate
+++ b/chrome/browser_tests.isolate
@@ -9,6 +9,7 @@
           '../testing/xvfb.py',
           '<(PRODUCT_DIR)',
           '<(PRODUCT_DIR)/browser_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '../testing/xvfb.py',
@@ -128,6 +129,7 @@
         'command': [
           '../testing/test_env.py',
           '<(PRODUCT_DIR)/browser_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
       },
     }],
diff --git a/chrome/interactive_ui_tests.isolate b/chrome/interactive_ui_tests.isolate
index ff3d1d4..357684e 100644
--- a/chrome/interactive_ui_tests.isolate
+++ b/chrome/interactive_ui_tests.isolate
@@ -9,6 +9,7 @@
           '../testing/xvfb.py',
           '<(PRODUCT_DIR)',
           '<(PRODUCT_DIR)/interactive_ui_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '../testing/xvfb.py',
@@ -66,6 +67,7 @@
         'command': [
           '../testing/test_env.py',
           '<(PRODUCT_DIR)/interactive_ui_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
       },
     }],
diff --git a/chrome/sync_integration_tests.isolate b/chrome/sync_integration_tests.isolate
index 460bde8..8419d54 100644
--- a/chrome/sync_integration_tests.isolate
+++ b/chrome/sync_integration_tests.isolate
@@ -9,6 +9,7 @@
           '../testing/xvfb.py',
           '<(PRODUCT_DIR)',
           '<(PRODUCT_DIR)/sync_integration_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '../testing/test_env.py',
@@ -52,6 +53,7 @@
       'variables': {
         'command': [
           '<(PRODUCT_DIR)/sync_integration_tests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '<(PRODUCT_DIR)/ffmpegsumo.so',
diff --git a/content/content_browsertests.isolate b/content/content_browsertests.isolate
index 4cc0900..dcc33da 100644
--- a/content/content_browsertests.isolate
+++ b/content/content_browsertests.isolate
@@ -27,6 +27,7 @@
           '../testing/xvfb.py',
           '<(PRODUCT_DIR)',
           '<(PRODUCT_DIR)/content_browsertests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '../testing/xvfb.py',
@@ -76,6 +77,7 @@
         'command': [
           '../testing/test_env.py',
           '<(PRODUCT_DIR)/content_browsertests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
       },
     }],
diff --git a/content/public/test/test_launcher.cc b/content/public/test/test_launcher.cc
index 2fdc2a2..de05538 100644
--- a/content/public/test/test_launcher.cc
+++ b/content/public/test/test_launcher.cc
@@ -399,20 +399,6 @@ void WrapperTestLauncherDelegate::GTestCallback(
   test_launcher->OnTestFinished(result);
 }
 
-bool GetSwitchValueAsInt(const std::string& switch_name, int* result) {
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switch_name))
-    return true;
-
-  std::string switch_value =
-      CommandLine::ForCurrentProcess()->GetSwitchValueASCII(switch_name);
-  if (!base::StringToInt(switch_value, result) || *result < 1) {
-    LOG(ERROR) << "Invalid value for " << switch_name << ": " << switch_value;
-    return false;
-  }
-
-  return true;
-}
-
 }  // namespace
 
 const char kHelpFlag[]   = "help";
@@ -495,23 +481,18 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
   testing::InitGoogleTest(&argc, argv);
   TestTimeouts::Initialize();
 
-  int jobs = default_jobs;
-  if (!GetSwitchValueAsInt(switches::kTestLauncherJobs, &jobs))
-    return 1;
-
   fprintf(stdout,
-      "Starting tests (using %d parallel jobs)...\n"
       "IMPORTANT DEBUGGING NOTE: each test is run inside its own process.\n"
       "For debugging a test inside a debugger, use the\n"
       "--gtest_filter=<your_test_name> flag along with either\n"
       "--single_process (to run the test in one launcher/browser process) or\n"
       "--single-process (to do the above, and also run Chrome in single-"
-          "process mode).\n", jobs);
+          "process mode).\n");
 
   base::MessageLoopForIO message_loop;
 
   WrapperTestLauncherDelegate delegate(launcher_delegate);
-  base::TestLauncher launcher(&delegate, jobs);
+  base::TestLauncher launcher(&delegate, default_jobs);
   bool success = launcher.Run(argc, argv);
   return (success ? 0 : 1);
 }
diff --git a/third_party/cacheinvalidation/cacheinvalidation_unittests.isolate b/third_party/cacheinvalidation/cacheinvalidation_unittests.isolate
index 370b783..f802a45 100644
--- a/third_party/cacheinvalidation/cacheinvalidation_unittests.isolate
+++ b/third_party/cacheinvalidation/cacheinvalidation_unittests.isolate
@@ -7,6 +7,7 @@
       'variables': {
         'command': [
           '<(PRODUCT_DIR)/cacheinvalidation_unittests<(EXECUTABLE_SUFFIX)',
+          '--test-launcher-bot-mode',
         ],
         'isolate_dependency_tracked': [
           '<(PRODUCT_DIR)/cacheinvalidation_unittests<(EXECUTABLE_SUFFIX)',