environs/ec2: test that we can connect to zookeeper after bootstrap.

Also simplify tests a little.

https://code.launchpad.net/~rogpeppe/juju/go-zk-connect/+merge/97076

(do not edit description out of merge proposal)

[rog, 2012-03-13 15:30:56]

Please take a look.

[rog, 2012-03-13 15:36:31]

Please take a look.

[niemeyer, 2012-03-13 15:41:10]

LGTM, but is there really only two files in this proposal? The content of the CL
doesn't seem to match the description.

https://codereview.appspot.com/5754103/diff/2001/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/5754103/diff/2001/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:105: c.Skip("cannot test bootstrap on local server")
A test that is empty and always skipped is of questionable usefulness. :-)

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:42: // repeat for a while to let eventual
consistency catch up, hopefully.
Eventual consistency catching up could mean a few different things. I suggest
something more clear:

// Stopping may not be noticed at first due to eventual
// consistency. Repeat a few times to ensure we get the error.

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:74: c.Errorf("state open failed: %v, %T, %d",
err, err, err)
Please use this instead:

    c.Assert(err, IsNil)

If you really want to highlight the fact it comes from state.Open (which isn't
necessary IMO.. we'll have the context, and you're even logging), you can use
this:

    // Open state failed
    c.Assert(err, IsNil)

Either of those will show the error value, its type, and its string
representation. The second will also show the comment, if that's really
necessary.

[fwereade, 2012-03-13 16:54:55]

LGTM

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:71: c.Logf("open state")
I'm still not sure what we get out of logging tests step by step in this way;
won't any failures we encounter tell us everything the log does?

[rog, 2012-03-13 18:03:05]

Please take a look.

[niemeyer, 2012-03-13 18:26:35]

LGTM, thanks.

https://codereview.appspot.com/5754103/diff/9001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/5754103/diff/9001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:53: CanConnect: true,
CanOpenState, please, so we can tell what it means more easily.

[niemeyer, 2012-03-13 18:27:51]

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:71: c.Logf("open state")
On 2012/03/13 16:54:55, fwereade wrote:
> I'm still not sure what we get out of logging tests step by step in this way;
> won't any failures we encounter tell us everything the log does?

Yeah, I also feel a bit awkward about such log messages that are actually
stating the precise action being realized. It feels like they're covering for:
1) Bad logging by the package itself; 2) Bad error messages. We should fix both.

[rog, 2012-03-13 18:31:49]

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:42: // repeat for a while to let eventual
consistency catch up, hopefully.
On 2012/03/13 15:41:10, niemeyer wrote:
> Eventual consistency catching up could mean a few different things. I suggest
> something more clear:
> 
> // Stopping may not be noticed at first due to eventual
> // consistency. Repeat a few times to ensure we get the error.

Done.

https://codereview.appspot.com/5754103/diff/2001/environs/jujutest/livetests....
environs/jujutest/livetests.go:74: c.Errorf("state open failed: %v, %T, %d",
err, err, err)
On 2012/03/13 15:41:10, niemeyer wrote:
> Please use this instead:
> 
>     c.Assert(err, IsNil)
> 
> If you really want to highlight the fact it comes from state.Open (which isn't
> necessary IMO.. we'll have the context, and you're even logging), you can use
> this:
> 
>     // Open state failed
>     c.Assert(err, IsNil)
> 
> Either of those will show the error value, its type, and its string
> representation. The second will also show the comment, if that's really
> necessary.

this is a hangover from trying to work out what the errors returned from state
signified (which was why i made a noise about fixing error messages in state and
in general). as it happens, i'd probably fixed the print about 30 seconds after
you started reviewing - you were too quick! :-)

you're right - the Destroy can go.

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:71: c.Logf("open state")
On 2012/03/13 16:54:55, fwereade wrote:
> I'm still not sure what we get out of logging tests step by step in this way;
> won't any failures we encounter tell us everything the log does?

it's very convenient to see the log messages when each step can take minutes. (i
usually run go test -amazon -gocheck.vv)

[niemeyer, 2012-03-13 18:33:10]

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/5754103/diff/18/environs/jujutest/livetests.go...
environs/jujutest/livetests.go:71: c.Logf("open state")
On 2012/03/13 18:31:50, rog wrote:
> On 2012/03/13 16:54:55, fwereade wrote:
> > I'm still not sure what we get out of logging tests step by step in this
way;
> > won't any failures we encounter tell us everything the log does?
> 
> it's very convenient to see the log messages when each step can take minutes.
(i
> usually run go test -amazon -gocheck.vv)

That's definitely true, at all times! Such log message should be a nicely
polished log message within the package itself, not in the test.

[rog, 2012-03-14 12:32:55]

*** Submitted:

environs/ec2: test that we can connect to zookeeper after bootstrap.

Also simplify tests a little.

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/5754103