On 2013/09/11 12:05:59, jameinel wrote:
> Because this is being added to the environs interface we should have a
test
> added in environs/jujutest/livetests.go Or possibly just
".../tests.go" but I'd
> like to see the former. That gives us an interface-level test against
all things
> that implement Environs. (Except Azure, because it hasn't been hooked
into the
> live testing infrastructure yet.)

Thanks, I wasn't familiar with this common test code. Done.

> Offhand this could just be that constraints that we will definitely
support
> (like all values of nil) are checked to not return an error. And maybe
testing
> an LXC sort of constraint and test that either it succeeds (err ==
nil) or it
> fails in a known way.

> Given the functions you've written, something like:

> func (s *LiveSuite) TestSanityCheckConstraints(c *gc.C) {
>      var cons constraints.Value
>      c.Check(s.Env.SanityCheckConstraints(cons), gc.IsNil)
>      cons.Container = new(instance.ContainerType)
>      *cons.Container = instance.LXC
>      err := s.Env.SanityCheckConstraints(cons)
>      // If err is nil, that is fine, some providers support containers
>      if err != nil {
>          // But for ones that don't, they should have a standard syntax
for
> erroring
>          c.Check(err, gc.ErrorMatches, ".*provider does not support
containers")
>      }
> }

Thanks, added that in. I tidied up the imports while I was at it.


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go
> File cmd/juju/addmachine_test.go (right):


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addmachine_test.go#newcode112
> cmd/juju/addmachine_test.go:112: dummy.SanityCheckConstraintsError =
> errors.New("computer says no")
> You don't clean up after setting this value, which will give errors in
strange
> ways for other tests (I think).
> You need something like:

> defer func() { dummy.SanityCheckConstraints = nil }


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go
> File cmd/juju/addunit_test.go (right):


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/addunit_test.go#newcode143
> cmd/juju/addunit_test.go:143: dummy.SanityCheckConstraintsError =
> errors.New("computer says no")
> Similarly here, you should be resetting the error when the test
finishes.


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go
> File cmd/juju/deploy_test.go (right):


https://codereview.appspot.com/13632046/diff/4001/cmd/juju/deploy_test.go#newcode250
> cmd/juju/deploy_test.go:250: dummy.SanityCheckConstraintsError =
> stderrors.New("computer says no")
> and here

> Maybe turn SanityCheckConstraintsError into a function that returns a
cleanup
> func, rather than reproducing that in every test that wants to
override it.

> So you would have:

> cleanup := dummy.SetSanityCheckConstraintsError(errors.New("stuff"))
> defer cleanup()

> https://codereview.appspot.com/13632046/diff/4001/juju/conn.go
> File juju/conn.go (right):


https://codereview.appspot.com/13632046/diff/4001/juju/conn.go#newcode342
> juju/conn.go:342: }
> My first thought on reading this was that you don't need to validate
> containerType if you got "3/lxc/2" because that machine must already
exist.
> But looking closer, I'm a bit confused by the logic here. It looks
like you get
> really weird behavior if you do: lxc:2:3

> It actually looks like you could do: lxc:2:lxc:3 to create a new lxc
on machine
> 2/lxc/3, though if we really want to support that, we'd use the syntax
> lxc:2/lxc/3
> (I don't really understand why we have the strings.Join(specParts[1:])

> Anyway, I realize you just moved the code. Though I don't fully know
why you had
> to move it.


https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test.go
> File provider/azure/environ_test.go (right):


https://codereview.appspot.com/13632046/diff/4001/provider/azure/environ_test.go#newcode94
> provider/azure/environ_test.go:94: *cons.Container = instance.LXC
> It is really unfortunate that it has to be written this way.
> But from what I can tell, you can't take the address of a const, and
you can't
> use &instance.ContainerType("lxc")

> You can do it as:

> container := instance.LXC
> cons.Container = &container

> I don't know if that is actually cleaner, though. (I'm just generally
not a big
> fan of new())


https://codereview.appspot.com/13632046/diff/4001/provider/local/environ.go
> File provider/local/environ.go (right):


https://codereview.appspot.com/13632046/diff/4001/provider/local/environ.go#newcode100
> provider/local/environ.go:100: return nil
> I don't think we actually support nesting LXC's in the local provider.
So we
> should probably check and return an error here, rather than allowing
it.



https://codereview.appspot.com/13632046/