environs/jujutest: fix live tests.

environs/jujutest: fix live tests.



https://code.launchpad.net/~rogpeppe/juju-core/085-fix-ec2-live/+merge/126506

(do not edit description out of merge proposal)

[niemeyer, 2012-09-26 17:33:37]

I know it's in progress, but some randoms to help:

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

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:273: for _ = range w.Changes() {
As far as I can see, this isn't doing anything useful.

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:282: // nil if the waiter dies.
It should return the error instead, as usual.

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:291: changed := w.lastTools == nil || *tools ==
*w.lastTools
The second comparison is inverted. Is this working?

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:310: c.Assert(tools, NotNil)
Assert(err, IsNil) would be better.

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:337: c.Assert(tools, NotNil)
Ditto.

[rog, 2012-09-27 11:38:44]

Please take a look.

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

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:273: for _ = range w.Changes() {
On 2012/09/26 17:33:37, niemeyer wrote:
> As far as I can see, this isn't doing anything useful.

currently it's not, but as implemented, a toolsWaiter can be implement on a Unit
too, which will be done in an upcoming branch.

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:282: // nil if the waiter dies.
On 2012/09/26 17:33:37, niemeyer wrote:
> It should return the error instead, as usual.

Done.

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:291: changed := w.lastTools == nil || *tools ==
*w.lastTools
On 2012/09/26 17:33:37, niemeyer wrote:
> The second comparison is inverted. Is this working?

that was indeed the bug i was looking for. thanks!

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:310: c.Assert(tools, NotNil)
On 2012/09/26 17:33:37, niemeyer wrote:
> Assert(err, IsNil) would be better.

Done.

[rog, 2012-09-27 11:48:40]

Please take a look.

[fwereade, 2012-09-27 12:24:50]

LGTM again :)

[niemeyer, 2012-09-27 13:15:23]

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

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:273: for _ = range w.Changes() {
On 2012/09/27 11:38:44, rog wrote:
> On 2012/09/26 17:33:37, niemeyer wrote:
> > As far as I can see, this isn't doing anything useful.
> 
> currently it's not, but as implemented, a toolsWaiter can be implement on a
Unit
> too, which will be done in an upcoming branch.

A comment would be nice then, for the next person in line to not have to make
the same question.

https://codereview.appspot.com/6566058/diff/5006/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6566058/diff/5006/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:200: mgoPort),
This is the only place in this package that we use this private variable. I
suggest dropping the constant and hardcoding it instead. It's cleaner and more
readable.

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

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:234: err = w1.EnsureDead()
Huh?

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:259: watcher
This doesn't look good. This is a tools waiter. Why are we exposing the
interface of our first class objects via a *tool waiter*!?

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:295: c.Assert(err, IsNil)
if err != nil {
    return err
}

This method doesn't need c.

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:303: return nil, fmt.Errorf("watcher finished:
%v", w.Err())
That looks like a success message.

"watcher channel closed prematurely: %v"

https://codereview.appspot.com/6566058/diff/5006/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/6566058/diff/5006/state/ssh.go#newcode33
state/ssh.go:33: conn, err := mgo.DialWithTimeout(fwd.localAddr, 30*time.Minute)
I'd prefer to not change this now, and if we do change it should be changed
consistently. I'd prefer to know if it's taking more than 10 minutes now.

[rog, 2012-09-27 14:08:30]

Please take a look.

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

https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:273: for _ = range w.Changes() {
On 2012/09/27 13:15:23, niemeyer wrote:
> On 2012/09/27 11:38:44, rog wrote:
> > On 2012/09/26 17:33:37, niemeyer wrote:
> > > As far as I can see, this isn't doing anything useful.
> > 
> > currently it's not, but as implemented, a toolsWaiter can be implement on a
> Unit
> > too, which will be done in an upcoming branch.
> 
> A comment would be nice then, for the next person in line to not have to make
> the same question.

i put a comment on the field.

https://codereview.appspot.com/6566058/diff/5006/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6566058/diff/5006/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:200: mgoPort),
On 2012/09/27 13:15:23, niemeyer wrote:
> This is the only place in this package that we use this private variable. I
> suggest dropping the constant and hardcoding it instead. It's cleaner and more
> readable.

Actually there are quite a few places here that use mgoPortSuffix which derives
from this variable. If we're not going to hard code all of those, then I'd
prefer to derive this from the same source.

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

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:234: err = w1.EnsureDead()
On 2012/09/27 13:15:23, niemeyer wrote:
> Huh?

Done.

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:259: watcher
On 2012/09/27 13:15:23, niemeyer wrote:
> This doesn't look good. This is a tools waiter. Why are we exposing the
> interface of our first class objects via a *tool waiter*!?

done, i think.

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:295: c.Assert(err, IsNil)
On 2012/09/27 13:15:23, niemeyer wrote:
> if err != nil {
>     return err
> }
> 
> This method doesn't need c.

done, but kept c for the logging.

https://codereview.appspot.com/6566058/diff/5006/environs/jujutest/livetests....
environs/jujutest/livetests.go:303: return nil, fmt.Errorf("watcher finished:
%v", w.Err())
On 2012/09/27 13:15:23, niemeyer wrote:
> That looks like a success message.
> 
> "watcher channel closed prematurely: %v"

Done.

https://codereview.appspot.com/6566058/diff/5006/state/ssh.go
File state/ssh.go (right):

https://codereview.appspot.com/6566058/diff/5006/state/ssh.go#newcode33
state/ssh.go:33: conn, err := mgo.DialWithTimeout(fwd.localAddr, 30*time.Minute)
On 2012/09/27 13:15:23, niemeyer wrote:
> I'd prefer to not change this now, and if we do change it should be changed
> consistently. I'd prefer to know if it's taking more than 10 minutes now.

Done.

[niemeyer, 2012-09-27 14:15:41]

LGTM. Just a couple of questions for your consideration:

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

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:210: instId1, err := m1.InstanceId()
Do we need to refresh to pick the new instance id?

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:260: watcher
I don't see the value of having this as an anonymous field. Am I missing
something?

[rog, 2012-09-27 14:29:41]

*** Submitted:

environs/jujutest: fix live tests.

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

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

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:210: instId1, err := m1.InstanceId()
On 2012/09/27 14:15:42, niemeyer wrote:
> Do we need to refresh to pick the new instance id?

no, but done anyway, so we're not relying on the toolsWaiter semantics.

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:260: watcher
On 2012/09/27 14:15:42, niemeyer wrote:
> I don't see the value of having this as an anonymous field. Am I missing
> something?

i used it for the Stop method, but changed to make it more explicit.