Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1599)

Issue 6495086: testing: make charm API into a test fixture.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by rog
Modified:
10 years, 2 months ago
Reviewers:
gustavo, mp+122864, dfc, niemeyer, fwereade
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 19

Patch Set 2 : testing: make charm API into a test fixture. #

Patch Set 3 : testing: make charm API into a test fixture. #

Total comments: 7

Patch Set 4 : testing: make charm API into a test fixture. #

Patch Set 5 : testing: make charm API into a test fixture. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+234 lines, -208 lines) Patch
A [revision details] View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M charm/bundle_test.go View 1 5 chunks +8 lines, -7 lines 1 comment Download
M charm/charm_test.go View 1 1 chunk +9 lines, -3 lines 0 comments Download
M charm/config_test.go View 1 2 chunks +11 lines, -5 lines 0 comments Download
M charm/dir_test.go View 1 6 chunks +14 lines, -8 lines 0 comments Download
M charm/meta_test.go View 1 5 chunks +20 lines, -14 lines 0 comments Download
M charm/repo_test.go View 1 3 chunks +11 lines, -8 lines 0 comments Download
M cmd/juju/addunit_test.go View 1 2 chunks +1 line, -2 lines 0 comments Download
M cmd/juju/deploy_test.go View 1 8 chunks +12 lines, -19 lines 0 comments Download
M cmd/juju/expose_test.go View 1 2 chunks +1 line, -2 lines 0 comments Download
M cmd/juju/status_test.go View 1 2 3 4 9 chunks +28 lines, -30 lines 0 comments Download
M cmd/juju/unexpose_test.go View 1 2 chunks +1 line, -2 lines 0 comments Download
M juju/deploy_test.go View 1 12 chunks +22 lines, -30 lines 0 comments Download
M juju/testing/conn.go View 1 2 3 5 chunks +10 lines, -3 lines 0 comments Download
M mstate/conn_test.go View 1 3 chunks +3 lines, -1 line 0 comments Download
M mstate/state_test.go View 1 2 3 4 2 chunks +1 line, -2 lines 0 comments Download
M state/state_test.go View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M store/branch_test.go View 1 2 chunks +1 line, -2 lines 0 comments Download
M store/store_test.go View 1 3 chunks +4 lines, -1 line 0 comments Download
M testing/charm.go View 1 2 chunks +70 lines, -65 lines 1 comment Download
M worker/uniter/charm/charm_test.go View 1 1 chunk +1 line, -1 line 0 comments Download
M worker/uniter/uniter_test.go View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 12
niemeyer
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): ...
10 years, 3 months ago (2012-09-05 14:06:45 UTC) #1
rog
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 ...
10 years, 3 months ago (2012-09-05 15:12:14 UTC) #2
fwereade
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", ...
10 years, 3 months ago (2012-09-06 07:44:24 UTC) #3
rog
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 ...
10 years, 3 months ago (2012-09-06 15:05:05 UTC) #4
dfc
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 ...
10 years, 3 months ago (2012-09-11 04:59:15 UTC) #5
fwereade
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 ...
10 years, 3 months ago (2012-09-11 07:10:20 UTC) #6
fwereade
Whoops, looking at wrong version :/. LGTM.
10 years, 3 months ago (2012-09-11 09:13:27 UTC) #7
niemeyer
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 ...
10 years, 3 months ago (2012-09-11 19:45:05 UTC) #8
rog
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): > ...
10 years, 3 months ago (2012-09-12 08:10:23 UTC) #9
niemeyer
> I see your concern about the run time implications of forking > at all ...
10 years, 2 months ago (2012-09-12 18:36:10 UTC) #10
rog
On 12 September 2012 19:36, <n13m3y3r@gmail.com> wrote: >> I'm not sure what you mean by ...
10 years, 2 months ago (2012-09-12 19:15:19 UTC) #11
gustavo_niemeyer.net
10 years, 2 months ago (2012-09-12 19:54:27 UTC) #12
> 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b