Make test charm repository available from testing package

Make test charm repository available from testing package

charm, state, and store were all testing against the same repo, and very
soon so will hook (so it can have a real unit to work with); seemed sensible
to make it hard for tests to accidentally write to charms they shouldn't,
and to avoid explicit use of "../charm/testrepo" in other packages.

Only notable change is file mode testing; it was actually only tested in one
place, and implicitly at that, so it's now explicitly tested in its own
test.

https://code.launchpad.net/~fwereade/juju/go-testing-charm/+merge/97956

(do not edit description out of merge proposal)

[fwereade, 2012-03-16 18:40:20]

Please take a look.

[fwereade, 2012-03-16 18:45:00]

Please take a look.

[niemeyer, 2012-03-16 19:25:13]

Great idea. A few ideas for improvements:

https://codereview.appspot.com/5845051/diff/12/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode11
testing/charm.go:11: var srcRepo =
os.ExpandEnv("$GOPATH/src/launchpad.net/juju/go/testing/repo")
$GOPATH may be set to something like "$HOME:/usr/local/gopath", so this isn't
safe. You can use the standard go/build package to find the right directory with
something like this:

  p, err := build.Import("launchpad.net/juju/go/testing", "", build.FindOnly)

p.Dir will have the directory.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode14
testing/charm.go:14: path string
This may be exposed. We'll have tests that need the repository path to verify
repo-handling behavior.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode20
testing/charm.go:20: c.Assert(err, IsNil)
The use of Assert and Fatal within the testing package feels a bit incorrect,
now that I think of it. Besides being a burden to be providing "c" to every
single function and method, this is wrong from a testing perspective. Assert is
a verification that behavior we're testing works as expected, but this and most
uses below of "c" are used in a context that is simply setting up the
environment for the actual test, and they should *never* fail, or it is a bug in
the harness, not in the tested logic. These are, to me, very legitimate
locations to use a panic.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode25
testing/charm.go:25: return &Repo{clone(c, srcRepo)}
It'd be nice to not have clone being the standard behavior, as most tests don't
actually mutate the charm. In fact, Repo might be a singleton variable in the
package (no c, taking in consideration the above):

    dummy := testing.Charms.Bundle("dummy")

[fwereade, 2012-03-16 21:22:14]

Great comments, thanks.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode11
testing/charm.go:11: var srcRepo =
os.ExpandEnv("$GOPATH/src/launchpad.net/juju/go/testing/repo")
On 2012/03/16 19:25:14, niemeyer wrote:
> $GOPATH may be set to something like "$HOME:/usr/local/gopath", so this isn't
> safe. You can use the standard go/build package to find the right directory
with
> something like this:
> 
>   p, err := build.Import("launchpad.net/juju/go/testing", "", build.FindOnly)
> 
> p.Dir will have the directory.

Awesome, thanks.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode20
testing/charm.go:20: c.Assert(err, IsNil)
On 2012/03/16 19:25:14, niemeyer wrote:
> The use of Assert and Fatal within the testing package feels a bit incorrect,
> now that I think of it. Besides being a burden to be providing "c" to every
> single function and method, this is wrong from a testing perspective. Assert
is
> a verification that behavior we're testing works as expected, but this and
most
> uses below of "c" are used in a context that is simply setting up the
> environment for the actual test, and they should *never* fail, or it is a bug
in
> the harness, not in the tested logic. These are, to me, very legitimate
> locations to use a panic. 

Yeah, I didn't really like the C everywhere, but I didn't think of panic :).

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode25
testing/charm.go:25: return &Repo{clone(c, srcRepo)}
On 2012/03/16 19:25:14, niemeyer wrote:
> It'd be nice to not have clone being the standard behavior, as most tests
don't
> actually mutate the charm. In fact, Repo might be a singleton variable in the
> package (no c, taking in consideration the above):
> 
>     dummy := testing.Charms.Bundle("dummy")

My concern is that tests could easily mutate the charms without the author
really thinking about it; but yeah, I can accept that's premature, seems like a
good idea to just go with the singleton until we actually see a problem.

[fwereade, 2012-03-16 22:20:59]

Please take a look.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode11
testing/charm.go:11: var srcRepo =
os.ExpandEnv("$GOPATH/src/launchpad.net/juju/go/testing/repo")
On 2012/03/16 21:22:14, fwereade wrote:
> On 2012/03/16 19:25:14, niemeyer wrote:
> > $GOPATH may be set to something like "$HOME:/usr/local/gopath", so this
isn't
> > safe. You can use the standard go/build package to find the right directory
> with
> > something like this:
> > 
> >   p, err := build.Import("launchpad.net/juju/go/testing", "",
build.FindOnly)
> > 
> > p.Dir will have the directory.
> 
> Awesome, thanks.

Done.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode14
testing/charm.go:14: path string
On 2012/03/16 19:25:14, niemeyer wrote:
> This may be exposed. We'll have tests that need the repository path to verify
> repo-handling behavior.

Done.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode20
testing/charm.go:20: c.Assert(err, IsNil)
On 2012/03/16 21:22:14, fwereade wrote:
> On 2012/03/16 19:25:14, niemeyer wrote:
> > The use of Assert and Fatal within the testing package feels a bit
incorrect,
> > now that I think of it. Besides being a burden to be providing "c" to every
> > single function and method, this is wrong from a testing perspective. Assert
> is
> > a verification that behavior we're testing works as expected, but this and
> most
> > uses below of "c" are used in a context that is simply setting up the
> > environment for the actual test, and they should *never* fail, or it is a
bug
> in
> > the harness, not in the tested logic. These are, to me, very legitimate
> > locations to use a panic. 
> 
> Yeah, I didn't really like the C everywhere, but I didn't think of panic :).

Done.

https://codereview.appspot.com/5845051/diff/12/testing/charm.go#newcode25
testing/charm.go:25: return &Repo{clone(c, srcRepo)}
On 2012/03/16 21:22:14, fwereade wrote:
> On 2012/03/16 19:25:14, niemeyer wrote:
> > It'd be nice to not have clone being the standard behavior, as most tests
> don't
> > actually mutate the charm. In fact, Repo might be a singleton variable in
the
> > package (no c, taking in consideration the above):
> > 
> >     dummy := testing.Charms.Bundle("dummy")
> 
> My concern is that tests could easily mutate the charms without the author
> really thinking about it; but yeah, I can accept that's premature, seems like
a
> good idea to just go with the singleton until we actually see a problem.

Done.

[fwereade, 2012-03-17 00:13:38]

Please take a look.

[rog, 2012-03-22 10:57:46]

LGTM bar lack of docs.

https://codereview.appspot.com/5845051/diff/3002/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/5845051/diff/3002/testing/charm.go#newcode23
testing/charm.go:23: type Repo struct {
everything exported in this file should have doc comments.

[fwereade, 2012-04-18 16:47:19]

Please take a look.

[fwereade, 2012-04-23 08:15:28]

Please take a look.

[fwereade, 2012-04-23 08:41:45]

Please take a look.

[rog, 2012-04-27 09:40:47]

On 2012/04/23 08:41:45, fwereade wrote:
> Please take a look.

LGTM

[niemeyer, 2012-04-27 14:50:51]

LGTM too.

https://codereview.appspot.com/5845051/diff/17001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/5845051/diff/17001/charm/bundle_test.go#newcode64
charm/bundle_test.go:64: func (s *BundleSuite) TestBundleFileModes(c *C) {
Took me a bit to figure where these tests had gone. This is nice, thanks.

[fwereade, 2012-04-27 15:08:43]

*** Submitted:

Make test charm repository available from testing package

charm, state, and store were all testing against the same repo, and very
soon so will hook (so it can have a real unit to work with); seemed sensible
to make it hard for tests to accidentally write to charms they shouldn't,
and to avoid explicit use of "../charm/testrepo" in other packages.

Only notable change is file mode testing; it was actually only tested in one
place, and implicitly at that, so it's now explicitly tested in its own
test.

R=niemeyer, rog
CC=
https://codereview.appspot.com/5845051