Looks good and is pretty well tested, although I think the "retrier" is not really a retrier! Please consider my suggestion about it below, although it might be a larger change than you want to do right now. Otherwise there's a few small things you can change and then land away. General observation - where you're using time.Millisecond in your tests for the retry interval, please use time.Nanosecond. No need to keep the tests waiting any longer :) 117 +// It it usually created using RetryPolicy.getRetrier() and then used by Typo "It it" -> "it is" 117 +// It it usually created using RetryPolicy.getRetrier() and then used by 118 +// performing the request governed by the policy the retrier enforces 119 +// repeatedly until retrier.shouldRetry() returns false. This sentence is pretty hard to read, I've scanned it several times before working it out. Let me suggest an alternative: // retrier is usually created using RetryPolicy.getRetrier(). The retrier enforces the // policy during repeated requests until retrier.shouldRetry() returns false. 137 + ret.retriesLeft -= 1 Go has ++ and --, might as well use -- here: ret.retriesLeft-- I'm not sure it's a good idea to mix the sleep inside this function, it would be better if it stuck to the single task of making the decision of whether to retry or not and allowing the callsite to sleep if it wants. As it stands, it's a strange mix of functionality. It might be an idea to return a time.Delay instead of a bool, which tells the caller how long to sleep, where zero means "don't retry". You could then fold that into another receiver method on retrier which will do all of check, wait and retry. We could define an interface that specs the function out like this so that we can use it in the storage and the management side: type RetryableRequest interface { func RetryRequest(http.Request) (http.Response, error) } and then all the retrying is *completely* encapsulated in the, well, retrier :) 171 +func (*retryPolicySuite) TestIsRetryCodeChecksStatusCode(c *C) { 172 + var testValues = []struct { 173 + retryStatusCodes []int 174 + testStatusCode int 175 + expectedOutcome bool 176 + }{ 177 + {[]int{http.StatusConflict, http.StatusInternalServerError}, http.StatusConflict, true}, 178 + {[]int{http.StatusConflict}, http.StatusConflict, true}, 179 + {[]int{http.StatusConflict}, http.StatusOK, false}, 180 + {[]int{}, http.StatusConflict, false}, 181 + } 182 + for _, test := range testValues { 183 + policy := RetryPolicy{httpStatusCodes: test.retryStatusCodes} 184 + c.Check(policy.isRetryCode(test.testStatusCode), Equals, test.expectedOutcome) 185 + } You have a habit of doing these looping tests! I'm not sure this is any more readable than four simple calls to c.Check. In fact I'm pretty sure it isn't! In fact there's an argument to break it into four separate tests, whose names tell me exactly what scenario is being tested. 248 + for { ... 259 + // Check if the context's retry policy commands to retry the request. 260 + if !retrier.shouldRetry(resp.StatusCode) { 261 + break Wouldn't it be nice if Go had a do-while.... :/ 254 + body, err = readAndClose(resp.Body) 255 + if err != nil { 256 + return nil, nil, fmt.Errorf("failed to read response data: %v", err) 257 + } 258 + 259 + // Check if the context's retry policy commands to retry the request. 260 + if !retrier.shouldRetry(resp.StatusCode) { 261 + break 262 + } I think the readAndClose can be moved outside this loop, there's no need to read the body every time when we only check the status code in the loop. In fact if you take my advice from above and make the retrier live up to its name then this loop is not needed at all. 405 + // Check if the session's retry policy commands to retry the request. 406 + if !retrier.shouldRetry(response.StatusCode) { 407 + break 408 + } Again this could be done much sooner inside a smaller loop, I'm not sure of the benefit of all the other body processing if we are simply going to retry anyway.