Merge lp://staging/~rogpeppe/juju-core/562-lose-testbase into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+221560_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/99670045/

Affected files (+59, -189 lines):
   A [revision details]
   M agent/tools/tools_test.go
   M cmd/args_test.go
   M cmd/package_test.go
   M cmd/supercommand_test.go
   M dependencies.tsv
   M environs/sync/sync_test.go
   M juju/osenv/package_test.go
   M state/api/rsyslog/rsyslog_test.go
   M testing/base.go
   M testing/imports.go
   D testing/testbase/log.go
   D testing/testbase/log_test.go
   D testing/testbase/package_test.go
   M utils/exec/package_test.go
   M utils/fslock/package_test.go
   M utils/home_windows_test.go
   M utils/package_test.go

Please take a look.

https://codereview.appspot.com/99670045/

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#newcode11
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#newcode28
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#newcode7
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.go#newcode7
utils/fslock/package_test.go:7: stdtesting "testing"
Ditto.

https://codereview.appspot.com/99670045/

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#newcode11
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#newcode7
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.

https://codereview.appspot.com/99670045/

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#newcode16
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#newcode15
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#newcode32
environs/sync/sync_test.go:32: coretesting
"launchpad.net/juju-core/testing"
Again, here we do alias to coretesting.

https://codereview.appspot.com/99670045/

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#newcode16
> 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#newcode15
> 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#newcode32
> 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.

https://codereview.appspot.com/99670045/

I'm pretty sure Tim makes active use of the "run the test suite with a different log level", though I think he's the only one that I know of that does.

It would be good to at least keep him up to date on the change and where it is headed.

I do agree with the idea from William that it will be easier to understand test suites if we use common naming for the actual test packages. Though it really starts to sound like we should just be naming the packages like that, rather than forcing everyone to do the aliasing.

Especially "testing" because it is reasonably likely that you might need more than one of those in a new test suite.

On 30 May 2014 17:30, <email address hidden> 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.

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#newcode16
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#newcode15
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.

https://codereview.appspot.com/99670045/

The attempt to merge lp:~rogpeppe/juju-core/562-lose-testbase into lp:juju-core failed. Below is the output from the failed tests.

godeps: cannot update "/home/tarmac/trees/src/github.com/juju/testing": fatal: reference is not a tree: 77f66ce9a8a203fd6af74b38fb207666d5cdeae6
mongod: no process found