Please take a look.
https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go File environs/ec2/juju_test.go (right): https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:20: // jujuLocalTests wraps jujutest.Tests by adding s/jujuLocalTests/localTests/ As we talked in the other CL, let's kill the overuse of "juju" in variables/packages here. We don't have to change other things in this CL, but since you're renaming these, let's take the chance to get rid of it already. https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:26: type jujuLocalTests struct { localTests https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:32: type jujuLocalLiveTests struct { localLiveTests https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:84: }, The indenting here feels overboard: srv: localServer{setup: scn.setup}, https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:93: }, Ditto. https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go File environs/ec2/live_test.go (right): https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go#newco... environs/ec2/live_test.go:10: // integration_test_environments holds the environments configuration Name is wrong. https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go#newco... environs/ec2/live_test.go:23: func registerJujuIntegrationTests() { s/Juju// https://codereview.appspot.com/5490073/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/5490073/diff/1/environs/jujutest/livetests.go#... environs/jujutest/livetests.go:10: func (t *LiveTests) TestStartStop(c *C) { I guess that's alright to begin with, since we have nothing better, but right now things are a bit awkward.. we have several scenarios, that run exactly the same tests in LiveTests and Tests, and now we have the same test in Tests and LiveTests.. ubber duplication of testing. Good to begin with, but let's make some sense of that in the future. https://codereview.appspot.com/5490073/diff/2002/environs/ec2/juju_test.go File environs/ec2/juju_test.go (right): https://codereview.appspot.com/5490073/diff/2002/environs/ec2/juju_test.go#ne... environs/ec2/juju_test.go:124: c.Assert(Regions["test"].EC2Endpoint, Not(Equals), "") Please drop this assertion from the test (which means we don't need TestStartStop here). This is testing the test suite.
PTAL https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go File environs/ec2/juju_test.go (right): https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:20: // jujuLocalTests wraps jujutest.Tests by adding On 2011/12/19 17:07:33, niemeyer wrote: > s/jujuLocalTests/localTests/ > > As we talked in the other CL, let's kill the overuse of "juju" in > variables/packages here. We don't have to change > other things in this CL, but since you're renaming these, > let's take the chance to get rid of it already. done https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:26: type jujuLocalTests struct { On 2011/12/19 17:07:33, niemeyer wrote: > localTests Done. https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:32: type jujuLocalLiveTests struct { On 2011/12/19 17:07:33, niemeyer wrote: > localLiveTests Done. https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:84: }, On 2011/12/19 17:07:33, niemeyer wrote: > The indenting here feels overboard: > > srv: localServer{setup: scn.setup}, Done. https://codereview.appspot.com/5490073/diff/1/environs/ec2/juju_test.go#newco... environs/ec2/juju_test.go:93: }, On 2011/12/19 17:07:33, niemeyer wrote: > Ditto. Done. https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go File environs/ec2/live_test.go (right): https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go#newco... environs/ec2/live_test.go:10: // integration_test_environments holds the environments configuration On 2011/12/19 17:07:33, niemeyer wrote: > Name is wrong. Done. https://codereview.appspot.com/5490073/diff/1/environs/ec2/live_test.go#newco... environs/ec2/live_test.go:23: func registerJujuIntegrationTests() { On 2011/12/19 17:07:33, niemeyer wrote: > s/Juju// Done. https://codereview.appspot.com/5490073/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/5490073/diff/1/environs/jujutest/livetests.go#... environs/jujutest/livetests.go:10: func (t *LiveTests) TestStartStop(c *C) { On 2011/12/19 17:07:33, niemeyer wrote: > I guess that's alright to begin with, since we have nothing better, but right > now things are a bit awkward.. we have several scenarios, that run exactly the > same tests in LiveTests and Tests, and now we have the same test in Tests and > LiveTests.. ubber duplication of testing. Good to begin with, but let's make > some sense of that in the future. agreed https://codereview.appspot.com/5490073/diff/2002/environs/ec2/juju_test.go File environs/ec2/juju_test.go (right): https://codereview.appspot.com/5490073/diff/2002/environs/ec2/juju_test.go#ne... environs/ec2/juju_test.go:124: c.Assert(Regions["test"].EC2Endpoint, Not(Equals), "") On 2011/12/19 17:07:33, niemeyer wrote: > Please drop this assertion from the test (which means we don't need > TestStartStop here). This is testing the test suite. i put this in after i had a bug which wasn't easy to find caused by the server-setup logic being wrong. but since that seems to work now, i'll delete it.
LGTM, thanks!
*** Submitted: environs/ec2test: add new test suite for live tests R=niemeyer CC= https://codereview.appspot.com/5490073
