testing: add dummy testing environment.

testing: add dummy testing environment.

This is a preliminary to being able to test the
commands without interacting with any particular
provider implementation.

https://code.launchpad.net/~rogpeppe/juju/go-testing-environs/+merge/99050

(do not edit description out of merge proposal)

[rog, 2012-03-23 15:44:02]

Please take a look.

[rog, 2012-03-23 16:44:15]

Please take a look.

[rog, 2012-03-26 11:40:06]

Please take a look.

[rog, 2012-03-26 11:41:40]

On 2012/03/26 11:40:06, rog wrote:
> Please take a look.

note that the changes to state are to remove the dependency cycle
internal_test.go->testing->state.

[niemeyer, 2012-03-26 22:53:05]

Sorry, quite a few comments, but they're mainly polishing suggestions. I quite
like the improvements there.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go
File testing/environs.go (right):

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode1
testing/environs.go:1: package testing
This doesn't seem like a good fit for the testing package. The repo stuff that
we've added here already made me wonder, but this one feels clearly like a
misfit for a top-level testing, and bound to create confusion.

A much better location for it, for example, would be "environs/dummy",
sustaining the dummy terminology. Among other problems, this will also avoid the
"testing environment" terminology, which is ambiguous with any other environment
being used for testing.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode14
testing/environs.go:14: type EnvOp struct {
Moving that onto environs/dummy, I'd rename types as follows:

EnvOp => Operation
EnvOpKind => OperationKind
EnvBootstrap => OpBootstrap

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode16
testing/environs.go:16: Name string
Kind and Name refer to the kind and the name of different things, which is
confusing (kind of the op, name of the env). EnvironName might help.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode32
testing/environs.go:32: type testEenvirons struct {
testEenvirons, testingEnvirons, testEnvirons.. we need better names.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode39
testing/environs.go:39: // It can be shared between several environ insts,
s/insts/values/? There's something that is called as "insts" in this context,
but it doesn't look like this is referring to it.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode50
testing/environs.go:50: // DiscardEnvOps can be used to pass to ListenEnvirons.
s/can be used to pass to/may be provided to/

That said, this seems to be an internal implementation detail that doesn't have
to be exposed as it's of no use except for simplifying the implementation of
this package. I suggest just taking nil in the ListenEnvirons function for
dropping the previously registered function, and internally adding a "value
eater", if that makes the implementation simpler.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode68
testing/environs.go:68: // ListenEnvirons registers the given channel to receive
operations
This is doing way more than listening. It actually resets the environment,
de-registers the previous function, closes the previous listener, etc. I suggest
calling it something that is a bit more obviously messing around with things,
such as dummy.Reset(ch) or similar.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode73
testing/environs.go:73: // unless it was nil, and so will panic if any values
are sent to it.
The sentences above would benefit from clarification too. Here is a suggested
shortened comment. Feel free to tweak if necessary.

// Reset closes any previously registered operation channel,
// cleans the environment state, and registers c to receive
// notifications of operations performed on newly opened
// dummy environments. All opened environments after a Reset
// will share the same underlying state (instances, etc).

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode76
testing/environs.go:76: // must specify a "name" property. Instance ids will
Every environment necessarily has a name. We shouldn't need an additional
property for a second name.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode80
testing/environs.go:80: // The DNS name of insts is the same as the Id,
s/insts/instances/

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode86
testing/environs.go:86: close(testingEnvirons.ops)
This would close the DiscardEnvOps too, and cause trouble.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode143
testing/environs.go:143: id: fmt.Sprintf("%s-%d", e.name, e.state.n),
Nitpick: I'd compress these three lines in a single one.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode163
testing/environs.go:163: n := 0
s/n/notFound/; such context-less yet semantically rich one-letters are the
devil. One has to read the whole function to see what it's supposed to mean.

[rog, 2012-03-27 12:49:35]

Please take a look.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go
File testing/environs.go (right):

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode1
testing/environs.go:1: package testing
On 2012/03/26 22:53:05, niemeyer wrote:
> This doesn't seem like a good fit for the testing package. The repo stuff that
> we've added here already made me wonder, but this one feels clearly like a
> misfit for a top-level testing, and bound to create confusion.
> 
> A much better location for it, for example, would be "environs/dummy",
> sustaining the dummy terminology. Among other problems, this will also avoid
the
> "testing environment" terminology, which is ambiguous with any other
environment
> being used for testing.

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode14
testing/environs.go:14: type EnvOp struct {
On 2012/03/26 22:53:05, niemeyer wrote:
> Moving that onto environs/dummy, I'd rename types as follows:
> 
> EnvOp => Operation
> EnvOpKind => OperationKind
> EnvBootstrap => OpBootstrap

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode16
testing/environs.go:16: Name string
On 2012/03/26 22:53:05, niemeyer wrote:
> Kind and Name refer to the kind and the name of different things, which is
> confusing (kind of the op, name of the env). EnvironName might help.

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode32
testing/environs.go:32: type testEenvirons struct {
On 2012/03/26 22:53:05, niemeyer wrote:
> testEenvirons, testingEnvirons, testEnvirons.. we need better names.

Done. I hope.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode39
testing/environs.go:39: // It can be shared between several environ insts,
On 2012/03/26 22:53:05, niemeyer wrote:
> s/insts/values/? There's something that is called as "insts" in this context,
> but it doesn't look like this is referring to it.

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode50
testing/environs.go:50: // DiscardEnvOps can be used to pass to ListenEnvirons.
On 2012/03/26 22:53:05, niemeyer wrote:
> s/can be used to pass to/may be provided to/
> 
> That said, this seems to be an internal implementation detail that doesn't
have
> to be exposed as it's of no use except for simplifying the implementation of
> this package. I suggest just taking nil in the ListenEnvirons function for
> dropping the previously registered function, and internally adding a "value
> eater", if that makes the implementation simpler.

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode68
testing/environs.go:68: // ListenEnvirons registers the given channel to receive
operations
On 2012/03/26 22:53:05, niemeyer wrote:
> This is doing way more than listening. It actually resets the environment,
> de-registers the previous function, closes the previous listener, etc. I
suggest
> calling it something that is a bit more obviously messing around with things,
> such as dummy.Reset(ch) or similar. 

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode73
testing/environs.go:73: // unless it was nil, and so will panic if any values
are sent to it.
On 2012/03/26 22:53:05, niemeyer wrote:
> The sentences above would benefit from clarification too. Here is a suggested
> shortened comment. Feel free to tweak if necessary.
> 
> // Reset closes any previously registered operation channel,
> // cleans the environment state, and registers c to receive
> // notifications of operations performed on newly opened
> // dummy environments. All opened environments after a Reset
> // will share the same underlying state (instances, etc).

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode76
testing/environs.go:76: // must specify a "name" property. Instance ids will
On 2012/03/26 22:53:05, niemeyer wrote:
> Every environment necessarily has a name. We shouldn't need an additional
> property for a second name.

agreed. but i need at least one mandatory field so that environs can test that
failing to provide a mandatory field correctly gives an error.

i've added mandatory boolean field specifying whether or not to start zookeeper
when StateInfo is called. (currently not implemented - it must always be false).

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode80
testing/environs.go:80: // The DNS name of insts is the same as the Id,
On 2012/03/26 22:53:05, niemeyer wrote:
> s/insts/instances/

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode86
testing/environs.go:86: close(testingEnvirons.ops)
On 2012/03/26 22:53:05, niemeyer wrote:
> This would close the DiscardEnvOps too, and cause trouble.

Done.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode143
testing/environs.go:143: id: fmt.Sprintf("%s-%d", e.name, e.state.n),
On 2012/03/26 22:53:05, niemeyer wrote:
> Nitpick: I'd compress these three lines in a single one.

i'd prefer to leave it as it was before, if that's ok. it's clear, and you
didn't complain when it was in dummyprovider_test.go :-)

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode163
testing/environs.go:163: n := 0
On 2012/03/26 22:53:05, niemeyer wrote:
> s/n/notFound/; such context-less yet semantically rich one-letters are the
> devil. One has to read the whole function to see what it's supposed to mean.

Done.

[rog, 2012-03-27 14:11:09]

Please take a look.

[niemeyer, 2012-03-28 01:22:45]

Very nice. Just a few additional trivials, and LGTM.

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go
File testing/environs.go (right):

https://codereview.appspot.com/5892043/diff/5001/testing/environs.go#newcode143
testing/environs.go:143: id: fmt.Sprintf("%s-%d", e.name, e.state.n),
On 2012/03/27 12:49:36, rog wrote:
> On 2012/03/26 22:53:05, niemeyer wrote:
> > Nitpick: I'd compress these three lines in a single one.
> 
> i'd prefer to leave it as it was before, if that's ok. it's clear, and you

No worries. It's an irrelevant nitpick.

> didn't complain when it was in dummyprovider_test.go :-)

I can easily overlook stuff in a review.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:19: // Operation represents an action on the testing
provider.
s/testing provider/dummy environment/

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:30: // dummyEnvirons represents the dummy provider. 
There is only ever one
Can we please call this an environProvider, same as its counterpart in ec2?

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:43: n            int // instance count
How's that different from len(insts)?  If there's a justifiable difference, this
needs a proper name such as "instCount" or whatever, and a comment explaining
why it's different.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:77: // the first time StateInfo is called on a newly
reset environment.
// NOTE: ZooKeeper isn't actually being started yet.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:91: environsInstance.state = &environState{
What about the other fields?

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:101: "zookeeper": schema.Const(false),
At the end of the line: // TODO

[rog, 2012-03-28 07:09:51]

Please take a look.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:19: // Operation represents an action on the testing
provider.
On 2012/03/28 01:22:45, niemeyer wrote:
> s/testing provider/dummy environment/

Done.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:43: n            int // instance count
On 2012/03/28 01:22:45, niemeyer wrote:
> How's that different from len(insts)?  If there's a justifiable difference,
this
> needs a proper name such as "instCount" or whatever, and a comment explaining
> why it's different.

yeah, the comment and name were bogus. fixed.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:91: environsInstance.state = &environState{
On 2012/03/28 01:22:45, niemeyer wrote:
> What about the other fields?

the zero default is fine for all others - we just need this
for making the maps.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:101: "zookeeper": schema.Const(false),
On 2012/03/28 01:22:45, niemeyer wrote:
> At the end of the line: // TODO

Done.

[niemeyer, 2012-03-28 15:03:59]

LGTM, thanks.

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/5892043/diff/9003/environs/dummy/environs.go#n...
environs/dummy/environs.go:91: environsInstance.state = &environState{
On 2012/03/28 07:09:51, rog wrote:
> On 2012/03/28 01:22:45, niemeyer wrote:
> > What about the other fields?
> 
> the zero default is fine for all others - we just need this
> for making the maps.

Sorry, I mixed environState and environsInstance in my head.

https://codereview.appspot.com/5892043/diff/23001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/5892043/diff/23001/environs/dummy/environs.go#...
environs/dummy/environs.go:49: var environsInstance environProvider
s/environsInstance/providerInstance/?

[rog, 2012-03-28 15:25:39]

*** Submitted:

testing: add dummy testing environment.

This is a preliminary to being able to test the
commands without interacting with any particular
provider implementation.

R=
CC=
https://codereview.appspot.com/5892043