Code review comment for lp:~axwalk/juju-core/sshstorage-tmpdir

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go
File environs/sshstorage/storage_test.go (right):

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode80
environs/sshstorage/storage_test.go:80: for i := 0; i < 2; i++ {
On 2013/09/18 04:39:41, thumper wrote:
> It isn't immediately obvious why you are doing this twice.

Added a comment.

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode81
environs/sshstorage/storage_test.go:81: storage, err :=
NewSSHStorage("example.com", storageDir, "")
On 2013/09/18 04:39:41, thumper wrote:
> Same comment as for the filestorage, the "" here isn't obvious.

Done. Also updated Put to create/remove remove the default temporary
directory, though it's less of an issue here.

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode101
environs/sshstorage/storage_test.go:101:
c.Assert(os.Mkdir(filepath.Join(storageDir, "a"), 0755), gc.IsNil)
On 2013/09/18 04:39:41, thumper wrote:
> I have to admit to finding putting the work inside the assert harder
to follow
> than having them separated out. To me it appears to confuse the work
and the
> test.

I've separated the lengthier ones out to make it obvious what's the
code-under-test and what's the assertion; I've left many of the shorter
assertions alone, e.g. c.Assert(storage.Remove("a"), gc.NotNil).

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode170
environs/sshstorage/storage_test.go:170: defer storage.Close()
On 2013/09/18 04:39:41, thumper wrote:
> Now that I've seen this same code three times... a function is in
order.

> func (s *storageSuite) makeStorage(c *gc.C) <storageType> {
> storageDir := c.MkDir()
> storage, err := NewSSHStorage("example.com", storageDir, "")
> c.Assert(err, gc.IsNil)
> s.AddCleanup(func (*gc.C) {
> storage.Close()
> })
> return storage
> }

> The tests then each get shorter, and logic is in one place.

Done.

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode177
environs/sshstorage/storage_test.go:177:
c.Assert(ioutil.WriteFile(filepath.Join(storageDir, "a", "b1"), nil, 0),
gc.IsNil)
On 2013/09/18 04:39:41, thumper wrote:
> This is also screaming out for a helper method.

> You want the method to describe what is happening.

Yep, done. Added a "createFiles" test helper.

https://codereview.appspot.com/13660047/