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 sshstorage/ storage_ test.go (right):
File environs/
https:/ /codereview. appspot. com/13660047/ diff/1/ environs/ sshstorage/ storage_ test.go# newcode80 sshstorage/ storage_ test.go: 80: for i := 0; i < 2; i++ {
environs/
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 sshstorage/ storage_ test.go: 81: storage, err := "example. com", storageDir, "")
environs/
NewSSHStorage(
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 sshstorage/ storage_ test.go: 101: os.Mkdir( filepath. Join(storageDir , "a"), 0755), gc.IsNil)
environs/
c.Assert(
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 storage. Remove( "a"), gc.NotNil).
code-under-test and what's the assertion; I've left many of the shorter
assertions alone, e.g. c.Assert(
https:/ /codereview. appspot. com/13660047/ diff/1/ environs/ sshstorage/ storage_ test.go# newcode170 sshstorage/ storage_ test.go: 170: defer storage.Close()
environs/
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> { "example. com", storageDir, "")
> storageDir := c.MkDir()
> storage, err := NewSSHStorage(
> 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 sshstorage/ storage_ test.go: 177: ioutil. WriteFile( filepath. Join(storageDir , "a", "b1"), nil, 0),
environs/
c.Assert(
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/