Migrate: testbase to github testing

Migrate: testbase to github testing

Replace use of:

   launchpad.net/juju-core/testing/testbase
to
   github.com/juju/testing

https://code.launchpad.net/~waigani/juju-core/change-import-testbase/+merge/211188

(do not edit description out of merge proposal)

[waigani, 2014-03-17 01:23:18]

Please take a look.

[waigani, 2014-03-17 01:31:11]

On 2014/03/17 01:23:18, waigani wrote:
> Please take a look.

Main business logic changes:

- Deleted cmd, patch and cleanup from juju-core/testing/testbase.
- Embedded testing.LoggingSuite in testbase.LoggingSuite.
- Changed testbase func calls to testing calls where appropriate.

[axw, 2014-03-17 03:15:41]

LGTM

https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
File worker/rsyslog/rsyslog_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
worker/rsyslog/rsyslog_test.go:18: jujutesting
"launchpad.net/juju-core/juju/testing"
The new ambiguity between github.com/juju/testing and
launchpad.net/juju-core/juju/testing blows. Not your problem, just saying.

[thumper, 2014-03-18 02:42:36]

On 2014/03/17 03:15:41, axw wrote:
> LGTM
> 
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> File worker/rsyslog/rsyslog_test.go (right):
> 
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> worker/rsyslog/rsyslog_test.go:18: jujutesting
> "launchpad.net/juju-core/juju/testing"
> The new ambiguity between github.com/juju/testing and
> launchpad.net/juju-core/juju/testing blows. Not your problem, just saying.

NOT LGTM - yet

You need to resolve the conflicts and make sure the tests pass

[waigani, 2014-03-18 03:12:15]

On 2014/03/18 02:42:36, thumper wrote:
> On 2014/03/17 03:15:41, axw wrote:
> > LGTM
> > 
> >
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> > File worker/rsyslog/rsyslog_test.go (right):
> > 
> >
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> > worker/rsyslog/rsyslog_test.go:18: jujutesting
> > "launchpad.net/juju-core/juju/testing"
> > The new ambiguity between github.com/juju/testing and
> > launchpad.net/juju-core/juju/testing blows. Not your problem, just saying.
> 
> NOT LGTM - yet
> 
> You need to resolve the conflicts and make sure the tests pass

Pulled trunk, merged, ran bzr status: 0 conflicts, ran make check: 0 errors. ?

[waigani, 2014-03-18 03:14:49]

On 2014/03/18 03:12:15, waigani wrote:
> On 2014/03/18 02:42:36, thumper wrote:
> > On 2014/03/17 03:15:41, axw wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> > > File worker/rsyslog/rsyslog_test.go (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/76670043/diff/20001/worker/rsyslog/rsyslog_tes...
> > > worker/rsyslog/rsyslog_test.go:18: jujutesting
> > > "launchpad.net/juju-core/juju/testing"
> > > The new ambiguity between github.com/juju/testing and
> > > launchpad.net/juju-core/juju/testing blows. Not your problem, just saying.
> > 
> > NOT LGTM - yet
> > 
> > You need to resolve the conflicts and make sure the tests pass
> 
> Pulled trunk, merged, ran bzr status: 0 conflicts, ran make check: 0 errors. ?

Hang on, ignore me, I'm talking crap. Fixing now...

[thumper, 2014-03-18 03:17:56]

https://codereview.appspot.com/76670043/diff/20001/cmd/logging_test.go
File cmd/logging_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/logging_test.go#newcode23
cmd/logging_test.go:23: logging.LoggingSuite
this shouldn't be a LoggingSuite

It is testing the logging behaviour, so should stay a CleanupSuite.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validateimagemetadata_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:10:
"github.com/juju/testing"
don't need this import

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:152:
testing.PatchEnvironment("EC2_SECRET_KEY", "")
these four should all be:
  s.PatchEnvironment

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validatetoolsmetadata_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:10:
"github.com/juju/testing"
remove

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:97:
s.AddCleanup(func(*gc.C) { restore() })
change these to:

s.PatchEnvironment

no restore, no add cleanup needed.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:128:
testing.PatchEnvironment("AWS_SECRET_ACCESS_KEY", "")
ditto

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:96: defer testing.PatchValue(juju.ProviderConnectDelay,
coretesting.LongWait).Restore()
Add a variable to the reciever
  func (s *NewAPIClientSuite)

And use s.PatchValue, no defer needed.

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:145: defer testing.PatchValue(juju.APIOpen,
apiOpen).Restore()
ditto and for the following

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_linux_test.go
File juju/osenv/vars_linux_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_linux_test...
juju/osenv/vars_linux_test.go:12: testing.PatchEnvironment("HOME", h)
What is the import suite?

Can you just use PatchEnvironment from that?

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_windows_te...
File juju/osenv/vars_windows_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_windows_te...
juju/osenv/vars_windows_test.go:14: testing.PatchEnvironment("HOMEDRIVE", "")
ditto for using the PatchEnvironment from the importSuite

https://codereview.appspot.com/76670043/diff/20001/worker/instancepoller/mach...
File worker/instancepoller/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/instancepoller/mach...
worker/instancepoller/machine_test.go:49: defer testing.PatchValue(&ShortPoll,
coretesting.ShortWait/10).Restore()
all these can be changed to use the PatchValue from the suite

https://codereview.appspot.com/76670043/diff/20001/worker/peergrouper/worker_...
File worker/peergrouper/worker_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:65: testing.PatchValue(&pollInterval,
5*time.Millisecond)
Just use the patch from the suite.

https://codereview.appspot.com/76670043/diff/20001/worker/provisioner/contain...
File worker/provisioner/container_initialisation_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/provisioner/contain...
worker/provisioner/container_initialisation_test.go:54: aptCmdChan, cleanup :=
testing.HookCommandOutput(&utils.AptCommandOutput, []byte{}, nil)
should be able to use the HookCommandOutput from the suite.

[waigani, 2014-03-18 15:46:54]

Please take a look.

https://codereview.appspot.com/76670043/diff/20001/cmd/logging_test.go
File cmd/logging_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/logging_test.go#newcode23
cmd/logging_test.go:23: logging.LoggingSuite
On 2014/03/18 03:17:57, thumper wrote:
> this shouldn't be a LoggingSuite
> 
> It is testing the logging behaviour, so should stay a CleanupSuite.

Done.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validateimagemetadata_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:10:
"github.com/juju/testing"
On 2014/03/18 03:17:57, thumper wrote:
> don't need this import

Done.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:152:
testing.PatchEnvironment("EC2_SECRET_KEY", "")
On 2014/03/18 03:17:57, thumper wrote:
> these four should all be:
>   s.PatchEnvironment

Done.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/validatetoolsmetadata_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:10:
"github.com/juju/testing"
On 2014/03/18 03:17:57, thumper wrote:
> remove

Done.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:97:
s.AddCleanup(func(*gc.C) { restore() })
On 2014/03/18 03:17:57, thumper wrote:
> change these to:
> 
> s.PatchEnvironment
> 
> no restore, no add cleanup needed.

Done.

https://codereview.appspot.com/76670043/diff/20001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/validatetoolsmetadata_test.go:128:
testing.PatchEnvironment("AWS_SECRET_ACCESS_KEY", "")
On 2014/03/18 03:17:57, thumper wrote:
> ditto

Done.

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:96: defer testing.PatchValue(juju.ProviderConnectDelay,
coretesting.LongWait).Restore()
On 2014/03/18 03:17:57, thumper wrote:
> Add a variable to the reciever
>   func (s *NewAPIClientSuite)
> 
> And use s.PatchValue, no defer needed.

Done.

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:145: defer testing.PatchValue(juju.APIOpen,
apiOpen).Restore()
On 2014/03/18 03:17:57, thumper wrote:
> ditto and for the following

Done.

https://codereview.appspot.com/76670043/diff/20001/juju/apiconn_test.go#newco...
juju/apiconn_test.go:487: return stateClosed, testing.PatchValue(juju.APIClose,
apiClose)
Do I leave this line as it is?

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_linux_test.go
File juju/osenv/vars_linux_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_linux_test...
juju/osenv/vars_linux_test.go:12: testing.PatchEnvironment("HOME", h)
On 2014/03/18 03:17:57, thumper wrote:
> What is the import suite?
> 
> Can you just use PatchEnvironment from that?

Not unless I embed testing.CleanupSuite.

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_windows_te...
File juju/osenv/vars_windows_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/juju/osenv/vars_windows_te...
juju/osenv/vars_windows_test.go:14: testing.PatchEnvironment("HOMEDRIVE", "")
On 2014/03/18 03:17:57, thumper wrote:
> ditto for using the PatchEnvironment from the importSuite

Done. I've embed CleanupSuite in importSuite.

https://codereview.appspot.com/76670043/diff/20001/worker/instancepoller/mach...
File worker/instancepoller/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/instancepoller/mach...
worker/instancepoller/machine_test.go:49: defer testing.PatchValue(&ShortPoll,
coretesting.ShortWait/10).Restore()
On 2014/03/18 03:17:57, thumper wrote:
> all these can be changed to use the PatchValue from the suite

Done. I've also removed defer and Restore as this is now handled by PatchValue
internally.

https://codereview.appspot.com/76670043/diff/20001/worker/provisioner/contain...
File worker/provisioner/container_initialisation_test.go (right):

https://codereview.appspot.com/76670043/diff/20001/worker/provisioner/contain...
worker/provisioner/container_initialisation_test.go:54: aptCmdChan, cleanup :=
testing.HookCommandOutput(&utils.AptCommandOutput, []byte{}, nil)
On 2014/03/18 03:17:57, thumper wrote:
> should be able to use the HookCommandOutput from the suite.

Done.

[thumper, 2014-03-19 02:16:41]

A few more things to fix.

https://codereview.appspot.com/76670043/diff/40001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:121: defer restore()
Those two lines can just be:

  s.PatchValue(&version.Current, testVersion)

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:358: testing.PatchValue(&instancepoller.ShortPoll,
500*time.Millisecond)
You still need to have the restore called, but if you use the suite method it is
fine, so do this:

  s.PatchValue(&instancepoller.ShortPoll, 500*time.Millisecond)

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/main_test.go#new...
cmd/jujud/main_test.go:67: coretesting.MgoTestPackage(t)
I have to admit that this entire function looks like crack.

I don't expect you to fix it, but perhaps add a TODO refactoring needed into
real base suites.

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/package_test.go
File juju/osenv/package_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/package_test.go...
juju/osenv/package_test.go:20: testing.CleanupSuite
This suite doesn't really need it because it is doing hardly anything. Doesn't
hurt (much), but isn't really adding anything.

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/vars_linux_test.go
File juju/osenv/vars_linux_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/vars_linux_test...
juju/osenv/vars_linux_test.go:12: testing.PatchEnvironment("HOME", h)
should be:

func (s *importSuite) TestHomeLinux(c *gc.C) {
   h := "/home/foo/bar"
   s.PatchEnvironment("HOME", h)

otherwise the value isn't being reset.

https://codereview.appspot.com/76670043/diff/40001/worker/instancepoller/mach...
File worker/instancepoller/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/instancepoller/mach...
worker/instancepoller/machine_test.go:10: jc "github.com/juju/testing/checkers"
these imports should be in the middle block

https://codereview.appspot.com/76670043/diff/40001/worker/peergrouper/worker_...
File worker/peergrouper/worker_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:14: jujutesting
"launchpad.net/juju-core/juju/testing"
You don't need this prefix now.

https://codereview.appspot.com/76670043/diff/40001/worker/provisioner/contain...
File worker/provisioner/container_initialisation_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/provisioner/contain...
worker/provisioner/container_initialisation_test.go:10: "os/exec"
line 8 and 9 should have a middle block.

https://codereview.appspot.com/76670043/diff/40001/worker/rsyslog/rsyslog_tes...
File worker/rsyslog/rsyslog_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/rsyslog/rsyslog_tes...
worker/rsyslog/rsyslog_test.go:38: restore :=
testing.PatchValue(rsyslog.LookupUser, func(username string) (uid, gid int, err
error) {
This is the second or third time I have seen this.

We should perhaps add a

  PatchValueForSuite
and
  PatchEnvironmentForSuite

That adds the cleanup to the suite cleanup...  but this can be done later.

[waigani, 2014-03-19 04:18:52]

Please take a look.

https://codereview.appspot.com/76670043/diff/40001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:121: defer restore()
On 2014/03/19 02:16:42, thumper wrote:
> Those two lines can just be:
> 
>   s.PatchValue(&version.Current, testVersion)

Done. How did I miss that?

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:358: testing.PatchValue(&instancepoller.ShortPoll,
500*time.Millisecond)
On 2014/03/19 02:16:42, thumper wrote:
> You still need to have the restore called, but if you use the suite method it
is
> fine, so do this:
> 
>   s.PatchValue(&instancepoller.ShortPoll, 500*time.Millisecond)

Done. And this one? I must have been half asleep.

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/main_test.go
File cmd/jujud/main_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/cmd/jujud/main_test.go#new...
cmd/jujud/main_test.go:67: coretesting.MgoTestPackage(t)
On 2014/03/19 02:16:42, thumper wrote:
> I have to admit that this entire function looks like crack.
> 
> I don't expect you to fix it, but perhaps add a TODO refactoring needed into
> real base suites.

Done.

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/package_test.go
File juju/osenv/package_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/package_test.go...
juju/osenv/package_test.go:20: testing.CleanupSuite
On 2014/03/19 02:16:42, thumper wrote:
> This suite doesn't really need it because it is doing hardly anything. Doesn't
> hurt (much), but isn't really adding anything.

It enables importSuite.PatchValue etc. Does that justify embedding it, or shall
I change the calls back to testing.PatchValue?

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/vars_linux_test.go
File juju/osenv/vars_linux_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/juju/osenv/vars_linux_test...
juju/osenv/vars_linux_test.go:12: testing.PatchEnvironment("HOME", h)
On 2014/03/19 02:16:42, thumper wrote:
> should be:
> 
> func (s *importSuite) TestHomeLinux(c *gc.C) {
>    h := "/home/foo/bar"
>    s.PatchEnvironment("HOME", h)
> 
> otherwise the value isn't being reset.

Done. How did I miss all these? Note to self: don't propose code in the middle
of the night.

https://codereview.appspot.com/76670043/diff/40001/worker/instancepoller/mach...
File worker/instancepoller/machine_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/instancepoller/mach...
worker/instancepoller/machine_test.go:10: jc "github.com/juju/testing/checkers"
On 2014/03/19 02:16:42, thumper wrote:
> these imports should be in the middle block

Done.

https://codereview.appspot.com/76670043/diff/40001/worker/peergrouper/worker_...
File worker/peergrouper/worker_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/peergrouper/worker_...
worker/peergrouper/worker_test.go:14: jujutesting
"launchpad.net/juju-core/juju/testing"
On 2014/03/19 02:16:42, thumper wrote:
> You don't need this prefix now.

Done.

https://codereview.appspot.com/76670043/diff/40001/worker/provisioner/contain...
File worker/provisioner/container_initialisation_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/provisioner/contain...
worker/provisioner/container_initialisation_test.go:10: "os/exec"
On 2014/03/19 02:16:42, thumper wrote:
> line 8 and 9 should have a middle block.

Done.

https://codereview.appspot.com/76670043/diff/40001/worker/rsyslog/rsyslog_tes...
File worker/rsyslog/rsyslog_test.go (right):

https://codereview.appspot.com/76670043/diff/40001/worker/rsyslog/rsyslog_tes...
worker/rsyslog/rsyslog_test.go:38: restore :=
testing.PatchValue(rsyslog.LookupUser, func(username string) (uid, gid int, err
error) {
On 2014/03/19 02:16:42, thumper wrote:
> This is the second or third time I have seen this.
> 
> We should perhaps add a
> 
>   PatchValueForSuite
> and
>   PatchEnvironmentForSuite
> 
> That adds the cleanup to the suite cleanup...  but this can be done later.

Done. Added TODO.

[waigani, 2014-03-19 21:25:31]

Please take a look.

[thumper, 2014-03-19 21:36:36]

On 2014/03/19 21:25:31, waigani wrote:
> Please take a look.

LGTM