Make browser_tests shardable.

chrome/test/out_of_proc_test_runner.cc

Index: chrome/test/out_of_proc_test_runner.cc
diff --git a/chrome/test/out_of_proc_test_runner.cc b/chrome/test/out_of_proc_test_runner.cc
index 4d70e17f2953b22a011c77843add432833d10719..aee0c6449a39aa33c5c2abf5bd02ef289e6b0a37 100644
--- a/chrome/test/out_of_proc_test_runner.cc
+++ b/chrome/test/out_of_proc_test_runner.cc
@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/environment.h"
 #include "base/file_util.h"
 #include "base/hash_tables.h"
 #include "base/linked_ptr.h"
@@ -66,6 +67,11 @@ const char kHelpFlag[]   = "help";
 
 const char kTestTerminateTimeoutFlag[] = "test-terminate-timeout";
 
+// The environment variable name for the test shard index.
+static const char kTestShardIndex[] = "GTEST_SHARD_INDEX";
+// The environment variable name for the total number of test shards.
+static const char kTestTotalShards[] = "GTEST_TOTAL_SHARDS";
+
 // How long we wait for the subprocess to exit (with a success/failure code).
 // See http://crbug.com/43862 for some discussion of the value.
 const int kDefaultTestTimeoutMs = 20000;
@@ -74,6 +80,63 @@ const int kDefaultTestTimeoutMs = 20000;
 static const FilePath::CharType kDefaultOutputFile[] = FILE_PATH_LITERAL(
     "test_detail.xml");
 
+// Parses the environment variable var as an Int32. If it is unset,
+// returns default_val. If it is not an Int32, prints an error

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

This comment is lying. We print error and abort when we can't unset the
variable, but when it's not an int the code just returns the default value.

[chase, 2011/01/14 23:39:35]

Done.

+// and aborts.
+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))
+    return default_val;
+  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 to each test

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: "assigned" -> "assigned by this launcher", to avoid confusion with gtest
which might also assign its own numbers.

[chase, 2011/01/14 23:39:35]

Done.

+// method. Assumes that 0 <= shard_index < total_shards.

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: Can we check that assumption?

[chase, 2011/01/14 23:39:35]

This assumption is verified in ShouldShard().  I updated the comment to point
that out.

+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
@@ -365,9 +428,15 @@ bool RunTests() {
   }
 
   int test_run_count = 0;
+  int num_runnable_tests = 0;
   int ignored_failure_count = 0;
   std::vector<std::string> failed_tests;
 
+  int32 total_shards;
+  int32 shard_index;
+  bool should_shard = ShouldShard(&total_shards, &shard_index);
+  bool should_run;

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: Why is |should_run| declared here, so far from the place of use?

[chase, 2011/01/14 23:39:35]

Moved below.

+
   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);
@@ -393,6 +462,17 @@ bool RunTests() {
                           false, false, false, 0);
         continue;
       }
+      // Decide if this test should be run.
+      should_run = true;

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: It should be possible to declare bool should_run here, or maybe add an
isolated function ShouldRun.

[chase, 2011/01/14 23:39:35]

Declared here.

+      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_shard && !should_run) {

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: Is should_shard needed in the condition?

If should_shard is false (and the loop body doesn't change it), should_run will
always be true.

[chase, 2011/01/14 23:39:35]

Done.

+        continue;
+      }
       base::Time start_time = base::Time::Now();
       ++test_run_count;
       int exit_code = RunTest(test_name);
@@ -420,10 +500,10 @@ bool RunTests() {
     }
   }
 
-  printf("%d test%s run\n", test_run_count, test_run_count > 1 ? "s" : "");
+  printf("%d test%s run\n", test_run_count, test_run_count == 1 ? "" : "s");

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: Why? This change seems unrelated, I'd prefer not to put more things into
the diff unless necessary (you can TBR those later).

[chase, 2011/01/14 23:39:35]

Removed.

   printf("%d test%s failed (%d ignored)\n",
          static_cast<int>(failed_tests.size()),
-         failed_tests.size() > 1 ? "s" : "", ignored_failure_count);
+         failed_tests.size() == 1 ? "" : "s", ignored_failure_count);

[Paweł Hajdan Jr., 2011/01/14 09:32:45]

nit: Why? This change seems unrelated, I'd prefer not to put more things into
the diff unless necessary (you can TBR those later).

[chase, 2011/01/14 23:39:35]

Removed.

   if (failed_tests.size() - ignored_failure_count == 0)
     return true;