uniter_test: snappier waitUniterDead

uniter_test: snappier waitUniterDead

waitUniterDead was on occasion waiting almost the full 5 seconds before the
uniter happened to notice it was ready to die. We now repeatedly sync state
while waiting, in order to ensure timely detection of state changes on the
uniter side. On this machine, this change speeds up the uniter suite by
roughly 20%.

https://code.launchpad.net/~fwereade/juju-core/trivial-speed-uniter-tests/+merge/133041

(do not edit description out of merge proposal)

[fwereade, 2012-11-06 11:34:51]

Please take a look.

[jameinel, 2012-11-06 12:20:43]

The StartSync comment seems to indicate that it is already called in the
background, however I don't see anything that calls it that isn't in a test
file.

I'm happy to see the test suite improvements, but I'm wondering if we should be
polling StartSync inline, or if it should be more a goroutine that runs as part
of test setup.

Otherwise: LGTM

[aram, 2012-11-06 13:58:34]

> The StartSync comment seems to indicate that it is already called
> in the background, however I don't see anything that calls it
> that isn't in a test file.

StartSync is a function that forces a run of the bottom-half of the low-level
watcher. It has no semantic meaning (code should run with and without it). It is
used to make tests run faster, but should not be used for real code.

The watchers' main loops run periodically, that's why there is
no need to ever call StartSync.

> I'm wondering if we should be polling StartSync inline, or if it
> should be more a goroutine that runs as part of test setup.

No. One calls StartSync when it expects results from the
watchers. It can't be done independent of the code that wants it.

[aram, 2012-11-06 13:58:37]

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:848: timeout := time.After(5 * time.Second)
Why not just put ctx.st.StartSync() before the select, and leave the select as
is? That's what we do everywhere else.

[rog, 2012-11-09 10:21:35]

LGTM but i'd like to know the same thing as Aram. it could at least do with a
comment.

[fwereade, 2012-11-12 13:22:30]

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:848: timeout := time.After(5 * time.Second)
On 2012/11/06 13:58:37, aram wrote:
> Why not just put ctx.st.StartSync() before the select, and leave the select as
> is? That's what we do everywhere else.

Because that only works when the changes we're waiting for have already
occurred. In lots of places, we can guarantee that; in this case, we know
nothing about the context in which we handle a waitDead step, and it could
easily be that a few separate events need to occur before the uniter actually
stops.

[aram, 2012-11-12 14:12:45]

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:848: timeout := time.After(5 * time.Second)
I'm slightly concerned about this. It looks like we have a race and we
decided to solve it by altering timings and (almost) busy waiting.

Isn't there a clean way to solve this problem? (I'm just asking because I
am not familiar with the code). If there isn't any cleaner way to solve
this problem, or if the technical cost of solving it in a better way is
too high, LGTM (but please add a comment).

[fwereade, 2012-11-13 16:37:45]

*** Submitted:

uniter_test: snappier waitUniterDead

waitUniterDead was on occasion waiting almost the full 5 seconds before the
uniter happened to notice it was ready to die. We now repeatedly sync state
while waiting, in order to ensure timely detection of state changes on the
uniter side. On this machine, this change speeds up the uniter suite by
roughly 20%.

R=
CC=
https://codereview.appspot.com/6815089

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6815089/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:848: timeout := time.After(5 * time.Second)
On 2012/11/12 14:12:45, aram wrote:
> I'm slightly concerned about this. It looks like we have a race and we
> decided to solve it by altering timings and (almost) busy waiting.
> 
> Isn't there a clean way to solve this problem? (I'm just asking because I
> am not familiar with the code). If there isn't any cleaner way to solve
> this problem, or if the technical cost of solving it in a better way is
> too high, LGTM (but please add a comment).

Well, it *is* rare for us to need additional syncs, but it's not unheard of and
the bit that demands it was discussed and OKed a while ago. Commented and
submitted.