Make browser_tests shardable.

Make browser_tests shardable.

Add methods to browser_tests which allow sharding
control to be used to select the tests that are
called by RunTest().  The same basic process is
used by GTest to decide if a given test should be
run (assuming the suite should be sharded).

Reuse GTEST_TOTAL_SHARDS and GTEST_SHARD_INDEX since
browser_tests is a wrapper around GTest and using the
same variable names will simplify our tools.

BUG=69423
TEST=browser_tests runs fine unsharded, can be run
sharded

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71530

[chase, 2011-01-14 01:55:30]

Making browser_tests shardable makes it possible to run browser_tests across
more than one of our test systems.  Doing so, I expect to reduce XP Tests
(dbg)(4)'s test cycle from 50 mins to 30 mins and Vista Tests (dbg)(4)'s test
cycle from 25 mins to 15 mins.

[Paweł Hajdan Jr., 2011-01-14 09:32:42]

Some minor comments, LGTM after fixing. Reducing bot cycle time sounds great to
me, I appreciate your work on this.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:84: // returns default_val. If it is not
an Int32, prints an error
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.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:134: // some arbitrary but unique
non-negative integer assigned to each test
nit: "assigned" -> "assigned by this launcher", to avoid confusion with gtest
which might also assign its own numbers.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:135: // method. Assumes that 0 <=
shard_index < total_shards.
nit: Can we check that assumption?

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:438: bool should_run;
nit: Why is |should_run| declared here, so far from the place of use?

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:466: should_run = true;
nit: It should be possible to declare bool should_run here, or maybe add an
isolated function ShouldRun.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:473: if (should_shard && !should_run) {
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.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:503: printf("%d test%s run\n",
test_run_count, test_run_count == 1 ? "" : "s");
nit: Why? This change seems unrelated, I'd prefer not to put more things into
the diff unless necessary (you can TBR those later).

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:506: failed_tests.size() == 1 ? "" : "s",
ignored_failure_count);
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]

Thanks Paweł.  Addressed all comments.  Will land when this passes compile on
the trybots.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:84: // returns default_val. If it is not
an Int32, prints an error
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> 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.

Done.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:134: // some arbitrary but unique
non-negative integer assigned to each test
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: "assigned" -> "assigned by this launcher", to avoid confusion with gtest
> which might also assign its own numbers.

Done.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:135: // method. Assumes that 0 <=
shard_index < total_shards.
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: Can we check that assumption?

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

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:438: bool should_run;
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: Why is |should_run| declared here, so far from the place of use?

Moved below.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:466: should_run = true;
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: It should be possible to declare bool should_run here, or maybe add an
> isolated function ShouldRun.

Declared here.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:473: if (should_shard && !should_run) {
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> 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.

Done.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:503: printf("%d test%s run\n",
test_run_count, test_run_count == 1 ? "" : "s");
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: Why? This change seems unrelated, I'd prefer not to put more things into
> the diff unless necessary (you can TBR those later).

Removed.

http://codereview.chromium.org/6333004/diff/3001/chrome/test/out_of_proc_test...
chrome/test/out_of_proc_test_runner.cc:506: failed_tests.size() == 1 ? "" : "s",
ignored_failure_count);
On 2011/01/14 09:32:45, Paweł Hajdan Jr. wrote:
> nit: Why? This change seems unrelated, I'd prefer not to put more things into
> the diff unless necessary (you can TBR those later).

Removed.