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

Issue 6566058: environs/jujutest: fix live tests.

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

Description

environs/jujutest: fix live tests. https://code.launchpad.net/~rogpeppe/juju-core/085-fix-ec2-live/+merge/126506 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 11

Patch Set 2 : environs/jujutest: fix live tests. #

Patch Set 3 : environs/jujutest: fix live tests. #

Patch Set 4 : environs/jujutest: fix live tests. #

Total comments: 12

Patch Set 5 : environs/jujutest: fix live tests. #

Patch Set 6 : environs/jujutest: fix live tests. #

Total comments: 4

Patch Set 7 : environs/jujutest: fix live tests. #

Patch Set 8 : environs/jujutest: fix live tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -75 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M environs/cloudinit/cloudinit.go View 1 3 chunks +5 lines, -3 lines 0 comments Download
M environs/jujutest/livetests.go View 1 2 3 4 5 6 7 5 chunks +114 lines, -72 lines 0 comments Download

Messages

Total messages: 8
niemeyer
I know it's in progress, but some randoms to help: https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#newcode273 ...
10 years, 2 months ago (2012-09-26 17:33:37 UTC) #1
rog
Please take a look. https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#newcode273 environs/jujutest/livetests.go:273: for _ = range w.Changes() ...
10 years, 2 months ago (2012-09-27 11:38:44 UTC) #2
rog
Please take a look.
10 years, 2 months ago (2012-09-27 11:48:40 UTC) #3
fwereade
LGTM again :)
10 years, 2 months ago (2012-09-27 12:24:50 UTC) #4
niemeyer
https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#newcode273 environs/jujutest/livetests.go:273: for _ = range w.Changes() { On 2012/09/27 11:38:44, ...
10 years, 2 months ago (2012-09-27 13:15:23 UTC) #5
rog
Please take a look. https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/6566058/diff/1/environs/jujutest/livetests.go#newcode273 environs/jujutest/livetests.go:273: for _ = range w.Changes() ...
10 years, 2 months ago (2012-09-27 14:08:30 UTC) #6
niemeyer
LGTM. Just a couple of questions for your consideration: https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests.go File environs/jujutest/livetests.go (right): https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests.go#newcode210 environs/jujutest/livetests.go:210: ...
10 years, 2 months ago (2012-09-27 14:15:41 UTC) #7
rog
10 years, 2 months ago (2012-09-27 14:29:41 UTC) #8
*** Submitted:

environs/jujutest: fix live tests.

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/6566058

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:210: instId1, err := m1.InstanceId()
On 2012/09/27 14:15:42, niemeyer wrote:
> Do we need to refresh to pick the new instance id?

no, but done anyway, so we're not relying on the toolsWaiter semantics.

https://codereview.appspot.com/6566058/diff/9005/environs/jujutest/livetests....
environs/jujutest/livetests.go:260: watcher
On 2012/09/27 14:15:42, niemeyer wrote:
> I don't see the value of having this as an anonymous field. Am I missing
> something?

i used it for the Stop method, but changed to make it more explicit.
Sign in to reply to this message.

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