GTTF: Support running browser tests in parallel

GTTF: Support running browser tests in parallel

This still defaults to serialized execution while I perform more testing.

BUG=236893
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224200

[sky, 2013-09-12 16:16:29]

LGTM

[Paweł Hajdan Jr., 2013-09-12 21:57:16]

Hey Scott, could you take another look?

I needed to make some changes, mostly how the temporary directory is created, to
pass the trybots.

[sky, 2013-09-12 23:32:56]

LGTM

[Paweł Hajdan Jr., 2013-09-19 20:56:57]

Committed patchset #10 manually as r224200.

[jam, 2013-09-25 20:02:02]

looks like this broke the property that if Foo.PRE_Bar fails, Foo.Bar is not
run.

I've filed crbug.com\298355. Can this be fixed soon, or should we revert this in
the meantime?

[Paweł Hajdan Jr., 2013-09-25 20:39:41]

On 2013/09/25 20:02:02, jam wrote:
> looks like this broke the property that if Foo.PRE_Bar fails, Foo.Bar is not
> run.
> 
> I've filed crbug.com\298355. Can this be fixed soon, or should we revert this
in
> the meantime?

Could you explain why it would be so bad to warrant a revert?

If PRE_ test fails, the whole test fails anyway, so it shouldn't matter whether
non-PRE part is executed or not. It shouldn't hurt to run it - and if the
argument is speed, then when tests start failing all bets are off - they could
very well be timing out, which is way slower than a failure.

I have a general idea of how I would fix this, but it'd make the code more
complex. I'd like to better understand the concern.

[jam, 2013-09-25 20:55:28]

On 2013/09/25 20:39:41, Paweł Hajdan Jr. wrote:
> On 2013/09/25 20:02:02, jam wrote:
> > looks like this broke the property that if Foo.PRE_Bar fails, Foo.Bar is not
> > run.
> > 
> > I've filed crbug.com\298355. Can this be fixed soon, or should we revert
this
> in
> > the meantime?
> 
> Could you explain why it would be so bad to warrant a revert?
> 
> If PRE_ test fails, the whole test fails anyway, so it shouldn't matter
whether
> non-PRE part is executed or not. 

It adds noise because if the Foo.Bar fails, we don't know if that failed because
PRE_Bar failed, or there's also another bug. This behavior was something that I
had specifically added when adding support for PRE_, so getting rid of it is a
regression.

> It shouldn't hurt to run it - and if the
> argument is speed, then when tests start failing all bets are off - they could
> very well be timing out, which is way slower than a failure.
> 
> I have a general idea of how I would fix this, but it'd make the code more
> complex. I'd like to better understand the concern.

I've mentioned this before, please call out any behavioral changes in CL
descriptions. If they're not simple refactorings with no behavior changes,
please also add me as a reviwer if it affects the browser test harness.

If this can be fixed within the next few days, it's fine. But if not, let's
revert. No reason getting extra noise in build logs for the failure case when
we're not even using this new code path yet.

[loislo, 2013-09-26 05:47:33]

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
content/public/test/test_launcher.cc:194: if (std::string(kPreTestPrefix) +
a.test_name == b.test_name)
This comparer doesn't work properly. It doesn't Transitive.

for the pair PRE_Indexed and Indexed it returns false. It is ok.
for the pair PRE_PRE_Indexed and PRE_Indexed it returns false. It is ok.
but for the pair PRE_PRE_Indexed and Indexed it returns true. It is wrong.

A < B and B < C but A > C

As a result sort produces wrong sequence of the tests.

[jam, 2013-09-26 21:36:03]

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
content/public/test/test_launcher.cc:194: if (std::string(kPreTestPrefix) +
a.test_name == b.test_name)
On 2013/09/26 05:47:33, loislo wrote:
> This comparer doesn't work properly. It doesn't Transitive.
> 
> for the pair PRE_Indexed and Indexed it returns false. It is ok.
> for the pair PRE_PRE_Indexed and PRE_Indexed it returns false. It is ok.
> but for the pair PRE_PRE_Indexed and Indexed it returns true. It is wrong.
> 
> A < B and B < C but A > C
> 
> As a result sort produces wrong sequence of the tests.

Pawel: ping
can this cause PRE tests to run in the wrong order?

[Paweł Hajdan Jr., 2013-09-26 23:32:27]

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/23757033/diff/37001/content/public/test/test_...
content/public/test/test_launcher.cc:194: if (std::string(kPreTestPrefix) +
a.test_name == b.test_name)
On 2013/09/26 21:36:03, jam wrote:
> On 2013/09/26 05:47:33, loislo wrote:
> > This comparer doesn't work properly. It doesn't Transitive.
> > 
> > for the pair PRE_Indexed and Indexed it returns false. It is ok.
> > for the pair PRE_PRE_Indexed and PRE_Indexed it returns false. It is ok.
> > but for the pair PRE_PRE_Indexed and Indexed it returns true. It is wrong.
> > 
> > A < B and B < C but A > C
> > 
> > As a result sort produces wrong sequence of the tests.
> 
> Pawel: ping
> can this cause PRE tests to run in the wrong order?

Given this comparator is not StrictWeakOrdering, all kinds of undefined behavior
can happen, including crashing the binary. This is a good catch, and only proves
comparators are hard.

Fix is up at https://codereview.chromium.org/24892002/ . Please review.