Implement unit test specific test launcher timeout

Committed: https://crrev.com/e8e762a3d4f51fc5d765cf23dd8d3f7d138d779c
Cr-Commit-Position: refs/heads/master@{#291673}

[jam, 2014-08-19 19:56:42]

lgtm, thanks

https://codereview.chromium.org/476543004/diff/1/base/test/test_timeouts.h
File base/test/test_timeouts.h (right):

https://codereview.chromium.org/476543004/diff/1/base/test/test_timeouts.h#ne...
base/test/test_timeouts.h:47: // Timeout for a single unit test launched used
built-in test launcher.
s/used/using

[Paweł Hajdan Jr., 2014-08-21 10:16:36]

+dalecurtis for media/ OWNERS

John, could you take another look? I needed to increase timeouts a bit to make
tryjobs pass. I added a TODO to further decrease them, and they're lower anyway
than for browser tests.

[jam, 2014-08-21 17:25:43]

On 2014/08/21 10:16:36, Paweł Hajdan Jr. wrote:
> +dalecurtis for media/ OWNERS
> 
> John, could you take another look? I needed to increase timeouts a bit to make
> tryjobs pass. I added a TODO to further decrease them, and they're lower
anyway
> than for browser tests.

is the only thing that's slow media_perftests and/or media_unittests? If so we
should use a separate timeout for them but keep everything else small

[DaleCurtis, 2014-08-21 18:12:06]

Hmm, I'm surprised to hear these benchmarks taking 45+ seconds.  rileya@ I see
you're assigned http://crbug.com/399002 -- How long do these tests take to run
locally?  Is it < 5 seconds and we're seeing a > 9x increase in cost on release
bots?

[rileya (GONE FROM CHROMIUM), 2014-08-21 20:07:18]

On 2014/08/21 18:12:06, DaleCurtis wrote:
> Hmm, I'm surprised to hear these benchmarks taking 45+ seconds.  rileya@ I see
> you're assigned http://crbug.com/399002 -- How long do these tests take to run
> locally?  Is it < 5 seconds and we're seeing a > 9x increase in cost on
release
> bots?

Looks like ~20s on a Linux z620, so a Windows bot taking ~45s+ doesn't seem too
unrealistic...

https://codereview.chromium.org/495193002 reduces the test iterations, bringing
it well under 5s on my machine.

[Paweł Hajdan Jr., 2014-08-22 11:40:17]

On 2014/08/21 17:25:43, jam wrote:
> is the only thing that's slow media_perftests and/or media_unittests? If so we
> should use a separate timeout for them but keep everything else small

No, please see tryjob results for Patch Set 4. I think it's better to land this
change for now (which lowers the timeouts anyway) and then fix tests (there is a
TODO). This will prevent further regressions meanwhile.

Dale, could you review the change to media/ ? IMHO it'd still be beneficial to
split the perf test. Thanks for making it faster as well.

[jam, 2014-08-22 17:09:19]

On 2014/08/22 11:40:17, Paweł Hajdan Jr. wrote:
> On 2014/08/21 17:25:43, jam wrote:
> > is the only thing that's slow media_perftests and/or media_unittests? If so
we
> > should use a separate timeout for them but keep everything else small
> 
> No, please see tryjob results for Patch Set 4. I think it's better to land
this
> change for now (which lowers the timeouts anyway) and then fix tests (there is
a
> TODO). This will prevent further regressions meanwhile.
> 
> Dale, could you review the change to media/ ? IMHO it'd still be beneficial to
> split the perf test. Thanks for making it faster as well.

Let's disable
FakeVideoCaptureDeviceTest.CaptureVariableResolution
and
SignalSenderVerificationTest.TestSignalRejected on CrOS
and
ProcessSingletonPosixTest.NotifyOtherProcessDifferingHost
ProcessSingletonPosixTest.NotifyOtherProcessFailure
ProcessSingletonPosixTest.NotifyOtherProcessOrCreate_DifferingHost on Linux/Cros
and
BootstrapSandboxTest.DistributedNotifications_SandboxDeny on Mac

That's only 6 tests out of > 10K tests.

[DaleCurtis, 2014-08-22 18:22:11]

lgtm

https://codereview.chromium.org/476543004/diff/80001/media/base/demuxer_perft...
File media/base/demuxer_perftest.cc (right):

https://codereview.chromium.org/476543004/diff/80001/media/base/demuxer_perft...
media/base/demuxer_perftest.cc:235: INSTANTIATE_TEST_CASE_P(, DemuxerPerfTest,
Hmm, is this valid syntax? Never seen the no first arg variant before.  Do we
want to run this on windows now that it's split up?

[jam, 2014-08-22 19:31:39]

On 2014/08/22 17:09:19, jam wrote:
> On 2014/08/22 11:40:17, Paweł Hajdan Jr. wrote:
> > On 2014/08/21 17:25:43, jam wrote:
> > > is the only thing that's slow media_perftests and/or media_unittests? If
so
> we
> > > should use a separate timeout for them but keep everything else small
> > 
> > No, please see tryjob results for Patch Set 4. I think it's better to land
> this
> > change for now (which lowers the timeouts anyway) and then fix tests (there
is
> a
> > TODO). This will prevent further regressions meanwhile.

going from 45s to 15s for a unit test isn't that big of an improvement. even 5s
seems too long. so that's why i'd rather we disable any really slow ones and
start at 5.
> > 
> > Dale, could you review the change to media/ ? IMHO it'd still be beneficial
to
> > split the perf test. Thanks for making it faster as well.
> 
> Let's disable
> FakeVideoCaptureDeviceTest.CaptureVariableResolution
> and
> SignalSenderVerificationTest.TestSignalRejected on CrOS
> and
> ProcessSingletonPosixTest.NotifyOtherProcessDifferingHost
> ProcessSingletonPosixTest.NotifyOtherProcessFailure
> ProcessSingletonPosixTest.NotifyOtherProcessOrCreate_DifferingHost on
Linux/Cros
> and
> BootstrapSandboxTest.DistributedNotifications_SandboxDeny on Mac
> 
> That's only 6 tests out of > 10K tests.

[Paweł Hajdan Jr., 2014-08-25 12:40:33]

https://codereview.chromium.org/476543004/diff/80001/media/base/demuxer_perft...
File media/base/demuxer_perftest.cc (right):

https://codereview.chromium.org/476543004/diff/80001/media/base/demuxer_perft...
media/base/demuxer_perftest.cc:235: INSTANTIATE_TEST_CASE_P(, DemuxerPerfTest,
On 2014/08/22 18:22:10, DaleCurtis wrote:
> Hmm, is this valid syntax? Never seen the no first arg variant before.

It is valid, used e.g. by FTP tests. And it passed the trybots.

> Do we want to run this on windows now that it's split up?

Can be done in separate patch, I'd prefer that to owner of the original bug.

[Paweł Hajdan Jr., 2014-08-25 15:10:52]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/504813003/ by phajdan.jr@chromium.org.

The reason for reverting is: Multiple tests are timing out on dbg bots..

[Finnur, 2014-08-25 16:41:01]

Paweł: I would kindly request that the next time you lower the timeout you
notify the sheriffs in advance. This kind of change has a high chance of causing
a large number of failures throughout the test infrastructure for possibly a
lengthy amount of time and without advance notice you send us (sheriffs) first
on a wild-goose chase (wait, why the heck are all these tests timing out???) and
then for a game of whack-a-mole as different tests need to be disabled on each
subsequent run.

Also, as Paul Jensen pointed out, it would be good to have a clear cutoff point
defined before checking in, meaning: "we can tolerate x many tests failing
before we consider the CL not ready to be introduced".

And, in an effort to make sure the sheriffs can focus on actual problems I'd
also like to see the person lowering the timeout responsible for monitoring the
tree and disabling the tests that fail due to a timeout (and promptly
communicate ownership of the failure(s) to the sheriff so there's no duplicate
work). 

As for the initiative itself (lowering timeouts), I do wonder if we are better
served by *not* restricting the timeout on the bots, but instead on the
developers (as in have artificially low timeouts, but raise them just for the
bots). I think that would force the developers to write faster tests while at
the same time not punish the sheriffs (who need to track down the failure) nor
our fellow coders (who might have to live with a closed tree while the sheriffs
investigate).

Just something to consider.

[commit-bot: I haz the power, 2014-09-10 02:34:14]

Patchset 6 (id:??) landed as
https://crrev.com/e8e762a3d4f51fc5d765cf23dd8d3f7d138d779c
Cr-Commit-Position: refs/heads/master@{#291673}