Break up unit test to avoid internal timing interactions between parts....

Break up unit test to avoid internal timing interactions between parts.

Hopefully this will cure teh flaky problem under Valgrind.

I also had to re-read my own code, and so I put in to minor changes to
hopefully make it incrementally easier ot read.  There should be no semantic
changes caused by my coding changes, but the level of nested braces is 
reduced, which makes it easier ot see what is (supposed to be) going on.


r=dank,phajdan

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

[Evan Martin, 2009-04-07 01:24:31]

Ping on this.  Is it still valid?

You can mention this bug in the description if you like.
http://code.google.com/p/chromium/issues/detail?id=9194

[Evan Martin, 2009-05-13 18:38:48]

Ping again for great justice!!!

[dank, 2009-05-13 18:43:20]

I don't think this change helped.  
http://codereview.chromium.org/42147 did, but I stopped
pursuing it after jar had a whack at it, hoping he'd
solve it.  Maybe we should go back to my try?

[jar (doing other things), 2009-05-13 19:45:51]

LGTM

I think I see the fundamental problem with the unit test is that I can't re-use
the watchdog "too quickly."  There is indeed a race (as the incrementing of the
counter is not atomic with the reseting of the alarm... and it is probably hard
to get it right with a lock in the derived (testing) class :-/ ).

Breaking the test up into two pieces avoids the issue of reuse, and avoids the
race.  To put it another way: That rapid-reuse element should not have been
tested, as it is not ensured by the implementation.  Maybe I *should* ensure
such, but it is not IMO necessary.

FYI: Your cleanup code of the if/else using continue instead has been sitting in
one of my trees forever.  It definitely makes it easier to read.

[Evan Martin, 2009-05-13 20:05:44]

On 2009/05/13 19:45:51, jar wrote:
> LGTM

Wait, you're the one who wrote this -- you can't LGTM it yourself.  :)

I was pinging in the hopes that you'd finish the patch, as it's one of our
valgrind bugs and today is the valgrind fixit.

[jar (doing other things), 2009-05-13 23:24:32]

<doh>... now I know why it looked so familiar.  Note that I created this patch
almost two months ago, but never landed it.  <sigh>

This patch will likely help with this specific test (and readability).

Is Dank asserting it won't help the test in question?

Evan: Do you like it enough that I can land it?

[dank, 2009-05-26 20:39:29]

It didn't help when I tried it.  
Can you test with valgrind yourself to see if it helps on your system?

[jar (doing other things), 2009-06-04 22:48:06]

Wan Teh,

Can you take a look at this?

The race condition I noted is that I may attempt to re-arm the alarm before the
first alarm has completed its cycle back into a calm mode. This happens when the
first alarm changes the status, but then gets suspended <oops>.  The result is
the second alarm arming does not take place (the alarm is still armed :-/), and
the unit test fails.  By braking the test into two pieces I avoid this
high-speed-reuse problem.

Thanks,

Jim

[wtc, 2009-06-04 23:14:24]

LGTM.

http://codereview.chromium.org/42266/diff/4/1003
File base/watchdog_unittest.cc (right):

http://codereview.chromium.org/42266/diff/4/1003#newcode80
Line 80: // Make sure a basic alarm fires when the time has expired.
This comment should show how AlarmPriorTimeTest differs from
AlarmTest.  This comment is exactly the same as the comment on
line 71!

http://codereview.chromium.org/42266/diff/4/1003#newcode82
Line 82: WatchdogCounter watchdog(TimeDelta::TimeDelta(), "Enabled2", true);
Why is the first argument TimeDelta::TimeDelta() instead
of TimeDelta::FromMilliseconds(10) or something like that?

Why can't this watchdog be named "Enabled"?  You didn't
changed the "Enabled" names in StartupShutdownTest and
ArmDisarmTest.