NewTabUITest.NTPHasThumbnails can be flakey if it takes more than

NewTabUITest.NTPHasThumbnails can be flakey if it takes more than
2 sec for the history results to be returned (displaying the
thumbnails).  Work around this by giving extra time to the test.

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

[Paweł Hajdan Jr., 2009-10-27 21:07:37]

OK after fixing the comments.

It's still going to be flaky. Please either don't mark the bug as fixed, or put
a TODO in the code (or open a new bug etc just so that we fix that at some
point).

It'd be nice if we could wait for an event like "all thumbnails loaded", but it
seems non-trivial. But maybe we can record in the backend how many thumbnails
loaded, and make an automation call to wait for that?

http://codereview.chromium.org/339029/diff/1/2
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/339029/diff/1/2#newcode46
Line 46: ASSERT_TRUE(tab->ExecuteAndExtractInt(L"",
It may improve things a little when instead of ASSERTing true here, you just
take bool success = tab->Execute... and then change the conditional below to
success && filler_thumbnails_count != 0.

http://codereview.chromium.org/339029/diff/1/2#newcode52
Line 52: wait_time -= kWaitDuration;
It seems it loops immediately, doesn't sleep.

[tony, 2009-10-27 21:27:41]

On 2009/10/27 21:07:37, Paweł Hajdan Jr. wrote:
> OK after fixing the comments.
> 
> It's still going to be flaky. Please either don't mark the bug as fixed, or
put
> a TODO in the code (or open a new bug etc just so that we fix that at some
> point).

Ok, I will leave the bug open and add the below suggestion in a comment.

> It'd be nice if we could wait for an event like "all thumbnails loaded", but
it
> seems non-trivial. But maybe we can record in the backend how many thumbnails
> loaded, and make an automation call to wait for that?
> 
> http://codereview.chromium.org/339029/diff/1/2
> File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):
> 
> http://codereview.chromium.org/339029/diff/1/2#newcode46
> Line 46: ASSERT_TRUE(tab->ExecuteAndExtractInt(L"",
> It may improve things a little when instead of ASSERTing true here, you just
> take bool success = tab->Execute... and then change the conditional below to
> success && filler_thumbnails_count != 0.

I'd like to see this cause a problem before changing it.  I don't think it
should ever fail.

> http://codereview.chromium.org/339029/diff/1/2#newcode52
> Line 52: wait_time -= kWaitDuration;
> It seems it loops immediately, doesn't sleep.

Oops.  Done.

[Paweł Hajdan Jr., 2009-10-27 21:31:33]

LGTM

> Ok, I will leave the bug open and add the below suggestion in a comment.

Thanks.

> > http://codereview.chromium.org/339029/diff/1/2#newcode46
> > Line 46: ASSERT_TRUE(tab->ExecuteAndExtractInt(L"",
> > It may improve things a little when instead of ASSERTing true here, you just
> > take bool success = tab->Execute... and then change the conditional below to
> > success && filler_thumbnails_count != 0.
> 
> I'd like to see this cause a problem before changing it.  I don't think it
> should ever fail.

Makes sense.