mstate: add tests for ops w.r.t. life cycle

mstate: add tests for ops w.r.t. life cycle

Also some related bug fixes, and add LoggingSuite to MgoSuite
(to be removed when mstate is renamed and we can use "juju/testing".ConnSuite)

There are still bugs - deadOnAbort is used inappropriately in places.
To be fixed in subsequent branches.

https://code.launchpad.net/~rogpeppe/juju-core/069-mstate-not-dead/+merge/124948

(do not edit description out of merge proposal)

[rog, 2012-09-18 15:56:06]

Please take a look.

[niemeyer, 2012-09-18 17:02:01]

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go
File mstate/assign_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go#newcod...
mstate/assign_test.go:159: return unit.AssignToMachine(machine)
This is TestAssignSubordinatesToMachine. Feels like these checks are unrelated.

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go#newcod...
mstate/assign_test.go:171: // the machine, because that will fail if the unit is
Dying.
So just create a new unit? I doubt that'd take more space than all the logic
below plus this comment, and we'd not have to build this whole scenario in mind
to see what it means.

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go
File mstate/life_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go#newcode205
mstate/life_test.go:205: // in all life states.
The basic concept is great, but it feels like all these helpers are spoiling the
idea and making the tests harder to read, with pretty much no gain. Compare
this:

  assertOkForAllLife(c, obj, fs)

With this:

  testWhenDying(c, obj, noErr, noErr, fs)

I suggest renaming assertOkForLife to testWhenDying, and  dropping all those
helpers.

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go#newcode222
mstate/life_test.go:222: const notAliveErrPat = ".*: not found or not alive"
s/Pat// seems good enough.

Additionally, noErr = "" would make things more clear.

https://codereview.appspot.com/6527047/diff/2001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/machine.go#newcode169
mstate/machine.go:169: sel := append(notDead, D{{"principal", pudoc.Name}}...)
Why notDead?

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go
File mstate/machine_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go#newco...
mstate/machine_test.go:155: func (s *MachineSuite) TestRefreshWhenNotAlive(c *C)
{
This should be something like TestMethodsWhenDying, or the test should be split.

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go#newco...
mstate/machine_test.go:156: // Refresh and instance id should work regardless of
liveness status.
// Check method behavior when machine is dying.

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go#newco...
mstate/machine_test.go:164: if err != nil {
Double-testing.

https://codereview.appspot.com/6527047/diff/2001/mstate/relation_test.go
File mstate/relation_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/relation_test.go#newc...
mstate/relation_test.go:83: assertOkForAllLife(c, rel, func() error {
We really need this to be "testWhenDying", otherwise there is no hint in the
test that this is changing rel.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode171
mstate/service.go:171: Assert: isAlive,
Nice catch.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode178
mstate/service.go:178: err = fmt.Errorf("unit already exists or service is not
alive")
The unit can't exist. We've just created a globally unique name.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode220
mstate/service.go:220: return nil, fmt.Errorf("cannot add unit to service %q:
%v", s, err)
Oops.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode272
mstate/service.go:272: err = s.st.units.Find(D{{"service",
s.doc.Name}}).Sort("_id").All(&docs)
Why the Sort? Note that foo/10 comes before foo/2.

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go#newco...
mstate/service_test.go:31: assertOkWhenAliveOnly(c, s.service, func() error {
The check below is verifying a specific bug that happened. It feels pretty bad
to be abusing such an unrelated check to test for liveness modifications.

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go#newco...
mstate/service_test.go:213: // Check that retrieving all units works.
This should be in TestAllUnitsWhenDying.

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go
File mstate/unit.go (left):

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go#oldcode358
mstate/unit.go:358: case u.doc.MachineId != nil && *u.doc.MachineId != m.Id():
The error reporting was quite good here. Why are we losing it?

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go#newcode327
mstate/unit.go:327: return fmt.Errorf("unit is subordinate")
s/sub/a sub/

[rog, 2012-09-18 18:42:22]

Please take a look.

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go
File mstate/assign_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go#newcod...
mstate/assign_test.go:159: return unit.AssignToMachine(machine)
On 2012/09/18 17:02:01, niemeyer wrote:
> This is TestAssignSubordinatesToMachine. Feels like these checks are
unrelated.

moved into another test.

https://codereview.appspot.com/6527047/diff/2001/mstate/assign_test.go#newcod...
mstate/assign_test.go:171: // the machine, because that will fail if the unit is
Dying.
On 2012/09/18 17:02:01, niemeyer wrote:
> So just create a new unit? I doubt that'd take more space than all the logic
> below plus this comment, and we'd not have to build this whole scenario in
mind
> to see what it means.

removed comment as discussed in person

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go
File mstate/life_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go#newcode205
mstate/life_test.go:205: // in all life states.
On 2012/09/18 17:02:01, niemeyer wrote:
> The basic concept is great, but it feels like all these helpers are spoiling
the
> idea and making the tests harder to read, with pretty much no gain. Compare
> this:
> 
>   assertOkForAllLife(c, obj, fs)
> 
> With this:
> 
>   testWhenDying(c, obj, noErr, noErr, fs)
> 
> I suggest renaming assertOkForLife to testWhenDying, and  dropping all those
> helpers.

Done.

https://codereview.appspot.com/6527047/diff/2001/mstate/life_test.go#newcode222
mstate/life_test.go:222: const notAliveErrPat = ".*: not found or not alive"
On 2012/09/18 17:02:01, niemeyer wrote:
> s/Pat// seems good enough.
> 
> Additionally, noErr = "" would make things more clear.

Done.

https://codereview.appspot.com/6527047/diff/2001/mstate/machine.go
File mstate/machine.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/machine.go#newcode169
mstate/machine.go:169: sel := append(notDead, D{{"principal", pudoc.Name}}...)
On 2012/09/18 17:02:01, niemeyer wrote:
> Why notDead?

removed filter.

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go
File mstate/machine_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/machine_test.go#newco...
mstate/machine_test.go:155: func (s *MachineSuite) TestRefreshWhenNotAlive(c *C)
{
On 2012/09/18 17:02:01, niemeyer wrote:
> This should be something like TestMethodsWhenDying, or the test should be
split.

removed instance id test as discussed in person

https://codereview.appspot.com/6527047/diff/2001/mstate/relation_test.go
File mstate/relation_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/relation_test.go#newc...
mstate/relation_test.go:83: assertOkForAllLife(c, rel, func() error {
On 2012/09/18 17:02:01, niemeyer wrote:
> We really need this to be "testWhenDying", otherwise there is no hint in the
> test that this is changing rel.

Done.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode178
mstate/service.go:178: err = fmt.Errorf("unit already exists or service is not
alive")
On 2012/09/18 17:02:01, niemeyer wrote:
> The unit can't exist. We've just created a globally unique name.

Done.

https://codereview.appspot.com/6527047/diff/2001/mstate/service.go#newcode272
mstate/service.go:272: err = s.st.units.Find(D{{"service",
s.doc.Name}}).Sort("_id").All(&docs)
On 2012/09/18 17:02:01, niemeyer wrote:
> Why the Sort? Note that foo/10 comes before foo/2.

moved the sort into the test.

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go
File mstate/service_test.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go#newco...
mstate/service_test.go:31: assertOkWhenAliveOnly(c, s.service, func() error {
On 2012/09/18 17:02:01, niemeyer wrote:
> The check below is verifying a specific bug that happened. It feels pretty bad
> to be abusing such an unrelated check to test for liveness modifications.

left alone after discussion in person.

https://codereview.appspot.com/6527047/diff/2001/mstate/service_test.go#newco...
mstate/service_test.go:213: // Check that retrieving all units works.
On 2012/09/18 17:02:01, niemeyer wrote:
> This should be in TestAllUnitsWhenDying.

Done.

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6527047/diff/2001/mstate/unit.go#newcode327
mstate/unit.go:327: return fmt.Errorf("unit is subordinate")
On 2012/09/18 17:02:01, niemeyer wrote:
> s/sub/a sub/

Done.

[niemeyer, 2012-09-18 18:46:54]

LGTM!

[rog, 2012-09-18 18:47:59]

*** Submitted:

mstate: add tests for ops w.r.t. life cycle

Also some related bug fixes, and add LoggingSuite to MgoSuite
(to be removed when mstate is renamed and we can use "juju/testing".ConnSuite)

There are still bugs - deadOnAbort is used inappropriately in places.
To be fixed in subsequent branches.

R=niemeyer
CC=
https://codereview.appspot.com/6527047