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#newcode262
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#newcode273
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#newcode288
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#newcode292
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#newcode317
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#newcode452
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#newcode480
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#newcode483
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#newcode493
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/testing.go
File environs/tools/testing/testing.go (right):

https://codereview.appspot.com/87130045/diff/80001/environs/tools/testing/testing.go#newcode52
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/testing.go#newcode69
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/testing.go#newcode90
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/testing.go#newcode281
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?

https://codereview.appspot.com/87130045/