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

Mostly comments about test improvements.

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++ {
It isn't immediately obvious why you are doing this twice.

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, "")
Same comment as for the filestorage, the "" here isn't obvious.

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)
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.

https://codereview.appspot.com/13660047/diff/1/environs/sshstorage/storage_test.go#newcode170
environs/sshstorage/storage_test.go:170: defer storage.Close()
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.

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)
This is also screaming out for a helper method.

You want the method to describe what is happening.

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