Fix flaky CountingPolicyTest.CapReturns test

Fix flaky CountingPolicyTest.CapReturns test

The CountingPolicyTest.CapReturns and FullStreamUIPolicyTest.CapReturns tests are occasionally flaking because the IO is slow and we have a low timeout of 5s set. This raises the timeout to 8s and makes sure the flush happens before our timeout starts.

Shoutout to mvrable for figuring out what was going on.

BUG=285823
NOTRY=true
R=mpcomplete@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221516

[felt, 2013-09-05 18:01:28]

Hi Matt, can you please review? Tiny Cl to fix a flaky test.

[mvrable, 2013-09-05 18:10:55]

lgtm except for two small comments.

Matt should still give this code a review.

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right):

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/counting_policy_unittest.cc:95:
FROM_HERE, timeout.callback(), base::TimeDelta::FromSeconds(5));
I would update this timeout as well to be consistent.

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc
(right):

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:123:
FROM_HERE, timeout.callback(), base::TimeDelta::FromSeconds(5));
This is the timeout which is actually used by the test in question.

[felt, 2013-09-05 18:17:56]

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc
(right):

https://codereview.chromium.org/23991002/diff/1/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:123:
FROM_HERE, timeout.callback(), base::TimeDelta::FromSeconds(5));
On 2013/09/05 18:10:55, mvrable wrote:
> This is the timeout which is actually used by the test in question.

Whoops, forgot my other CL (which removes the redundancy) hadn't landed yet.

[Matt Perry, 2013-09-05 18:49:19]

LGTM

https://codereview.chromium.org/23991002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc
(right):

https://codereview.chromium.org/23991002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:669:
base::MessageLoop::current()->Run();
nit: use RunLoop instead. see
https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&q=...

[Matt Perry, 2013-09-05 18:50:06]

BTW it'll probably still be flaky with 8s, just less so. Is there any way you
can restructure this to not rely on a timeout?

[mvrable, 2013-09-05 19:02:21]

On 2013/09/05 18:50:06, Matt Perry wrote:
> BTW it'll probably still be flaky with 8s, just less so. Is there any way you
> can restructure this to not rely on a timeout?

Adding
  policy->Flush();
  WaitOnThread(BrowserThread::DB);
(as this CL also does) should ensure that the time-consuming I/O completes
before we start the timer for the timeout.  So increasing the timeout probably
isn't necessary, but adds some safety.

The timeout is present to give a better error message, and to let other unit
tests run, if the data ready callback never fires for some reason.  Would you
rather not have that?

[Matt Perry, 2013-09-05 19:59:40]

On 2013/09/05 19:02:21, mvrable wrote:
> On 2013/09/05 18:50:06, Matt Perry wrote:
> > BTW it'll probably still be flaky with 8s, just less so. Is there any way
you
> > can restructure this to not rely on a timeout?
> 
> Adding
>   policy->Flush();
>   WaitOnThread(BrowserThread::DB);
> (as this CL also does) should ensure that the time-consuming I/O completes
> before we start the timer for the timeout.  So increasing the timeout probably
> isn't necessary, but adds some safety.
> 
> The timeout is present to give a better error message, and to let other unit
> tests run, if the data ready callback never fires for some reason.  Would you
> rather not have that?

OK, let's leave the timeout in.

[commit-bot: I haz the power, 2013-09-05 21:09:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/23991002/21001

[commit-bot: I haz the power, 2013-09-05 21:11:14]

Change committed as 221516