testing/testbase: remove

testing/testbase: remove

It's unnecessary - we move FindJujuCoreImports to
testing (and make it call the less specific version in
github.com/juju/testing).

We temporarily lose the ability to change the logging level
in tests, but that will be reinstated directly in LoggingSuite - it's
not crucial behaviour.

Eventually most tests will use testing.IsolationSuite,
but in the meantime, add testing.CleanupSuite to
those tests that need it.

https://code.launchpad.net/~rogpeppe/juju-core/562-lose-testbase/+merge/221560

(do not edit description out of merge proposal)

[rog, 2014-05-30 15:01:18]

Please take a look.

[rog, 2014-05-30 15:34:47]

Please take a look.

[frankban, 2014-05-30 16:14:57]

Thanks for this branch Roger!
LGTM with some minor/questions below.

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go
File cmd/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go#newcode26
cmd/package_test.go:26: jc.DeepEquals,
So I presume github.com/juju/testing/checkers.DeepEquals is preferred over
launchpad.net/gocheck.DeepEquals.

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
cmd/supercommand_test.go:11: gitjujutesting "github.com/juju/testing"
We use just "github.com/juju/testing" and coretesting for
"launchpad.net/juju-core/testing" elsewhere. 
Anyway this makes the diff short and clean, so <shrug>.
Do we have conventions for import names?

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
cmd/supercommand_test.go:28: gitjujutesting.IsolationSuite
Cool.

https://codereview.appspot.com/99670045/diff/40001/testing/base.go
File testing/base.go (right):

https://codereview.appspot.com/99670045/diff/40001/testing/base.go#newcode33
testing/base.go:33: t.CleanupSuite.SetUpSuite(c)
In the Isolation suite we use the reversed order:
s.CleanupSuite.SetUpSuite(c)
s.LoggingSuite.SetUpSuite(c)
I don't think in this case it makes so much difference, but keeping them in sync
now can avoid some confusion in the future.
What do you think?
This also applies to sync_test.

https://codereview.appspot.com/99670045/diff/40001/testing/imports.go
File testing/imports.go (right):

https://codereview.appspot.com/99670045/diff/40001/testing/imports.go#newcode17
testing/imports.go:17: imps, err := testing.FindImports(packageName,
jujuPkgPrefix)
Nice.

https://codereview.appspot.com/99670045/diff/40001/utils/exec/package_test.go
File utils/exec/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/utils/exec/package_test.go...
utils/exec/package_test.go:7: stdtesting "testing"
This looks like an import error: stdtesting vs testing.

https://codereview.appspot.com/99670045/diff/40001/utils/fslock/package_test.go
File utils/fslock/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/utils/fslock/package_test....
utils/fslock/package_test.go:7: stdtesting "testing"
Ditto.

[rog, 2014-05-30 16:19:41]

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go
File cmd/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go#newcode26
cmd/package_test.go:26: jc.DeepEquals,
On 2014/05/30 16:14:56, frankban wrote:
> So I presume github.com/juju/testing/checkers.DeepEquals is preferred over
> launchpad.net/gocheck.DeepEquals.

It gives better diagnostics on failure.

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
cmd/supercommand_test.go:11: gitjujutesting "github.com/juju/testing"
On 2014/05/30 16:14:57, frankban wrote:
> We use just "github.com/juju/testing" and coretesting for
> "launchpad.net/juju-core/testing" elsewhere. 
> Anyway this makes the diff short and clean, so <shrug>.
> Do we have conventions for import names?

not really. i asked about this on juju-dev, and no-one seemed that bothered.

https://codereview.appspot.com/99670045/diff/40001/testing/base.go
File testing/base.go (right):

https://codereview.appspot.com/99670045/diff/40001/testing/base.go#newcode33
testing/base.go:33: t.CleanupSuite.SetUpSuite(c)
On 2014/05/30 16:14:57, frankban wrote:
> In the Isolation suite we use the reversed order:
> s.CleanupSuite.SetUpSuite(c)
> s.LoggingSuite.SetUpSuite(c)
> I don't think in this case it makes so much difference, but keeping them in
sync
> now can avoid some confusion in the future.
> What do you think?
> This also applies to sync_test.

sgtm, will do.

https://codereview.appspot.com/99670045/diff/40001/utils/exec/package_test.go
File utils/exec/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/utils/exec/package_test.go...
utils/exec/package_test.go:7: stdtesting "testing"
On 2014/05/30 16:14:57, frankban wrote:
> This looks like an import error: stdtesting vs testing.

yup, i haven't resolved all the trunk merging issues. will fix.

[wwitzel3, 2014-05-30 16:30:53]

My feeling is, I don't care where and how things are aliased, but it should be
consistent so that when I am looking at a file and I see "coretesting" and the
next file I see "testing" .. I shouldn't have to think that might be
juju-core/testing, just sans alias.

Also I'm -1 with respect to alias of the standard lib testing to stdtesting.
That just inserts unneeded convention in to our code. We can easily alias our
other testing packages and not have to remember that stdtesting is really just
testing.

I think we should try to use the aliases coretesting, gitjujutesting,
statetesting, jujutesting, etc.. even if they aren't conflicting with any other
imported packages. That consistency will make our code a whole lot easier to
follow IMO.

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go
File cmd/package_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go#newcode7
cmd/package_test.go:7: stdtesting "testing"
Why alias the standard library testing package?

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
cmd/supercommand_test.go:16: "launchpad.net/juju-core/testing"
Why not alias this to coretesting?

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:15: "testing"
Same vein. Here we don't alias to stdtesting.

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:32: coretesting "launchpad.net/juju-core/testing"
Again, here we do alias to coretesting.

[wwitzel3, 2014-05-30 16:31:46]

On 2014/05/30 16:30:53, wwitzel3 wrote:
> My feeling is, I don't care where and how things are aliased, but it should be
> consistent so that when I am looking at a file and I see "coretesting" and the
> next file I see "testing" .. I shouldn't have to think that might be
> juju-core/testing, just sans alias.
> 
> Also I'm -1 with respect to alias of the standard lib testing to stdtesting.
> That just inserts unneeded convention in to our code. We can easily alias our
> other testing packages and not have to remember that stdtesting is really just
> testing.
> 
> I think we should try to use the aliases coretesting, gitjujutesting,
> statetesting, jujutesting, etc.. even if they aren't conflicting with any
other
> imported packages. That consistency will make our code a whole lot easier to
> follow IMO.
> 
> https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go
> File cmd/package_test.go (right):
> 
>
https://codereview.appspot.com/99670045/diff/40001/cmd/package_test.go#newcode7
> cmd/package_test.go:7: stdtesting "testing"
> Why alias the standard library testing package?
> 
> https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
> File cmd/supercommand_test.go (right):
> 
>
https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
> cmd/supercommand_test.go:16: "launchpad.net/juju-core/testing"
> Why not alias this to coretesting?
> 
> https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go
> File environs/sync/sync_test.go (right):
> 
>
https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go...
> environs/sync/sync_test.go:15: "testing"
> Same vein. Here we don't alias to stdtesting.
> 
>
https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go...
> environs/sync/sync_test.go:32: coretesting "launchpad.net/juju-core/testing"
> Again, here we do alias to coretesting.

And by "how" I meant, naming wise.

[rog, 2014-06-02 13:46:01]

On 30 May 2014 17:30,  <wayne.witzel@canonical.com> wrote:
> My feeling is, I don't care where and how things are aliased, but it
> should be consistent so that when I am looking at a file and I see
> "coretesting" and the next file I see "testing" .. I shouldn't have to
> think that might be juju-core/testing, just sans alias.
>
> Also I'm -1 with respect to alias of the standard lib testing to
> stdtesting. That just inserts unneeded convention in to our code. We can
> easily alias our other testing packages and not have to remember that
> stdtesting is really just testing.
>
> I think we should try to use the aliases coretesting, gitjujutesting,
> statetesting, jujutesting, etc.. even if they aren't conflicting with
> any other imported packages. That consistency will make our code a whole
> lot easier to follow IMO.

Perhaps you're right, but that's not been the convention up until now.
If we do decide to do that, perhaps we should just rename those packages so
we don't need the identifier.

[rog, 2014-06-02 13:47:24]

Please take a look.

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go
File cmd/supercommand_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/cmd/supercommand_test.go#n...
cmd/supercommand_test.go:16: "launchpad.net/juju-core/testing"
On 2014/05/30 16:30:52, wwitzel3 wrote:
> Why not alias this to coretesting?

The original rationale was that juju-core/testing feels "native" so earns the
right to be unaliased when reasonable. I've left it that way here to save churn,
although it could easily be coretesting too.

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/99670045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:15: "testing"
On 2014/05/30 16:30:52, wwitzel3 wrote:
> Same vein. Here we don't alias to stdtesting.

Yeah, conventionally we would alias to stdtesting here
(after all, the identifier is only used once)

https://codereview.appspot.com/99670045/diff/40001/testing/base.go
File testing/base.go (right):

https://codereview.appspot.com/99670045/diff/40001/testing/base.go#newcode33
testing/base.go:33: t.CleanupSuite.SetUpSuite(c)
On 2014/05/30 16:19:40, rog wrote:
> On 2014/05/30 16:14:57, frankban wrote:
> > In the Isolation suite we use the reversed order:
> > s.CleanupSuite.SetUpSuite(c)
> > s.LoggingSuite.SetUpSuite(c)
> > I don't think in this case it makes so much difference, but keeping them in
> sync
> > now can avoid some confusion in the future.
> > What do you think?
> > This also applies to sync_test.
> 
> sgtm, will do.

Done.