Move rate limiting/retrying to error catching, add tests for rate limit handling #570
Conversation
Codecov Report
@@ Coverage Diff @@
## master #570 +/- ##
==========================================
+ Coverage 81.72% 85.86% +4.14%
==========================================
Files 6 6
Lines 279 276 -3
Branches 43 42 -1
==========================================
+ Hits 228 237 +9
+ Misses 36 27 -9
+ Partials 15 12 -3
Continue to review full report at Codecov.
|
src/WebClient.spec.js
Outdated
| it('should expose retry headers in the response', function() { | ||
| const scope = nock('https://slack.com') | ||
| .post(/api/) | ||
| .reply(200, { ok: true }, { |
There was a problem hiding this comment.
hmmm... not quite. we would only ever see retry-after when the response has a status code of 429. i'm not 100% sure what the body would contain, but { ok: true } seems wrong to me, too.
There was a problem hiding this comment.
my bad; i was supposed to change that to 429, but forgot about it. Thanks!
src/WebClient.spec.js
Outdated
| it('should automatically retry the request after the specified timeout', function() { | ||
| const scope = nock('https://slack.com') | ||
| .post(/api/) | ||
| .reply(429, { ok: true }, { |
There was a problem hiding this comment.
Will change this and every instance thereof
src/WebClient.spec.js
Outdated
| const client = new WebClient(token, { retryConfig: { retries: 1 } }); | ||
| return client.apiCall('method') | ||
| .then((resp) => { | ||
| assert.propertyVal(resp, 'ok', true);; |
There was a problem hiding this comment.
how can we verify that the request actually occurred after the specified timeout (1 second)?
There was a problem hiding this comment.
note: time specific, not existence
src/WebClient.spec.js
Outdated
| .post(/api/) | ||
| .reply(200, { ok: true }); | ||
| const client = new WebClient(token, { retryConfig: { retries: 3 } }); | ||
| return client.apiCall('method') |
There was a problem hiding this comment.
i don't think this reflects the intention of the test case. the idea is: given a WebClient with a request concurrency of 2 (just to keep things simple), you fire three requests concurrently (without waiting for the promises to resolve). the first request gets rate-limited, for a specified amount of time, let's say 1 second. the second request would have already been triggered (the concurrency allows for 2 requests to be in flight at the same time) and let's say it completes very quickly (~10ms), but the third request should not even start until after the 1 second timeout from the first request has elapsed. does this make sense?
There was a problem hiding this comment.
i get it, will change to reflect this
src/WebClient.spec.js
Outdated
| assert.fail(0, 1, 'Rate Limit retry not turned off'); | ||
| }) | ||
| .catch((error) => { | ||
| assert.isFalse(scope.isDone()); |
There was a problem hiding this comment.
it makes sense that you're setting up responses that are never meant to trigger in the scope, and therefore you check scope.isDone() rather than calling scope.done(). do you think we should also be cleaning up these test cases in a teardown method with scope.cleanAll()?
src/WebClient.ts
Outdated
| if (error.statusCode === 429) { | ||
| const retryAfterMs = result.retryAfter !== undefined ? result.retryAfter : (60 * 1000); | ||
| this.emit('rate_limited', retryAfterMs / 1000); | ||
| if (this.retryConfig.retries !== 0) { |
There was a problem hiding this comment.
ah-hah. i think you found an issue that actually resides within the tests. the case should allow rate limit triggered retries to be turned off was noted assuming that's functionality we intend to support. but some time after that note, it was made clear that the retryConfig has nothing to do with the rate-limit control built into this object. see: #451
specifically, i think we need to remove this condition and think about that problem more holistically. right now you check a very specific retries property of the retryConfig, but its actually a complex object that's meant to be interpreted by a more sophisticated library like retry. long story short: we need to solve for that test case in a different way, and we should probably remove its implementation for now.
we can start work on resolving #451, but i think that should happen in a separate PR.
accidentally put 'only' remove overly specific rate limiting functionality, remove tests that need further consideration remove unnecessary lines
Summary
apiCall()'s.then()area stopped it from actually running, so I moved it to the.catch()portion to make it work.Requirements (place an
xin each[ ])