Merge lp://staging/~fwereade/juju-core/jujud-integrate-cleaner-resumer into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+170907_code.launchpad.net,

Message:
Please take a look.

Description:
jujud: JobManageState runs cleaner, resumer

Nothing to see there... but I also got angry and tweaked Agent so I
could
just test the damn tasks themselves and not worry about the side
effects.
And then I needed to test RunAgentLoop as well to verify that it does
the
right thing with the tasks it gets from a mocked Agent.

https://code.launchpad.net/~fwereade/juju-core/jujud-integrate-cleaner-resumer/+merge/170907

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10439046/

Affected files:
   A [revision details]
   M cmd/jujud/agent.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go
   M cmd/jujud/unit.go
   M environs/testing/storage.go

Some questions about the testing infrastructure, but overall
LGTM

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent.go#oldcode130
cmd/jujud/agent.go:130: RunOnce(st *state.State, entity AgentState)
error
I do really like the rename of RunOnce to StartTasks.
As RunOnce can be interpreted as "only ever run this once", which
confuses it a bit with why you are calling it in a loop.

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
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?)

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
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.)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcode448
cmd/jujud/agent_test.go:448: }
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.

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.

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).

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 conn...

Conflicts irreparably with API changes to jujud

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...