Increase timer in render_widget_host_unittest to reduce flakiness in valgrind

Increase timer in render_widget_host_unittest to reduce flakiness in valgrind

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

[Charlie Reis, 2011-11-14 20:59:34]

LGTM.

[jbates, 2011-11-14 22:38:09]

Adding jam for owners stamp.

[jam, 2011-11-15 00:14:03]

adding Pawel as well for his thoughts, he's dealt with this a lot.

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:752: new
MessageLoop::QuitTask(), 40);
this is really really flaky, independent of the timeout value picked. Whatever
machine this runs on can be doing other stuff in the background, and this would
fail randomly, adding work to sheriffs/trybot runners/commit queue etc. It's
better not to have a test than to have one that picks a constant to sleep. Is
there no other event/notification to watch for?

[Charlie Reis, 2011-11-15 00:23:05]

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:752: new
MessageLoop::QuitTask(), 40);
On 2011/11/15 00:14:03, John Abd-El-Malek wrote:
> this is really really flaky, independent of the timeout value picked. Whatever
> machine this runs on can be doing other stuff in the background, and this
would
> fail randomly, adding work to sheriffs/trybot runners/commit queue etc. It's
> better not to have a test than to have one that picks a constant to sleep. Is
> there no other event/notification to watch for?

The point of this test is to test a timer.  If you set the timer to expire in N
milliseconds (line 748), then set another timer to expire in M >= N
milliseconds, the second timer shouldn't fire first.

If there are notifications to listen for, though, I agree that would be better.

[jam, 2011-11-15 00:37:58]

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:752: new
MessageLoop::QuitTask(), 40);
On 2011/11/15 00:23:05, creis wrote:
> On 2011/11/15 00:14:03, John Abd-El-Malek wrote:
> > this is really really flaky, independent of the timeout value picked.
Whatever
> > machine this runs on can be doing other stuff in the background, and this
> would
> > fail randomly, adding work to sheriffs/trybot runners/commit queue etc. It's
> > better not to have a test than to have one that picks a constant to sleep.
Is
> > there no other event/notification to watch for?
> 
> The point of this test is to test a timer.

Excuse the naive question, but if the point of the test is to test timer
support, shouldn't we be testing this at the lowest level, which is Timer's
unittest in base/timer_unittest.cc?

>  If you set the timer to expire in N
> milliseconds (line 748), then set another timer to expire in M >= N
> milliseconds, the second timer shouldn't fire first.
> 
> If there are notifications to listen for, though, I agree that would be
better.

[Charlie Reis, 2011-11-15 00:45:05]

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:752: new
MessageLoop::QuitTask(), 40);
On 2011/11/15 00:37:58, John Abd-El-Malek wrote:
> Excuse the naive question, but if the point of the test is to test timer
> support, shouldn't we be testing this at the lowest level, which is Timer's
> unittest in base/timer_unittest.cc?

Hopefully Timer is already tested there.

The RenderWidget's logic for starting and stopping its hang monitor timer is
complex in itself, which is why this test was added.  Thus, this test starts,
stops, and starts the hang monitor timer and makes sure that it still fires. 
Just testing Timer would miss bugs in RenderWidget (which we've had).

[jam, 2011-11-15 06:19:39]

On 2011/11/15 00:45:05, creis wrote:
>
http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
> File content/browser/renderer_host/render_widget_host_unittest.cc (right):
> 
>
http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
> content/browser/renderer_host/render_widget_host_unittest.cc:752: new
> MessageLoop::QuitTask(), 40);
> On 2011/11/15 00:37:58, John Abd-El-Malek wrote:
> > Excuse the naive question, but if the point of the test is to test timer
> > support, shouldn't we be testing this at the lowest level, which is Timer's
> > unittest in base/timer_unittest.cc?
> 
> Hopefully Timer is already tested there.
> 
> The RenderWidget's logic for starting and stopping its hang monitor timer is
> complex in itself, which is why this test was added.  Thus, this test starts,
> stops, and starts the hang monitor timer and makes sure that it still fires. 
> Just testing Timer would miss bugs in RenderWidget (which we've had).

I see, thanks for the explanation, this makes sense now to me. lgtm.

[Paweł Hajdan Jr., 2011-11-15 09:01:31]

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_unittest.cc (right):

http://codereview.chromium.org/8533015/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_unittest.cc:752: new
MessageLoop::QuitTask(), 40);
On 2011/11/15 00:14:03, John Abd-El-Malek wrote:
> this is really really flaky, independent of the timeout value picked.

Thanks for adding me. I don't know the code tested well. I think that if the
timer runs on a separate thread, it's definitely racy no matter the timeout you
pick here, and if it runs on the same thread it depends on the semantics of
MessageLoop and delayed tasks, which I also don't know very well, but then it
has a chance to work (and is probably very desirable to work that way).

In general I'm not a fan of tests that use timers or are otherwise
timing-dependent, but after reading the discussion it seems we need some kind of
"integration test" here.

LGTM, and feel free to let me know how the hang monitor timeout works, I might
have some ideas to improve testability.

[commit-bot: I haz the power, 2011-11-15 22:35:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/8533015/3001

[commit-bot: I haz the power, 2011-11-15 23:55:23]

Change committed as 110213