Fix 1307241. Isolate jujud.

Fix 1307241. Isolate jujud.

Add the following mock functions to
environs/tools/testing/testing.go:

GetMockUploadTools
GetMockBundleTools
GetMockBuildTools

Make envtools.BundleTools mockable.
Patch functions in tests where
needed to ensure tests pass without
dependencies on jujud.

https://code.launchpad.net/~waigani/juju-core/1307241-isolate-from-jujud/+merge/215590

(do not edit description out of merge proposal)

[waigani, 2014-04-15 09:15:27]

Please take a look.

[dave_cheney.net, 2014-04-15 10:10:15]

On 2014/04/15 09:15:27, waigani wrote:
> Please take a look.

I'm not sure I understand the rational for this change. If jujud fails to
compile, maybe that is where the test should stop.

[wallyworld, 2014-04-15 13:39:44]

On 2014/04/15 10:10:15, dfc wrote:
> On 2014/04/15 09:15:27, waigani wrote:
> > Please take a look.
> 
> I'm not sure I understand the rational for this change. If jujud fails to
> compile, maybe that is where the test should stop.

We want to mock out the BundleTools because it adds unnecessary complexity to
the tests which depend on it. eg when we test that tools are synced, we don't
care that the real compile is not done; we just want a stub which runs quickly
and is robust against whatever arch/series/os it is running on. Having said
that, there need to be tests added which confirm that BundleTools itself works
properly.

[wallyworld, 2014-04-15 13:42:05]

Looking good so far. I like that duped functionality has been consolidated.
If we are mocking out BundleTools, we need tests for BundleTools itself to
ensure that function does what it is supposed to do. We also need to ensure (if
not done already) that the tests which use the mock assert that the mock is
called correctly.

https://codereview.appspot.com/87130045/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/87130045/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:393: ttesting.ToolsDir = c.MkDir()
Don't do this - make toolsDir an argument to GetMockBuildTools and pass in
s.toolsDir

https://codereview.appspot.com/87130045/diff/20001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/20001/environs/tools/testing/tes...
environs/tools/testing/testing.go:34: var ToolsDir string
remove this and make it an arg to the functions which need it

[waigani, 2014-04-30 22:49:48]

Please take a look.

[fwereade, 2014-05-07 12:52:06]

I don't think this is quite there yet. Definitely some good stuff, but we should
chat.

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap_test.go:24: ttesting
"launchpad.net/juju-core/environs/tools/testing"
explicit "toolstesting" throughout please

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:275: os.Setenv("GOPATH", gopath)
do we *have* to mess with the real environment? I accept that *perhaps* we do,
but, eww.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:280: os.Setenv("GOPATH", os.Getenv("GOPATH"))
This is wrong. You need to capture the result of Getenv at setup time -- sadly,
you can't just drop the defer and move the code and actually have it work the
same ;).

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:440: // c.Assert(version{}, gc.IsNil)
c.Assert(vers, gc.DeepEquals, version.Binary{}) ?

https://codereview.appspot.com/87130045/diff/40001/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/tools/build.go#ne...
environs/tools/build.go:201: // BundleToolsFunc is a function which can bundles
all the current juju tools
s/bundles/bundle/

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:34: // mockUploadTools simulates the effect of
tools.Upload, but skips the time-
GetMockUploadTools returns an UploadFunc that...

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:37: // exposed and can itself be neatly
mocked?
I feel like fixing *this* TODO might have been a step in a slightly better
direction, but I'm not totally sure at this point.

edit: I'm getting surer. I'll be around later tonight, let's chat live -- it's
been a while since I saw this code, and I might be missing something, but I
think there's something funky about the layering here.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:49: versions = append(versions, newVers)
Don't we have a SeriesToUpload func somewhere? Shouldn't we be using that,
rather copy/pasting its functionality? (or, better, mocking a slightly different
layer such that the *only* thing we patch out is the costly build operation --
ISTM that all the rest of the time we really just want to patch out the
building, because we can cheaply upload to dummy-provider storage anyway.
Right?)

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:62: toolsDir = filepath.Join(c.MkDir(),
"jujud")
This seems strange to me... AFAICS the only client passes in "", and the
replacement "jujud" directory is always empty anyway, so can't we just: drop the
dir param, skip the Archive call, write nothing, return the hash of the empty
file, and be done?

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:76: // getMockBuildTools returns a
sync.BuildToolsTarballFunc implementation which generates
s/getM/GetM/

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:82: stor, err :=
filestorage.NewFileStorageWriter(toolsDir)
OK, I see what you're doing, but... this is so *not* about uploading anything.
If there's useful functionality in UploadFakeToolsVersions, great; but please
*extract* it and use the new method from both locations.

This is just adding a layer of cruft, and makes me a sad panda.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:89: uploadedTools, err :=
testing.UploadFakeToolsVersions(stor, versions...)
fwiw, the versions var STM to do nothing but obscure the fact that we're just
uploading one version.

[waigani, 2014-05-13 07:13:11]

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr...
File environs/bootstrap/bootstrap_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/bootstrap/bootstr...
environs/bootstrap/bootstrap_test.go:24: ttesting
"launchpad.net/juju-core/environs/tools/testing"
On 2014/05/07 12:52:05, fwereade wrote:
> explicit "toolstesting" throughout please

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:275: os.Setenv("GOPATH", gopath)
On 2014/05/07 12:52:06, fwereade wrote:
> do we *have* to mess with the real environment? I accept that *perhaps* we do,
> but, eww.

Done. Using s.PatchEnvPathPrepend.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:280: os.Setenv("GOPATH", os.Getenv("GOPATH"))
On 2014/05/07 12:52:05, fwereade wrote:
> This is wrong. You need to capture the result of Getenv at setup time --
sadly,
> you can't just drop the defer and move the code and actually have it work the
> same ;).

Done. I've mocked go cmd directly.

https://codereview.appspot.com/87130045/diff/40001/environs/sync/sync_test.go...
environs/sync/sync_test.go:440: // c.Assert(version{}, gc.IsNil)
On 2014/05/07 12:52:06, fwereade wrote:
> c.Assert(vers, gc.DeepEquals, version.Binary{}) ?

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:37: // exposed and can itself be neatly
mocked?
On 2014/05/07 12:52:06, fwereade wrote:
> I feel like fixing *this* TODO might have been a step in a slightly better
> direction, but I'm not totally sure at this point.
> 
> edit: I'm getting surer. I'll be around later tonight, let's chat live -- it's
> been a while since I saw this code, and I might be missing something, but I
> think there's something funky about the layering here.

UploadTools was being mocked in several places in the tests before my cl. Each
place reproduced the same functionality, so I created this function clean things
up.

As such, shouldn't removing UploadTools be done in a separate branch? If so, for
now shouldn't we leave this function as it at least cleans up all the stray mock
UploadTools funcs and eases making UploadTools unmockable?

What are you referring to when you talk about build logic in agent/tools?

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:62: toolsDir = filepath.Join(c.MkDir(),
"jujud")
On 2014/05/07 12:52:06, fwereade wrote:
> This seems strange to me... AFAICS the only client passes in "", and the
> replacement "jujud" directory is always empty anyway, so can't we just: drop
the
> dir param, skip the Archive call, write nothing, return the hash of the empty
> file, and be done?

Done.

https://codereview.appspot.com/87130045/diff/40001/environs/tools/testing/tes...
environs/tools/testing/testing.go:76: // getMockBuildTools returns a
sync.BuildToolsTarballFunc implementation which generates
On 2014/05/07 12:52:06, fwereade wrote:
> s/getM/GetM/

Done.

[waigani, 2014-05-14 02:49:37]

Please take a look.

[waigani, 2014-05-14 02:52:41]

On 2014/05/14 02:49:37, waigani wrote:
> Please take a look.

Please see my comments here: https://codereview.appspot.com/87130045/#msg7

[fwereade, 2014-05-15 08:57:20]

ISTM that we don't use GetMockUploadTools all *that* much, and it'd be worth
dropping it (and just patching out the building/bundling, so we get fake tools
put in the right place, in the right way). As it is I think GMUT is a liability,
and it needs to be dropped either in this CL or a followup (but a followup
*soon* -- I really don't want it hanging around).

Please run this by wallyworld again, as well: he's much more au fait with the
various tricky little details around here, but if he's happy I will be too
(modulo my various comments here).

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:262: type badBuildSuite struct {
btw, please don't just insert this test suite in between the definition and the
methods of the uploadSuite

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:273: // Create bad Go source file
This isn't accurate any more

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:288: c.Assert(err, gc.IsNil)
blank lines before comments in general, please

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "")
I'm not sure you *really* need this bit, but equally you don't have to take it
out :).

The super-sweet spot would be to pull all the MakeTool-style functionality out
into its own helpers and use all that same (tested) test-support code elsewhere
-- but if you want to do that, it'd deserve its own CL.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve
test speed.
It's not 100% clear what this test is meant to be doing, indeed. Given what we
have elsewhere I think I'd be testing this stuff by mocking the build/bundle
stuff, and directly checking what gets uploaded.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:452: c.Assert(err, gc.ErrorMatches, `build command
"go" failed: exit status 1; `)
please break all these cases into separate test methods where possible

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c
*gc.C) {
The SUT here is BuildToolsTarball, isn't it? We're testing that it calls
BundleTools in the expected way.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:483: writer       io.Writer
writer seems unused -- I'd expect us to write something to it and check that it
was somehow reflected in the result of BuildToolsTarball...

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:493: sync.BuildToolsTarball(&version.Current.Number)
error/result check?

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:52: }
I'm still nervous that this is a parallel implementation of logic in uploadTools
itself that *will* invitably change out of sync with this implementation. This
is basically how mocking ends up going wrong IMO...

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:69: 
not sure we need the blank lines here though :)

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:90: stor, err :=
filestorage.NewFileStorageWriter(toolsDir)
I still don't think we need to mess around with storage stuff. Can't we just
write direct to a file in a directory? What do we get from the extra layer of
complexity here?

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:281: // UploadToStorage uploads tools and
metadata for the specified versions to dir.
FWIW this is UploadToDirectory

[waigani, 2014-05-16 03:06:08]

Please take a look.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:262: type badBuildSuite struct {
On 2014/05/15 08:57:20, fwereade wrote:
> btw, please don't just insert this test suite in between the definition and
the
> methods of the uploadSuite

Ah right. The suite goes before the methods for the suite. Got it.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:273: // Create bad Go source file
On 2014/05/15 08:57:20, fwereade wrote:
> This isn't accurate any more

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:288: c.Assert(err, gc.IsNil)
On 2014/05/15 08:57:20, fwereade wrote:
> blank lines before comments in general, please

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "")
On 2014/05/15 08:57:19, fwereade wrote:
> I'm not sure you *really* need this bit, but equally you don't have to take it
> out :).
> 
> The super-sweet spot would be to pull all the MakeTool-style functionality out
> into its own helpers and use all that same (tested) test-support code
elsewhere
> -- but if you want to do that, it'd deserve its own CL.

Do you mean a helper suite i.e. FakeToolsSuite or a helper package like utils?

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve
test speed.
On 2014/05/15 08:57:19, fwereade wrote:
> It's not 100% clear what this test is meant to be doing, indeed. Given what we
> have elsewhere I think I'd be testing this stuff by mocking the build/bundle
> stuff, and directly checking what gets uploaded.

Done. Is TestUploadToolsBadBuild enough?

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:452: c.Assert(err, gc.ErrorMatches, `build command
"go" failed: exit status 1; `)
On 2014/05/15 08:57:19, fwereade wrote:
> please break all these cases into separate test methods where possible

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c
*gc.C) {
On 2014/05/15 08:57:20, fwereade wrote:
> The SUT here is BuildToolsTarball, isn't it? We're testing that it calls
> BundleTools in the expected way.

I don't follow sorry. I was testing if the mocked BundleTools gets called. I
could have easily used sync.Upload instead. I also don't know what SUT means
sorry.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:483: writer       io.Writer
On 2014/05/15 08:57:19, fwereade wrote:
> writer seems unused -- I'd expect us to write something to it and check that
it
> was somehow reflected in the result of BuildToolsTarball...

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:493: sync.BuildToolsTarball(&version.Current.Number)
On 2014/05/15 08:57:20, fwereade wrote:
> error/result check?

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:52: }
On 2014/05/15 08:57:20, fwereade wrote:
> I'm still nervous that this is a parallel implementation of logic in
uploadTools
> itself that *will* invitably change out of sync with this implementation. This
> is basically how mocking ends up going wrong IMO...

Done. It's GONE GONE GONE!

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:69: 
On 2014/05/15 08:57:20, fwereade wrote:
> not sure we need the blank lines here though :)

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:90: stor, err :=
filestorage.NewFileStorageWriter(toolsDir)
On 2014/05/15 08:57:20, fwereade wrote:
> I still don't think we need to mess around with storage stuff. Can't we just
> write direct to a file in a directory? What do we get from the extra layer of
> complexity here?

Done.

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:281: // UploadToStorage uploads tools and
metadata for the specified versions to dir.
On 2014/05/15 08:57:20, fwereade wrote:
> FWIW this is UploadToDirectory

Done. Did I do that?

[wallyworld, 2014-05-16 04:10:18]

LGTM

[fwereade, 2014-05-16 08:29:36]

Thanks, this is great. LGTM with trivials.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:292: c.Assert(string(out), gc.Equals, "")
On 2014/05/16 03:06:07, waigani wrote:
> On 2014/05/15 08:57:19, fwereade wrote:
> > I'm not sure you *really* need this bit, but equally you don't have to take
it
> > out :).
> > 
> > The super-sweet spot would be to pull all the MakeTool-style functionality
out
> > into its own helpers and use all that same (tested) test-support code
> elsewhere
> > -- but if you want to do that, it'd deserve its own CL.
> 
> Do you mean a helper suite i.e. FakeToolsSuite or a helper package like utils?

I'd guess a suite would work well for easy integration of setup/teardown, but
not necessarily. Whatever seems to work best :).

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:317: // sync.bundleTools can be mocked to improve
test speed.
On 2014/05/16 03:06:08, waigani wrote:
> On 2014/05/15 08:57:19, fwereade wrote:
> > It's not 100% clear what this test is meant to be doing, indeed. Given what
we
> > have elsewhere I think I'd be testing this stuff by mocking the build/bundle
> > stuff, and directly checking what gets uploaded.
> 
> Done. Is TestUploadToolsBadBuild enough?

Yeah, as long as we exercise both the happy path and the sad one, I think we're
good.

https://codereview.appspot.com/87130045/diff/80001/environs/sync/sync_test.go...
environs/sync/sync_test.go:480: func (s *uploadSuite) TestMockBundleTools(c
*gc.C) {
On 2014/05/16 03:06:08, waigani wrote:
> On 2014/05/15 08:57:20, fwereade wrote:
> > The SUT here is BuildToolsTarball, isn't it? We're testing that it calls
> > BundleTools in the expected way.
> 
> I don't follow sorry. I was testing if the mocked BundleTools gets called. I
> could have easily used sync.Upload instead. I also don't know what SUT means
> sorry.

System Under Test. And no need to apologise :).

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/tes...
environs/tools/testing/testing.go:52: }
On 2014/05/16 03:06:08, waigani wrote:
> On 2014/05/15 08:57:20, fwereade wrote:
> > I'm still nervous that this is a parallel implementation of logic in
> uploadTools
> > itself that *will* invitably change out of sync with this implementation.
This
> > is basically how mocking ends up going wrong IMO...
> 
> Done. It's GONE GONE GONE!

w00t!

https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g...
environs/sync/sync_test.go:284: // sync.bundleTools can be mocked to improve
test speed.
Hey, I think it's clicked. It's downloading the tools (and doing some basic
checks on them) to check that they really got uploaded. I think I'd be happiest
to see more direct, explicit tests: I think we actually have pretty detailed
ones around bootstrap/upgrade-juju, but it'd be best to have more detail in the
Upload tests and maybe a bit less in the cmd/juju ones.

https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g...
environs/sync/sync_test.go:467: // Test that Upload func passes after
BundleTools func is mocked out
maybe put htis comment just above the preceding s.PatchValue?

https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g...
environs/sync/sync_test.go:483: // mocked out
similarly

https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te...
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te...
environs/tools/testing/testing.go:58: // Write fake tools to storage
Not really accurate any more

https://codereview.appspot.com/87130045/diff/100001/environs/tools/testing/te...
environs/tools/testing/testing.go:66: StorageName: tools.StorageName(vers),
you can just use "name" here

[fwereade, 2014-05-16 08:30:21]

On 2014/05/16 08:29:36, fwereade wrote:
> Thanks, this is great. LGTM with trivials.
> https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.go
> File environs/sync/sync_test.go (right):
> 
>
https://codereview.appspot.com/87130045/diff/100001/environs/sync/sync_test.g...
> environs/sync/sync_test.go:284: // sync.bundleTools can be mocked to improve
> test speed.
> Hey, I think it's clicked. It's downloading the tools (and doing some basic
> checks on them) to check that they really got uploaded. I think I'd be
happiest
> to see more direct, explicit tests: I think we actually have pretty detailed
> ones around bootstrap/upgrade-juju, but it'd be best to have more detail in
the
> Upload tests and maybe a bit less in the cmd/juju ones.

I don't want that in this CL, though, it's big enough already.

[waigani, 2014-05-19 00:05:14]

Please take a look.