LGTM

I have some comments, but I can live with the code as it is written. So
if having a helper is too much distraction, just land it.



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

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode318
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State,
1000)
On 2013/07/10 18:28:46, dimitern wrote:
> why 1000? why not unbuffered?

I don't think we want unbuffered, because then when the other loop
starts up it will block until we do our select (I think).
The trick is that once you call 'sendOpenedStates' *all* things that
start up are going to be trying to send their state to this channel.


The general idea is: "I'm going to start something up, and I want a
backdoor into the State object it is using, so send it onto this channel
so I can pull it out in this code."

However, once you set the channel, more things are going to be sent on
it than you really need. We could just create a local goroutine that
consumes them all and puts it somewhere we can use it.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode319
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
On 2013/07/10 18:28:46, dimitern wrote:
> this a but confusing - perhaps a comment before this whole block
explaining
> what's being done will help

A slightly better pattern is probably:

oldChan := sendOpenedStates(agentStates)
defer sendOpenedStates(oldChan)

For things like this, I usually prefer to have the sendOpenedStates type
function actually return a func() that does the cleanup. So the code
becomes:

func sendOpenedStates(ch <-chan *state.State) {
   old, agentStates := agentStates, ch
   return func() { agentStates := old) }
}

As then the code in the test case is:

cleanup := sendOpenedStates(agentStates)
defer cleanup()

And that is a common pattern regardless of what internal poking a given
test helper has to do.

Nested function calls and defer are hard to parse for mere mortals,
which is why I don't spell it as:

defer sendOpenedStates(agentStates)()
     ^------------------------------^
The indicator of what you are deferring is very far from 'defer' vs

cleanup := ...
defer cleanup()

It is also why I'm not 100% sold on the:

go func() {
...
...
...
...
...
..
...
.
.
.
.
..
}()

Syntax. At least it is "go func() {" which makes it clear you are
thinking about calling func at the end of this. But when you leave off
those last two parenthesis it gets pretty confusing. :)

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode329
cmd/jujud/machine_test.go:329: case <-time.After(500 *
time.Millisecond):
Please use the testing.LongWait constants. It will help signal *why* you
are picking a given timeout, and lets us tweak them across the codebase
if we find they are too long/short.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode386
cmd/jujud/machine_test.go:386: timeout := time.After(500 *
time.Millisecond)
Same here on the timeout.

This loop has been used elsewhere, but I'm not quite sure why we have to
call s.State.StartSync() every 50ms. Is it because there are multiple
steps being done, and only 1 is done per sync?

Because if we *didn't* need to trigger the sync every 50ms, this would
be exactly the NotifyWatcherC code, and it would be nice if we could
reuse that object.

Maybe it needs to be another helper,

func (wc *NotifyWatcherC) AssertNoticeChanges(observe func() bool) {
   wc.State.Sync()
   timeout := time.After(testing.LongWait)
   for finished := false; !finished ; {
      select {
        case <- timeout:
          c.Fatalf("failed to notice the desired changes after %s",
LongWait)
        case <-time.After(ShortWait):
          c.State.StartSync()
        case <-wc.w.Changes():
          finished = observe()
   }
}

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newcode396
cmd/jujud/machine_test.go:396: return
'return' in a test suite is often confusing. Because it means if someone
adds assertions after this loop, they might not notice that they aren't
run.

I think using the 'done bool' pattern would work well here.

for finished := false; !finished ; {
...
   if err != nil {
     // We got an error, so we are done looping
     finished = true
     c.Assert(err, jc.Satifies, errors.IsNotFoundError)
   }
}

Or if we add AssertNoticeChanges this becomes:

wc.AssertNoticeChanges(func() bool {
   err := unit.Refresh()
   if err == nil {
     return false
   }
   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
   return true
}

(One can argue whether the return value should be "continue searching"
vs "done searching". I tend to prefer the positive value [opposite to
what I have written])

https://codereview.appspot.com/10825044/