Convert crash recovery tests to browser test framework, which should reduce flakiness.

Convert crash recovery tests to browser test framework, which should reduce flakiness.

TEST=none
http://crbug.com/16054

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

[Nicolas Sylvain, 2009-07-07 18:00:18]

http://codereview.chromium.org/149235/diff/1/2
File chrome/browser/crash_recovery_browsertest.cc (right):

http://codereview.chromium.org/149235/diff/1/2#newcode73
Line 73: crash_observer.WaitForRendererCrash();
how long would this wait? What if it does not crash?

http://codereview.chromium.org/149235/diff/1/2#newcode76
Line 76: browser()->Reload();
would reload not actually reload about:crash?

http://codereview.chromium.org/149235/diff/1/2#newcode97
Line 97: IN_PROC_BROWSER_TEST_F(CrashRecoveryBrowserTest, LoadInNewTab) {
Is this test not exactly the same as the other one except for the url? Maybe we
should create an helper function that does all the common work?

http://codereview.chromium.org/149235/diff/1/4
File chrome/chrome.gyp (right):

http://codereview.chromium.org/149235/diff/1/4#newcode23
Line 23: 'browser/crash_recovery_browsertest.cc',
Does this really work on linux and mac?

[Paweł Hajdan Jr., 2009-07-08 16:55:56]

On 2009/07/07 18:00:18, Nicolas Sylvain wrote:
> http://codereview.chromium.org/149235/diff/1/2
> File chrome/browser/crash_recovery_browsertest.cc (right):
> 
> http://codereview.chromium.org/149235/diff/1/2#newcode73
> Line 73: crash_observer.WaitForRendererCrash();
> how long would this wait? What if it does not crash?

I followed pattern from WaitForNavigation here. I'm worried that having any kind
of timeout here increases chance of flakiness.

If the tests are solid and you notice the hang (in case it doesn't crash),
you'll know something is wrong with the change - ok, not necessarily what's
wrong.

If you have a good idea how to best solve it, I'll just do it.

> http://codereview.chromium.org/149235/diff/1/2#newcode76
> Line 76: browser()->Reload();
> would reload not actually reload about:crash?

My experiment shows it reloads the page. And it's just porting the UI test to
new framework. It might be fragile if Reload's behavior changes. But I think we
generally want reload on a crashed page to reload the page and not the sad tab
image.

> http://codereview.chromium.org/149235/diff/1/2#newcode97
> Line 97: IN_PROC_BROWSER_TEST_F(CrashRecoveryBrowserTest, LoadInNewTab) {
> Is this test not exactly the same as the other one except for the url? Maybe
we
> should create an helper function that does all the common work?

Indeed. I extracted common things to ui_test_utils because they have a high
chance of being useful for future tests.

> http://codereview.chromium.org/149235/diff/1/4
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/149235/diff/1/4#newcode23
> Line 23: 'browser/crash_recovery_browsertest.cc',
> Does this really work on linux and mac?

Definitely. I developed this test on Linux. And I bet it works on Mac (but
didn't test).

[Nicolas Sylvain, 2009-07-08 17:12:56]

LGTM. 

Still a little worried about the lack of timeout, since it's going to make the
browser tests hangs forever, and will make us realize the failure a lot later
(since it will stay green/pending for a long time before it is actually
considered failing).  But this is a design problem with the browser_tests, so we
can punt on this for now.

thanks

[Finnur, 2009-07-08 17:20:20]

Didn't Jay already add timeout detection for the browser tests?

On Wed, Jul 8, 2009 at 10:12, <nsylvain@chromium.org> wrote:

>
> LGTM.
>
> Still a little worried about the lack of timeout, since it's going to
> make the browser tests hangs forever, and will make us realize the
> failure a lot later (since it will stay green/pending for a long time
> before it is actually considered failing).  But this is a design problem
> with the browser_tests, so we can punt on this for now.
>
> thanks
>
>
> http://codereview.chromium.org/149235
>