Code review comment for lp:~niemeyer/goamz/testing-cleanup

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM except for the query about the default security group test.

nice to see all that code factored out.

https://codereview.appspot.com/7232065/diff/1/ec2/ec2_test.go
File ec2/ec2_test.go (right):

https://codereview.appspot.com/7232065/diff/1/ec2/ec2_test.go#newcode24
ec2/ec2_test.go:24: testServer.Start()
that makes much more sense, given we're using the global below.

https://codereview.appspot.com/7232065/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/7232065/diff/1/ec2/ec2t_test.go#newcode447
ec2/ec2t_test.go:447: //resp, err :=
s.ec2.SecurityGroups([]ec2.SecurityGroup{{Name: "default"}}, nil)
why commented out? this was testing specific ec2test functionality.

have you tried these tests live? i seem to remember that
i needed to add the "default" group because it was
being returned for some of the filtering requests.

https://codereview.appspot.com/7232065/diff/1/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/7232065/diff/1/ec2/ec2test/server.go#newcode420
ec2/ec2test/server.go:420: // ImageId: accept anything, we can verify
later
align?
(and below)

https://codereview.appspot.com/7232065/diff/1/ec2/sign.go
File ec2/sign.go (right):

https://codereview.appspot.com/7232065/diff/1/ec2/sign.go#newcode23
ec2/sign.go:23: // be in natural order of the keys. This is distinct
from the
s/in natural/in the natural/
?

https://codereview.appspot.com/7232065/diff/1/exp/mturk/suite_test.go
File exp/mturk/suite_test.go (left):

https://codereview.appspot.com/7232065/diff/1/exp/mturk/suite_test.go#oldcode1
exp/mturk/suite_test.go:1: package mturk_test
great to see all this duplication go away.

https://codereview.appspot.com/7232065/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7232065/diff/1/s3/s3.go#newcode231
s3/s3.go:231: // into different groupings of keys, similar to how
folders would work.
s/would// ?

https://codereview.appspot.com/7232065/

« Back to merge proposal