Code review comment for lp://staging/~rogpeppe/juju-core/562-lose-testbase

Revision history for this message
Francesco Banconi (frankban) wrote :

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/

« Back to merge proposal