Code review comment for lp:~johnweldon4/juju-core/action

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode8
state/action_test.go:8: SettingsSuite
Ooh, didn't spot that: this will include all the Test* methods from
SettingsSuite as well, and we don't want that. I'd recommend ConnSuite
(the one right here in state_test, not the one in... er juju/testing?).

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
On 2014/05/15 16:23:28, johnweldon4 wrote:

> On 2014/05/15 12:47:26, fwereade wrote:
> > FWIW we usually leave these out, but I think the explicitness of
this approach
> > may actually be better. Don't feel obliged to include them if you
don't think
> > it's necessary.

> Do you mean the (SetUp|TearDown)(Suite|Test) funcs?
> Because they're just calling the inner SettingsSuite funcs does that
mean the
> test harness will do it automatically?

Yeah, if you embed a type you get all its methods (so long as there's no
ambiguity, which there isn't in this case).

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
On 2014/05/15 16:23:28, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > Hmm, I think we *should* make sure that the unit exists and isn't
Dead.
> >
> > (I've been thinking, and have tentatively concluded that actions on
dying
> units
> > are fine, they just might not run in time; but actions on dead units
are
> > definitely insane.)

> In this case I'm actually not even testing any "real" unit. Of course
when we
> do the refactor to make func (u* Unit) AddAction(...) then we'll need
a real
> Unit for this test.
> But this test is kind of superfluous any way. state_test has a very
similar
> test already.

I think this suite's existence is good, it's nice to cluster the tests
for related functionality, and there's an awful lot on the direct State
tests already.

https://codereview.appspot.com/98260043/