Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1694)

Issue 6567048: environs/ec2: several live tests fixes

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 2 months ago by niemeyer
Modified:
10 years, 2 months ago
Reviewers:
mp+126366, dfc
Visibility:
Public.

Description

environs/ec2: several live tests fixes This should get tests working, or pretty close to working. Time's up today. https://code.launchpad.net/~niemeyer/juju-core/live-tests-fixes/+merge/126366 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -35 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/ec2/live_test.go View 1 chunk +6 lines, -3 lines 1 comment Download
M environs/jujutest/livetests.go View 8 chunks +63 lines, -28 lines 6 comments Download
M environs/jujutest/test.go View 1 chunk +0 lines, -2 lines 0 comments Download
M environs/jujutest/tests.go View 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 5
niemeyer
Please take a look.
10 years, 2 months ago (2012-09-26 02:24:56 UTC) #1
dfc
On 2012/09/26 02:24:56, niemeyer wrote: > Please take a look. Everything but jujutest/livetests.go looks good. ...
10 years, 2 months ago (2012-09-26 02:45:15 UTC) #2
niemeyer
Okay, I'm submitting this as it's surely a step forward. I got the BootstrapAndDeploy passing ...
10 years, 2 months ago (2012-09-26 02:52:36 UTC) #3
dfc
Some comments that I missed. https://codereview.appspot.com/6567048/diff/1/environs/ec2/live_test.go File environs/ec2/live_test.go (left): https://codereview.appspot.com/6567048/diff/1/environs/ec2/live_test.go#oldcode109 environs/ec2/live_test.go:109: LGTM. I wonder why ...
10 years, 2 months ago (2012-09-26 05:11:20 UTC) #4
niemeyer
10 years, 2 months ago (2012-09-26 18:01:22 UTC) #5
https://codereview.appspot.com/6567048/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6567048/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:218: // 5. Provisioner removes dead machine
On 2012/09/26 05:11:20, dfc wrote:
> Can you add a line explaining why the above isn't/cannot be done right now.

I don't think there is any reason why it cannot be done. It isn't done just
because no one did it yet, but we should get there soon.

https://codereview.appspot.com/6567048/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:291: c.Assert(ok, Equals, true, Commentf("watcher
%d died: %v", i, w.Err()))
On 2012/09/26 05:11:20, dfc wrote:
> Should this move up two lines, if the channel is closed, you'll get machine 0

There's no machine id in the message.

https://codereview.appspot.com/6567048/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:379: c.Assert(err, IsNil)
On 2012/09/26 05:11:20, dfc wrote:
> err is shadowed by the inner loop, err here will never be non nil, you can
drop
> this test.

Will do, thanks.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b