New test suite: BaseSuite

New test suite: BaseSuite

A new base test suite is introduced. It is to
replace LoggingSuite as the preferred suite
which all test suites should encapsulate. 
BaseSuite itseld encapsulates LoggingSuite, plus
provides:
- isolation of user home via creation of temp home
  and setting of $HOME
- flag to ensure tests will error if outgoing
  network access is attempted

Much of the implementation was moved across from
testing/environs.go. There were a number of
slightly different methods to create a fake Juju
home setup - these have been consolidated and 
simplified.

The flag to turn off outgoing network access when
running tests is off until the tests are fixed.
Not all tests have been updated to use the new
suite; only those affected by the refactoring
have been fixed for now.

https://code.launchpad.net/~wallyworld/juju-core/network-test-isolation/+merge/219633

(do not edit description out of merge proposal)

[wallyworld, 2014-05-15 05:25:22]

Please take a look.

[axw, 2014-05-15 07:52:13]

A couple of little things to fix, but otherwise looking good.

I'm a little worried about BaseSuite growing bigger and slower, and slowing down
all the tests.

https://codereview.appspot.com/100460044/diff/1/cmd/plugins/juju-metadata/ima...
File cmd/plugins/juju-metadata/imagemetadata_test.go (left):

https://codereview.appspot.com/100460044/diff/1/cmd/plugins/juju-metadata/ima...
cmd/plugins/juju-metadata/imagemetadata_test.go:56: for _, envstring := range
s.environ {
??
Is this a remnant of pre-PatchEnvironment?

https://codereview.appspot.com/100460044/diff/1/testing/base.go
File testing/base.go (right):

https://codereview.appspot.com/100460044/diff/1/testing/base.go#newcode36
testing/base.go:36: t.Home = MakeFakeHome(c)
Have you measured the overhead of SetUpTest and TearDownTest? There's a bunch of
filesystem writes in MakeFakeHome that we could do without in a lot of tests.

I wonder if we should be treating BaseSuite as a safe-guard at the *suite*
level, but not at the test case level.

https://codereview.appspot.com/100460044/diff/1/testing/base.go#newcode143
testing/base.go:143: func AddEnvironments(c *gc.C, envConfig string, certNames
...string) {
AddEnvironments implies, to me, accretion. I think WriteEnvironments would be
more accurate.

https://codereview.appspot.com/100460044/diff/1/testing/base_test.go
File testing/base_test.go (right):

https://codereview.appspot.com/100460044/diff/1/testing/base_test.go#newcode21
testing/base_test.go:21: var _ = gc.Suite(&TestingBaseSuite{})
This is an idea that just occurred to me, but I wonder if we could make the use
of BaseSuite a little more robust.

Could we introduce a new testing.Suite function that wraps the provided suite
with one that sets up the environment and provides that to gc.Suite?

gocheck is based on reflection anyhow, so that's probably not that
straightforward.

https://codereview.appspot.com/100460044/diff/1/utils/http.go
File utils/http.go (right):

https://codereview.appspot.com/100460044/diff/1/utils/http.go#newcode113
utils/http.go:113: parts := strings.Split(addr, ":")
check OutgoingAccessAllowed before parsing

https://codereview.appspot.com/100460044/diff/1/utils/http_test.go
File utils/http_test.go (right):

https://codereview.appspot.com/100460044/diff/1/utils/http_test.go#newcode106
utils/http_test.go:106: _, err := utils.Dial("tcp", "10.0.0.1:80")
This assumes that 10.0.0.1 is accessible to whatever is testing, which may not
be true. I think some more mocking is required...

[fwereade, 2014-05-15 09:48:44]

pre-comment: most of our tests should not have a $HOME, or a $JUJU_HOME, at all.
They're only relevant on client tests.

[fwereade, 2014-05-15 12:49:31]

On 2014/05/15 09:48:44, fwereade wrote:
> pre-comment: most of our tests should not have a $HOME, or a $JUJU_HOME, at
all.
> They're only relevant on client tests.

Chatted live; I maintain that long-term we don't want to expose those vars, but
I also agree that this approach is an improvement on what went before. If I
don't get to a full review today, someone else can, I don't want to block this.

[wallyworld, 2014-05-16 01:36:10]

Please take a look.

https://codereview.appspot.com/100460044/diff/1/cmd/plugins/juju-metadata/ima...
File cmd/plugins/juju-metadata/imagemetadata_test.go (left):

https://codereview.appspot.com/100460044/diff/1/cmd/plugins/juju-metadata/ima...
cmd/plugins/juju-metadata/imagemetadata_test.go:56: for _, envstring := range
s.environ {
On 2014/05/15 07:52:12, axw wrote:
> ??
> Is this a remnant of pre-PatchEnvironment?

This was the only test AFAICT which explicitly cleared all the env vars before
testing. I plan to introduce this back into the BaseSuite once we can get all
the tests to pass since they currently fail with all the env vars cleared.

https://codereview.appspot.com/100460044/diff/1/testing/base.go
File testing/base.go (right):

https://codereview.appspot.com/100460044/diff/1/testing/base.go#newcode36
testing/base.go:36: t.Home = MakeFakeHome(c)
On 2014/05/15 07:52:12, axw wrote:
> Have you measured the overhead of SetUpTest and TearDownTest? There's a bunch
of
> filesystem writes in MakeFakeHome that we could do without in a lot of tests.
> 
> I wonder if we should be treating BaseSuite as a safe-guard at the *suite*
> level, but not at the test case level.

I've factored out a separate FakeJujuHomeSuite so that BaseSuite just clears the
Juju related env vars and nothing more.

https://codereview.appspot.com/100460044/diff/1/testing/base.go#newcode143
testing/base.go:143: func AddEnvironments(c *gc.C, envConfig string, certNames
...string) {
On 2014/05/15 07:52:12, axw wrote:
> AddEnvironments implies, to me, accretion. I think WriteEnvironments would be
> more accurate.

Done.

https://codereview.appspot.com/100460044/diff/1/utils/http.go
File utils/http.go (right):

https://codereview.appspot.com/100460044/diff/1/utils/http.go#newcode113
utils/http.go:113: parts := strings.Split(addr, ":")
On 2014/05/15 07:52:12, axw wrote:
> check OutgoingAccessAllowed before parsing

Done.

https://codereview.appspot.com/100460044/diff/1/utils/http_test.go
File utils/http_test.go (right):

https://codereview.appspot.com/100460044/diff/1/utils/http_test.go#newcode106
utils/http_test.go:106: _, err := utils.Dial("tcp", "10.0.0.1:80")
On 2014/05/15 07:52:13, axw wrote:
> This assumes that 10.0.0.1 is accessible to whatever is testing, which may not
> be true. I think some more mocking is required...

Done.

[axw, 2014-05-16 02:31:32]

LGTM with a few small changes

https://codereview.appspot.com/100460044/diff/20001/testing/environ_test.go
File testing/environ_test.go (right):

https://codereview.appspot.com/100460044/diff/20001/testing/environ_test.go#n...
testing/environ_test.go:55: // Copyright 2013 Canonical Ltd.
What happened here?

https://codereview.appspot.com/100460044/diff/20001/utils/http.go
File utils/http.go (right):

https://codereview.appspot.com/100460044/diff/20001/utils/http.go#newcode117
utils/http.go:117: parts := strings.Split(addr, ":")
Sorry, I just noticed; please use net.SplitHostPort here. That'll allow for IPv6
in the future.

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go
File utils/http_test.go (right):

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go#newcod...
utils/http_test.go:106: func (s *dialSuite) assertDial(c *gc.C, addr string,
allowDial bool) {
no point in having allowDial is there? it should never be false, since you
should get an outer error before the net.Dial attempt
I'd drop the parameter and just c.Assert(dialed, jc.IsFalse)

[wallyworld, 2014-05-16 03:03:54]

Please take a look.

https://codereview.appspot.com/100460044/diff/20001/testing/environ_test.go
File testing/environ_test.go (right):

https://codereview.appspot.com/100460044/diff/20001/testing/environ_test.go#n...
testing/environ_test.go:55: // Copyright 2013 Canonical Ltd.
On 2014/05/16 02:31:32, axw wrote:
> What happened here?

lbox acting stupid with the diff

https://codereview.appspot.com/100460044/diff/20001/utils/http.go
File utils/http.go (right):

https://codereview.appspot.com/100460044/diff/20001/utils/http.go#newcode117
utils/http.go:117: parts := strings.Split(addr, ":")
On 2014/05/16 02:31:32, axw wrote:
> Sorry, I just noticed; please use net.SplitHostPort here. That'll allow for
IPv6
> in the future.

Done.

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go
File utils/http_test.go (right):

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go#newcod...
utils/http_test.go:106: func (s *dialSuite) assertDial(c *gc.C, addr string,
allowDial bool) {
On 2014/05/16 02:31:32, axw wrote:
> no point in having allowDial is there? it should never be false, since you
> should get an outer error before the net.Dial attempt
> I'd drop the parameter and just c.Assert(dialed, jc.IsFalse)

allowDial can be either true or false depending on whether outgoing access
should be allowed. See the tests below.

[axw, 2014-05-16 03:37:36]

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go
File utils/http_test.go (right):

https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go#newcod...
utils/http_test.go:106: func (s *dialSuite) assertDial(c *gc.C, addr string,
allowDial bool) {
On 2014/05/16 03:03:54, wallyworld wrote:
> On 2014/05/16 02:31:32, axw wrote:
> > no point in having allowDial is there? it should never be false, since you
> > should get an outer error before the net.Dial attempt
> > I'd drop the parameter and just c.Assert(dialed, jc.IsFalse)
> 
> allowDial can be either true or false depending on whether outgoing access
> should be allowed. See the tests below.

I looked at them. There's only two, and they both pass in true. It wouldn't make
sense to pass in false, because it shouldn't be possible. If allowDial == false,
then you should get an error instead.

[wallyworld, 2014-05-16 04:06:10]

On 2014/05/16 03:37:36, axw wrote:
> https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go
> File utils/http_test.go (right):
> 
>
https://codereview.appspot.com/100460044/diff/20001/utils/http_test.go#newcod...
> utils/http_test.go:106: func (s *dialSuite) assertDial(c *gc.C, addr string,
> allowDial bool) {
> On 2014/05/16 03:03:54, wallyworld wrote:
> > On 2014/05/16 02:31:32, axw wrote:
> > > no point in having allowDial is there? it should never be false, since you
> > > should get an outer error before the net.Dial attempt
> > > I'd drop the parameter and just c.Assert(dialed, jc.IsFalse)
> > 
> > allowDial can be either true or false depending on whether outgoing access
> > should be allowed. See the tests below.
> 
> I looked at them. There's only two, and they both pass in true. It wouldn't
make
> sense to pass in false, because it shouldn't be possible. If allowDial ==
false,
> then you should get an error instead.

Ignore me, I had deleted some other tests and didn't refactor right.