Fix ShutdownTest flakiness by opening tabs one at a time.

Fix ShutdownTest flakiness by opening tabs one at a time.

BUG=46609
TEST=startup_tests should no longer flake out as much

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

[Patrick Horn, 2010-06-17 02:00:22]

This should fix flakiness on the shutdown tests by opening tabs one at a time,
instead of starting up with 20 tabs all contending for resources, which would
randomly time out.

This is an addendum to cl 2850006 which had marked all the failing tests as
FLAKY_

I'm leaving the TwentyTabsSessionEnding marked as flaky on windows and debug
because it mentions http://crbug.com/40671 which appears to be something
different.

I'm also changing some arguments to enums instead of booleans to improve
readability (see cl 2714015 for the discussion).

[Lei Zhang, 2010-06-17 02:31:44]

I think this is just avoiding the underlying problem. A user should be able to
run:
google-chrome foo.com bar.com quz.com ... baz.com

and the command should work. If the test that simulates this scenario fails, the
solution should not be to fix the test.

I'll all for the bits that make the code more readable, though.

[Patrick Horn, 2010-06-17 03:15:34]

On 2010/06/17 02:31:44, Lei Zhang wrote:
> I think this is just avoiding the underlying problem. A user should be able to
> run:
> google-chrome http://foo.com http://bar.com http://quz.com ... http://baz.com
> 
> and the command should work. If the test that simulates this scenario fails,
the
> solution should not be to fix the test.
> 
> I'll all for the bits that make the code more readable, though.

I definitely agree with you that this should be tested and fixed. In fact,
that's what my main project is during my internship.

However, that problem should be tested in StartupTest (PerfSeveralTabs is now
committed), and not ShutdownTest which doesn't measure the amount of time for
tab loads anyway, so the test results are only a boolean whether it got a
timeout or not.

If you look at the 10.6 perf(2) bot, you can see spikes where the startup test
takes many times longer to load than normal, after I had changed the test down
to 10 tabs instead of 20. I think those were where loading 20 tabs at once was
randomly taking more than 30 seconds to start and causing failures on the
shutdown and startup tests (previously those spikes may have been ignored
because of the assertion failure from timing out).
http://build.chromium.org/buildbot/perf/mac-release-10.6/startup/report.html

[Lei Zhang, 2010-06-17 06:31:23]

On 2010/06/17 03:15:34, Patrick Horn wrote:
> 
> I definitely agree with you that this should be tested and fixed. In fact,
> that's what my main project is during my internship.
> 
> However, that problem should be tested in StartupTest (PerfSeveralTabs is now
> committed), and not ShutdownTest which doesn't measure the amount of time for
> tab loads anyway, so the test results are only a boolean whether it got a
> timeout or not.

Ok, can you file a bug for this so we don't forget about this problem? Bonus
points for writing a new test that exposes the problem so we can work on it.

It seems this CL should be for just fixing the flakiness. Making the code more
readable (i.e. enum TestImportance) should go in a separate CL and also change
the other files in chrome/test/startup/ at the same time so they remain
consistent in style to each other.

[Patrick Horn, 2010-06-17 16:59:01]

Re-uploaded the test without the change to the existing arguments. I left the
new parameter as an enum.

[Lei Zhang, 2010-06-17 20:13:53]

LGTM if you file a bug for the twenty tabs launch problem.

http://codereview.chromium.org/2809017/diff/10001/11001
File chrome/test/startup/shutdown_test.cc (right):

http://codereview.chromium.org/2809017/diff/10001/11001#newcode34
chrome/test/startup/shutdown_test.cc:34: TWENTY_TABS
nit: comma after the last element.
Please put in a quick description of each element.

http://codereview.chromium.org/2809017/diff/10001/11001#newcode61
chrome/test/startup/shutdown_test.cc:61:
browser_proxy->AppendTab(net::FilePathToFileURL(test_cases[i]));
wrap inside ASSERT_TRUE() ?

[Patrick Horn, 2010-06-17 23:05:46]

Oh I think I forgot to mention the bug I filed is at http://crbug.com/46609
Is there something missing from that bug report? I'll edit the bug description
to make it clear that StartupTest.PerfSeveralTabs reproduces it as well, after
ShutdownTest is fixed.

http://codereview.chromium.org/2809017/diff/10001/11001
File chrome/test/startup/shutdown_test.cc (right):

http://codereview.chromium.org/2809017/diff/10001/11001#newcode34
chrome/test/startup/shutdown_test.cc:34: TWENTY_TABS
On 2010/06/17 20:13:53, Lei Zhang wrote:
> nit: comma after the last element.
> Please put in a quick description of each element.

Done.

http://codereview.chromium.org/2809017/diff/10001/11001#newcode61
chrome/test/startup/shutdown_test.cc:61:
browser_proxy->AppendTab(net::FilePathToFileURL(test_cases[i]));
On 2010/06/17 20:13:53, Lei Zhang wrote:
> wrap inside ASSERT_TRUE() ?

Done.

[Lei Zhang, 2010-06-17 23:09:24]

LGTM

http://codereview.chromium.org/2809017/diff/15001/16001
File chrome/test/startup/shutdown_test.cc (right):

http://codereview.chromium.org/2809017/diff/15001/16001#newcode33
chrome/test/startup/shutdown_test.cc:33: SIMPLE, // Runs with no command line
arguments (loads about:blank).
code linter says "At least two spaces is best between code and comments"