Thanks for the review; dropping this due to collision.


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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode275
cmd/jujud/agent_test.go:275: conf.MachineNonce = state.BootstrapNonce
On 2013/06/23 12:53:07, jameinel wrote:
> I thought all machines that aren't machine/0 were meant to have their
own
> Nonce's. Is this just to have *something*? (As in, it could just as
easily be a
> random string, as long as you set it and preserve it?)

Ha, yeah, I moved that method without reading it. Not sure why that's
there at all.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode430
cmd/jujud/agent_test.go:430: s.retryDelay, retryDelay = retryDelay,
time.Millisecond
On 2013/06/23 12:53:07, jameinel wrote:
> I appreciate not having delays in the test suite.
> I wonder about using a more obvious naming convention for
package-global
> variables that will be mutated in separate files.

> Certainly when I read this, it looks like a function-local
'retryDelay'
> variable. I realize there is no ':', and I can do the 'grep' to figure
out where
> 'retryDelay' is defined (somewhere in the cmd/jujud package).

> Nothing to do with your proposed changes, just something about "where
is this
> variable coming from" that I find hard to read with our current
idioms. (In
> python you at least have to say 'global retryDelay' so you know to go
look for
> it somewhere else. And even then it would still be in the same file.)

I think we've got a bit of a general problem with code being pretty
randomly scattered across files in a package... that's not for want of
bikeshedding, though. It's worse here because we're testing in-package
and the usual restrictions don't apply.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode448
cmd/jujud/agent_test.go:448: }
On 2013/06/23 12:53:07, jameinel wrote:
> Not specifically related to your patch, but "type Agent interface" has
no doc
> strings for me to understand what it is actually trying to do. And
AgentState is
> defined in 'upgrade.go' while 'Agent' is defined in agent.go.

I hate spelunking through this package for exactly these reasons :).

> Anyway, the point I wanted to make was to think about Mock and what we
are
> trying to do. The concern is always that a Mock gets out of sync with
the real
> thing, and your tests just always keep passing because Mock doesn't
migrate.

> I'm guessing go's "interface" helps us enough here, in that if Agent
changes its
> interface mockAgent will be updated to match (at least the function
signatures).
> Like in this case, you changed from RunOnce to StartTasks, you do have
to update
> the mock or the compiler will complain.

Yeah; doesn't help with semantic changes, but it's handy all the same.

> In general, I love to see decoupling of the various bits of Juju and
make it
> easier to test just one layer. Though I generally prefer tested
Doubles (they
> act like the thing they are doubling) rather than ultralightweight
Mocks (they
> record what they were called with).

No theoretical argument, but I'm not sure it's necessary to be any
heavier here. It's just a vehicle for navigating the connection dance
until we get to the point where we can inject our tasks.

> It also seems odd that you are able to mock out running 'heavyweight
tasks', but
> you still have to have a State object that connects to a real mongodb
running.
> It feels like if you are going to mock anything, you would want to
remove the
> State connection first.

I mocked what I needed to test what I changed, with a bias for avoidance
of potential rats' nests ;).

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode508
cmd/jujud/agent_test.go:508: worker.ErrTerminateAgent,
On 2013/06/23 12:53:07, jameinel wrote:
> If you are testing 'retry until something happens', shouldn't you have
more
> errors after the one you expect to terminate the loop, and then you
can check
> that it stopped at the right step?

> For example, what happens if you just remove
'worker.ErrTerminateAgent', from
> what I can see it would trigger a panic() in Wait because of a
> slice-out-of-bounds error. But I think that only triggers in the
goroutine, so
> you would still end up with the loop stopping. (I guess maybe it dies
with a
> timeout instead of a done signal because RunAgentLoop doesn't recover
from
> panics?)

Good points all. Cheers.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode518
cmd/jujud/agent_test.go:518: }
On 2013/06/23 12:53:07, jameinel wrote:
> Maybe adding a 'c.Check(errTask.waitCount, Equals, 6)' to be sure how
many times
> it waited, and what finally terminated the looping.

Bit more inclined to put a C into errTask, but we'll see.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode578
cmd/jujud/agent_test.go:578: tomb.Kill(stderrors.New("external"))
On 2013/06/23 12:53:07, jameinel wrote:
> I'm not sure this test does what you want it to.

> I dug into the tomb details, and it says "only the first non-nil error
is
> recorded as reason", and you are using tomb.Kill(non-nil) to initiate
the stop
> code.
> So even though your waitTask.Stop() code *also* signals an error, I'm
pretty
> sure this test is functionally identical to the TestRunUntilStopped.

> I would consider dropping it unless you can actually observe a
difference.

I'm actively testing that the task error while stopping does not leak
out of RunAgentLoop -- it's a different test to the one above, in which
the task exits cleanly. I could probably just Kill(nil) in both cases,
but turning a nil into a nil isn't very impressive.

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/machine_test.go#newcode253
cmd/jujud/machine_test.go:253: // This is horrible. But it makes this
functionality somewhat testable...
On 2013/06/23 12:53:07, jameinel wrote:
> I can't say I quite understand why this is horrible. Care to
elaborate?

I don't have any way to get an agent instance into this useful state
without mucking directly with internals, and it makes me nervous that
the test might not catch some relevant change at some point in the
future.

https://codereview.appspot.com/10439046/