testing: make charm API into a test fixture.

testing: make charm API into a test fixture.

Some advantages of the new scheme:

- some code becomes simpler.
- the API is smaller and arguably easier to use.
- we run no risk of modifying the original charms,
because there's no longer any distinction between
cloned charms and uncloned charms (all charms
are cloned on demand).
- it's easy to get a charm for a particular series
(something we want when live-deploying charms)

https://code.launchpad.net/~rogpeppe/juju-core/052-testing-charm-suite/+merge/122864

(do not edit description out of merge proposal)

[niemeyer, 2012-09-05 14:06:45]

Looks like a good direction, but there are a few details:

https://codereview.appspot.com/6495086/diff/1/charm/config_test.go
File charm/config_test.go (right):

https://codereview.appspot.com/6495086/diff/1/charm/config_test.go#newcode53
charm/config_test.go:53: type ConfigSuite struct {
It's nice to have the type definition above its methods.

https://codereview.appspot.com/6495086/diff/1/charm/meta_test.go
File charm/meta_test.go (right):

https://codereview.appspot.com/6495086/diff/1/charm/meta_test.go#newcode28
charm/meta_test.go:28: type MetaSuite struct {
Ditto.

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go
File juju/deploy_test.go (right):

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go#newcode65
juju/deploy_test.go:65: s.CharmDir("series", "riak")
This shouldn't be necessary. See the comment in the suite.

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go#newcode70
juju/deploy_test.go:70: s.CharmDir("series", "riak")
Ditto.

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go#newcode83
juju/deploy_test.go:83: s.CharmBundle("series", "riak")
There's a bug here. Please make sure this test is actually testing what it
intends to test (making it fail). The bundle lands somewhere else, and the test
below isn't using it.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode48
testing/charm.go:48: func (s *CharmSuite) Reset(c *C) {
s/Reset/ResetCharmRepo/

Out of context it will be vague to say "foo.Reset()". We should rename the Reset
in the other suite we have too.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode52
testing/charm.go:52: // CharmDir creates a charm directory holding a copy of
// CharmDir returns the charm.Dir with the requested testing
// charm. If the charm directory doesn't yet exist in the
// temporary repository, it will be copied from the pristine
// testing repository.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode61
testing/charm.go:61: _, err = os.Stat(path)
if os.IsNotExist(err) {
    s.ensureSeries(...)
    ...
} else {
    check(err)    
}

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode84
testing/charm.go:84: func (s *CharmSuite) CharmURL(series, name string)
*charm.URL {
Wouldn't it be sensible to call CharmDir here, so we don't have to artificially
call CharmDir just for the side-effect in tests?

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode97
testing/charm.go:97: file, err := ioutil.TempFile(s.ensureSeries(series), name)
This will mean we'll have to remember to clean that up every time. It needs to
do something like we used to do instead, to avoid having files left around.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode105
testing/charm.go:105: _, err = charm.ReadBundle(path)
Why? If you want to ensure it works, write a test for it, but it doesn't seem to
make sense to be testing it in every use case.

[rog, 2012-09-05 15:12:14]

Please take a look.

https://codereview.appspot.com/6495086/diff/1/charm/config_test.go
File charm/config_test.go (right):

https://codereview.appspot.com/6495086/diff/1/charm/config_test.go#newcode53
charm/config_test.go:53: type ConfigSuite struct {
On 2012/09/05 14:06:45, niemeyer wrote:
> It's nice to have the type definition above its methods.

Done.

https://codereview.appspot.com/6495086/diff/1/charm/meta_test.go
File charm/meta_test.go (right):

https://codereview.appspot.com/6495086/diff/1/charm/meta_test.go#newcode28
charm/meta_test.go:28: type MetaSuite struct {
On 2012/09/05 14:06:45, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go
File juju/deploy_test.go (right):

https://codereview.appspot.com/6495086/diff/1/juju/deploy_test.go#newcode83
juju/deploy_test.go:83: s.CharmBundle("series", "riak")
On 2012/09/05 14:06:45, niemeyer wrote:
> There's a bug here. Please make sure this test is actually testing what it
> intends to test (making it fail). The bundle lands somewhere else, and the
test
> below isn't using it.

actually, there's no bug. CharmBundle puts the charm as a bundle into the
repository, then it's found with the usual search mechanism. removing the above
line makes the test fail ("no charms found matching...").

this is the reason we don't make CharmURL automatically put the directory.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode52
testing/charm.go:52: // CharmDir creates a charm directory holding a copy of
On 2012/09/05 14:06:45, niemeyer wrote:
> // CharmDir returns the charm.Dir with the requested testing
> // charm. If the charm directory doesn't yet exist in the
> // temporary repository, it will be copied from the pristine
> // testing repository.

Done.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode61
testing/charm.go:61: _, err = os.Stat(path)
On 2012/09/05 14:06:45, niemeyer wrote:
> if os.IsNotExist(err) {
>     s.ensureSeries(...)
>     ...
> } else {
>     check(err)    
> }

Done.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode84
testing/charm.go:84: func (s *CharmSuite) CharmURL(series, name string)
*charm.URL {
On 2012/09/05 14:06:45, niemeyer wrote:
> Wouldn't it be sensible to call CharmDir here, so we don't have to
artificially
> call CharmDir just for the side-effect in tests?

see comment in other file. we can easily have URLs that refer to bundles too,
and we want that decision to be up to the caller.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode97
testing/charm.go:97: file, err := ioutil.TempFile(s.ensureSeries(series), name)
On 2012/09/05 14:06:45, niemeyer wrote:
> This will mean we'll have to remember to clean that up every time. It needs to
> do something like we used to do instead, to avoid having files left around.

note the first argument to TempFile.
i've changed the comment to be hopefully clearer.

https://codereview.appspot.com/6495086/diff/1/testing/charm.go#newcode105
testing/charm.go:105: _, err = charm.ReadBundle(path)
On 2012/09/05 14:06:45, niemeyer wrote:
> Why? If you want to ensure it works, write a test for it, but it doesn't seem
to
> make sense to be testing it in every use case.

Done.

[fwereade, 2012-09-06 07:44:24]

LGTM with uniter test concerns addressed

https://codereview.appspot.com/6495086/diff/4002/juju/deploy_test.go
File juju/deploy_test.go (right):

https://codereview.appspot.com/6495086/diff/4002/juju/deploy_test.go#newcode72
juju/deploy_test.go:72: curl := s.repo.URL("series", "riak")
Oh, much nicer :).

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go#newcode19
juju/testing/conn.go:19: // test.  It also includes testing.LoggingSuite and
testing.CharmSuite.
fix comment

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:298: base := ctx.s.Repo.Dir("dummy").Path
Hmmmm. I think I need a fresh clone here every time... it may not yet be called
more than once a test, but branches in review are doing so. Probably
best/easiest to just create a fresh Repo here every time.

[rog, 2012-09-06 15:05:05]

Please take a look.

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go#newcode19
juju/testing/conn.go:19: // test.  It also includes testing.LoggingSuite and
testing.CharmSuite.
On 2012/09/06 07:44:24, fwereade wrote:
> fix comment

Done.

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:298: base := ctx.s.Repo.Dir("dummy").Path
On 2012/09/06 07:44:24, fwereade wrote:
> Hmmmm. I think I need a fresh clone here every time... it may not yet be
called
> more than once a test, but branches in review are doing so. Probably
> best/easiest to just create a fresh Repo here every time.

Done.

[dave_cheney.net, 2012-09-11 04:59:15]

LGTM. Less imports, more concise, win.

https://codereview.appspot.com/6495086/diff/4004/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/6495086/diff/4004/charm/bundle_test.go#newcode67
charm/bundle_test.go:67: srcPath := s.repo.Dir("dummy").Path
Much nicer.

[fwereade, 2012-09-11 07:10:20]

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6495086/diff/4002/juju/testing/conn.go#newcode19
juju/testing/conn.go:19: // test.  It also includes testing.LoggingSuite and
testing.CharmSuite.
On 2012/09/06 15:05:06, rog wrote:
> On 2012/09/06 07:44:24, fwereade wrote:
> > fix comment
> 
> Done.

Sorry, I should have been more explicit... I see no CharmSuite in the code, so
it should probably not be in the comment.

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6495086/diff/4002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:298: base := ctx.s.Repo.Dir("dummy").Path
On 2012/09/06 15:05:06, rog wrote:
> On 2012/09/06 07:44:24, fwereade wrote:
> > Hmmmm. I think I need a fresh clone here every time... it may not yet be
> called
> > more than once a test, but branches in review are doing so. Probably
> > best/easiest to just create a fresh Repo here every time.
> 
> Done.

Sorry, not done, AFAICT. Two createCharms, called immediately after one another,
must have independent directories -- as it is ISTM that they will use the same
dir. Did I miss something?

[fwereade, 2012-09-11 09:13:27]

Whoops, looking at wrong version :/. LGTM.

[niemeyer, 2012-09-11 19:45:05]

https://codereview.appspot.com/6495086/diff/4004/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/6495086/diff/4004/testing/charm.go#newcode55
testing/charm.go:55: check(exec.Command("cp", "-r", unclonedDir.Path,
path).Run())
We had an interface that would read charms out of the testing directory
directly, and that needed no local variables for that, including no field in the
suite, no need to initialize a variable, and no SetUpTest or anything. We're
replacing this with one that forks out at all times and copies things it doesn't
have to, needs implicit initialization, and is error-prone (if you initialize
Path in SetUpSuite, tests will share the repository, potentially with changes in
it).

It doesn't feel like this is improving things, and if I understand the problem
you have it also feels quite unrelated to the real issue you're trying to solve.

Can't you solve the real issue you have, and even improve the interface a bit on
the way if you feel you know how to do that, but without changing the whole
mechanism that is working well today?

I'm getting increasingly concerned that we're spinning wheels too often without
actual progress.

[rog, 2012-09-12 08:10:23]

On 11 September 2012 20:45,  <n13m3y3r@gmail.com> wrote:
>
> https://codereview.appspot.com/6495086/diff/4004/testing/charm.go
> File testing/charm.go (right):
>
> https://codereview.appspot.com/6495086/diff/4004/testing/charm.go#newcode55
> testing/charm.go:55: check(exec.Command("cp", "-r", unclonedDir.Path,
> path).Run())
> We had an interface that would read charms out of the testing directory
> directly, and that needed no local variables for that, including no
> field in the suite, no need to initialize a variable, and no SetUpTest
> or anything. We're replacing this with one that forks out at all times
> and copies things it doesn't have to, needs implicit initialization, and
> is error-prone (if you initialize Path in SetUpSuite, tests will share
> the repository, potentially with changes in it).

I'm not sure I entirely understand the objections here.

I see your concern about the run time implications of forking
at all times, but the performance impact is unmeasurable
(for instance the charm package, which is one of the
heavier users of DirPath with one of the lowest overall test
times, runs its tests in exactly the same time on the new version).

I'm not sure what you mean by "needs implicit initialization".

I'm not sure what you mean by the last point either. A Repo
is just a directory containing zero or more copied charms - you can
use it as you like. I think it's actually less error-prone than
the current version, where Charms.Dir returns the actual directory
in the source tree, making it possible for a test to modify this
in place.

You were positive about the first version of this CL, and I'm convinced
this is an improvement on that. It simplifies all the code IMHO
and is an easy model to understand and work with. It also
gives me the functionality I needed without making anything
more complex.

[niemeyer, 2012-09-12 18:36:10]

> I see your concern about the run time implications of forking
> at all times, but the performance impact is unmeasurable

Saying it's not so much worse isn't a good reason to change it.

> I'm not sure what you mean by "needs implicit initialization".

repo.Path = c.MkDir()

> I'm not sure what you mean by the last point either. A Repo
> is just a directory containing zero or more copied charms - you can
> use it as you like. I think it's actually less error-prone than

The line above, if executed in a SetUpSuite, will create a repository that is
shared for all tests in that specific suite, no matter how much they hack the
directory, spoiling the reason why it's being copied.

> You were positive about the first version of this CL, and I'm convinced
> this is an improvement on that. It simplifies all the code IMHO

Exactly. Not only this isn't an improvement, but we're spinning our precious
wheels onto changes that contribute, if at all, very little to the project. I'd
really like to be reviewing changes that move us close to our goals for a
moment, instead of bikesheding on the interface of a test suite that works fine
today.

[rog, 2012-09-12 19:15:19]

On 12 September 2012 19:36,  <n13m3y3r@gmail.com> wrote:
>> I'm not sure what you mean by "needs implicit initialization".
>
>
> repo.Path = c.MkDir()
>
>
>> I'm not sure what you mean by the last point either. A Repo
>> is just a directory containing zero or more copied charms - you can
>> use it as you like. I think it's actually less error-prone than
>
>
> The line above, if executed in a SetUpSuite, will create a repository
> that is shared for all tests in that specific suite, no matter how much
> they hack the directory, spoiling the reason why it's being copied.

I presume you still wouldn't like it if suites held a *Repo
instead of a Repo and the initialisation was s.repo = &Repo{c.MkDir()} ?

Not that I really see any difference - if you're initialising something
in SetUpSuite when you should be doing so in SetUpTest, that's
just an error. Not a hard one to guard against though IMHO.

>> You were positive about the first version of this CL, and I'm
>
> convinced
>>
>> this is an improvement on that. It simplifies all the code IMHO
>
>
> Exactly. Not only this isn't an improvement, but we're spinning our
> precious wheels onto changes that contribute, if at all, very little to
> the project. I'd really like to be reviewing changes that move us close
> to our goals for a moment, instead of bikesheding on the interface of a
> test suite that works fine today.

Fair enough. I was convinced there was enough benefit, particularly
given your earlier encouraging remark, but it seems I was wrong.

I still need to be able to fetch charms with arbitrary series
(the original motivation behind this change).

Two options with the current interface - either I add a series
argument to all the entry points, or I add "WithSeries" variants
of all the functions. Any preference here before I go off and do the
wrong thing?

[gustavo_niemeyer.net, 2012-09-12 19:54:27]

> I presume you still wouldn't like it if suites held a *Repo
> instead of a Repo and the initialisation was s.repo = &Repo{c.MkDir()} ?
>
> Not that I really see any difference - if you're initialising something
> in SetUpSuite when you should be doing so in SetUpTest, that's
> just an error. Not a hard one to guard against though IMHO.

That's still arguing that it's easy to guard against a problem that we
don't have today. We don't *have* that problem, not even the one that
is easy to guard against.

> Two options with the current interface - either I add a series
> argument to all the entry points, or I add "WithSeries" variants
> of all the functions. Any preference here before I go off and do the
> wrong thing?

If you need a series argument, add a series argument. That sounds like
a trivial.


gustavo @ http://niemeyer.net