jujud: more robust test

jujud: more robust test

TestHostUnits was sometimes panicing with a nil state pointer in
waitDeployedUnits; now it shouldn't.

https://code.launchpad.net/~fwereade/juju-core/better-jujud-machine-test/+merge/142874

(do not edit description out of merge proposal)

[fwereade, 2013-01-11 11:28:55]

Please take a look.

[rog, 2013-01-11 11:49:27]

definitely needs fixing, but perhaps not *quite* like that. a suggestion below.

https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go#newcode90
cmd/jujud/deploy_test.go:90: close(mgr.inited)
not entirely sure about this - it'll panic if newDeployManager is called twice.
maybe better with a buffered channel and a select-default send.

in fact, rather than setting the state and then signalling on a channel, we
could send the state itself down a channel. then i think we can do away with the
st field in fakeManager entirely.

[fwereade, 2013-01-11 12:05:51]

https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go#newcode90
cmd/jujud/deploy_test.go:90: close(mgr.inited)
On 2013/01/11 11:49:27, rog wrote:
> not entirely sure about this - it'll panic if newDeployManager is called
twice.
> maybe better with a buffered channel and a select-default send.
> 
> in fact, rather than setting the state and then signalling on a channel, we
> could send the state itself down a channel. then i think we can do away with
the
> st field in fakeManager entirely.

Honestly, I'm more inclined to just assert that mgr.st is nil -- I don't see a
situation in which the manager is recreated that doesn't indicate a bug. Until
we need to verify a use case involving multiple dpeloyers, I'd rather not layer
on more complexity than is warranted. Sane?

[rog, 2013-01-11 13:04:47]

On 2013/01/11 12:05:51, fwereade wrote:
> https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go
> File cmd/jujud/deploy_test.go (right):
> 
>
https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go#newcode90
> cmd/jujud/deploy_test.go:90: close(mgr.inited)
> On 2013/01/11 11:49:27, rog wrote:
> > not entirely sure about this - it'll panic if newDeployManager is called
> twice.
> > maybe better with a buffered channel and a select-default send.
> > 
> > in fact, rather than setting the state and then signalling on a channel, we
> > could send the state itself down a channel. then i think we can do away with
> the
> > st field in fakeManager entirely.
> 
> Honestly, I'm more inclined to just assert that mgr.st is nil -- I don't see a
> situation in which the manager is recreated that doesn't indicate a bug. Until
> we need to verify a use case involving multiple dpeloyers, I'd rather not
layer
> on more complexity than is warranted. Sane?

i think the shared state is actually more complex to reason about than sending
the state over the channel. is there a reason to store the state in the manager?

[fwereade, 2013-01-11 13:08:11]

On 2013/01/11 13:04:47, rog wrote:
> On 2013/01/11 12:05:51, fwereade wrote:
> > https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go
> > File cmd/jujud/deploy_test.go (right):
> > 
> >
>
https://codereview.appspot.com/7094043/diff/1/cmd/jujud/deploy_test.go#newcode90
> > cmd/jujud/deploy_test.go:90: close(mgr.inited)
> > On 2013/01/11 11:49:27, rog wrote:
> > > not entirely sure about this - it'll panic if newDeployManager is called
> > twice.
> > > maybe better with a buffered channel and a select-default send.
> > > 
> > > in fact, rather than setting the state and then signalling on a channel,
we
> > > could send the state itself down a channel. then i think we can do away
with
> > the
> > > st field in fakeManager entirely.
> > 
> > Honestly, I'm more inclined to just assert that mgr.st is nil -- I don't see
a
> > situation in which the manager is recreated that doesn't indicate a bug.
Until
> > we need to verify a use case involving multiple dpeloyers, I'd rather not
> layer
> > on more complexity than is warranted. Sane?
> 
> i think the shared state is actually more complex to reason about than sending
> the state over the channel. is there a reason to store the state in the
manager?

waitDeployed is called several times per test, so we need to keep state
*somewhere*. Where are you suggesting?

[rog, 2013-01-11 13:11:14]

On 2013/01/11 13:08:11, fwereade wrote:
> waitDeployed is called several times per test, so we need to keep state
> *somewhere*. Where are you suggesting?

ah, good point.

LGTM as is then.

perennial rant: i hate all these arbitrary 50ms delays. both too little and too
much.

[fwereade, 2013-01-11 13:11:57]

On 2013/01/11 13:11:14, rog wrote:
> On 2013/01/11 13:08:11, fwereade wrote:
> > waitDeployed is called several times per test, so we need to keep state
> > *somewhere*. Where are you suggesting?
> 
> ah, good point.
> 
> LGTM as is then.

Cheers.

> perennial rant: i hate all these arbitrary 50ms delays. both too little and
too
> much.

Heh, yes.

[dimitern, 2013-01-11 15:17:37]

LGTM

[fwereade, 2013-01-11 15:44:59]

*** Submitted:

jujud: more robust test

TestHostUnits was sometimes panicing with a nil state pointer in
waitDeployedUnits; now it shouldn't.

R=rog, dimitern
CC=
https://codereview.appspot.com/7094043