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

Issue 6851112: Swift test service double implemented. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 4 months ago by dimitern
Modified:
11 years, 4 months ago
Reviewers:
jameinel, mp+136362
Visibility:
Public.

Description

Swift test service double implemented. Implements a minimal number of OS Swift API, needed for juju testing. https://code.launchpad.net/~dimitern/goose/swift-testing-service/+merge/136362 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 34

Patch Set 2 : Swift test service double implemented. #

Patch Set 3 : Swift test service double implemented. #

Total comments: 10

Patch Set 4 : Swift test service double implemented. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+668 lines, -0 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A testservices/swiftservice/service.go View 1 1 chunk +98 lines, -0 lines 0 comments Download
A testservices/swiftservice/service_http.go View 1 1 chunk +151 lines, -0 lines 0 comments Download
A testservices/swiftservice/service_http_test.go View 1 2 3 1 chunk +285 lines, -0 lines 3 comments Download
A testservices/swiftservice/service_test.go View 1 1 chunk +86 lines, -0 lines 0 comments Download
A testservices/swiftservice/setup_test.go View 1 chunk +10 lines, -0 lines 0 comments Download
A testservices/swiftservice/swiftservice.go View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download

Messages

Total messages: 10
dimitern
Please take a look.
11 years, 4 months ago (2012-11-27 10:41:39 UTC) #1
jameinel
I think this is reasonable to land as a first draft of the SWIFT services, ...
11 years, 4 months ago (2012-11-27 11:03:07 UTC) #2
dfc
Very nice. https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/service.go File testservices/swiftservice/service.go (right): https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/service.go#newcode51 testservices/swiftservice/service.go:51: _, err := s.GetObject(container, name) This can ...
11 years, 4 months ago (2012-11-27 11:13:33 UTC) #3
gz
Seems like a good start, a few cleanup suggestions for the service. https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/service.go File testservices/swiftservice/service.go ...
11 years, 4 months ago (2012-11-27 11:46:35 UTC) #4
dimitern
Please take a look. https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/service.go File testservices/swiftservice/service.go (right): https://codereview.appspot.com/6851112/diff/1/testservices/swiftservice/service.go#newcode44 testservices/swiftservice/service.go:44: return fmt.Errorf("container already exists %q", ...
11 years, 4 months ago (2012-11-27 18:57:30 UTC) #5
dimitern
Please take a look.
11 years, 4 months ago (2012-11-27 19:01:19 UTC) #6
jameinel
Overall LGTM. I'd like to see the tests split up a little bit more, but ...
11 years, 4 months ago (2012-11-28 10:19:33 UTC) #7
dimitern
Submitting, with the changes suggested. https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/service_http.go File testservices/swiftservice/service_http.go (right): https://codereview.appspot.com/6851112/diff/8002/testservices/swiftservice/service_http.go#newcode10 testservices/swiftservice/service_http.go:10: On 2012/11/28 10:19:33, jameinel ...
11 years, 4 months ago (2012-11-28 20:19:58 UTC) #8
dimitern
*** Submitted: Swift test service double implemented. Implements a minimal number of OS Swift API, ...
11 years, 4 months ago (2012-11-28 20:20:44 UTC) #9
jameinel
11 years, 4 months ago (2012-11-29 06:31:40 UTC) #10
LGTM

I have some comments, but don't think it needs another review before landing.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
File testservices/swiftservice/service_http_test.go (right):

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:38: func (s *SwiftHTTPSuite)
sendRequest(method, path string, body []byte) (*http.Response, error) {
All callers of this follow up with:
c.Assert(err, IsNil)
c.Assert(resp.ResponseCode, Equals, X)

So I would propose to move that code into this function, and simplify all of the
callers.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:50: if token != "" {
Where is 'token' being defined? I just realized I don't see its definition, and
it is both here and in the SetUpSuite.
If it is a global variable (which I'm not super excited about, but might be
necessary), then it *definitely* should not just be called "token".
The fact that we sometimes compare it to "" means that not only is it global
state, it is *mutable* global state, which is almost always incorrect.

https://codereview.appspot.com/6851112/diff/11001/testservices/swiftservice/s...
testservices/swiftservice/service_http_test.go:106: }
The 'removeContainer' here looks like cleanup. So if have a 'ensureNoContainer'
that removes it if it exists, but doesn't fail (removeContainer might already be
that), then we can do:

s.ensureNotContainer("test", c)
defer s.removeContainer("test", c)
s.sendRequest(c, "PUT", "test", nil, http.StatusCreated)

And that is the complete test. Though we might take it a step further, and
TearDownTest should be the thing that makes sure you don't leave any state in
the service.

Overall splitting these up looks good to me.
Sign in to reply to this message.

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