Skip to content

Commit

Permalink
fix line length for lint
Browse files Browse the repository at this point in the history 
accidentally put 'only'

remove overly specific rate limiting functionality, remove tests that need further consideration

remove unnecessary lines
  • Loading branch information
@ggruiz
ggruiz committed May 31, 2018
1 parent 983f42f commit 7086db2
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 84 deletions.
87 changes: 17 additions & 70 deletions src/WebClient.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ const { assert } = require('chai');
const { WebClient } = require('./WebClient');
const { LogLevel } = require('./logger');
const { addAppMetadata } = require('./util');
const rapidRetryPolicy = require('./retry-policies').rapidRetryPolicy;
const CaptureStdout = require('capture-stdout');
const isPromise = require('p-is-promise');
const nock = require('nock');
const Busboy = require('busboy');
const sinon = require('sinon');

const token = 'xoxa-faketoken';
const fastRetriesForTest = { minTimeout: 0, maxTimeout: 1 };

describe('WebClient', function () {

Expand Down Expand Up @@ -72,7 +72,7 @@ describe('WebClient', function () {

describe('apiCall()', function () {
beforeEach(function () {
this.client = new WebClient(token, { retryConfig: fastRetriesForTest });
this.client = new WebClient(token, { retryConfig: rapidRetryPolicy });
});

describe('when making a successful call', function () {
Expand Down Expand Up @@ -339,7 +339,7 @@ describe('WebClient', function () {
.post(/api/)
.reply(200, { ok: true });
// NOTE: appMetaData is only evalued on client construction, so we cannot use the client already created
const client = new WebClient(token, { retryConfig: fastRetriesForTest });
const client = new WebClient(token, { retryConfig: rapidRetryPolicy });
return client.apiCall('method')
.then(() => {
scope.done();
Expand All @@ -349,7 +349,7 @@ describe('WebClient', function () {
});

describe('apiCall() - without a token', function () {
const client = new WebClient(undefined, { retryConfig: fastRetriesForTest });
const client = new WebClient(undefined, { retryConfig: rapidRetryPolicy });

const scope = nock('https://slack.com')
// NOTE: this could create false negatives if the serialization order changes (it shouldn't matter)
Expand All @@ -365,7 +365,7 @@ describe('WebClient', function () {

describe('named method aliases (facets)', function () {
beforeEach(function () {
this.client = new WebClient(token, { retryConfig: fastRetriesForTest });
this.client = new WebClient(token, { retryConfig: rapidRetryPolicy });
});
it('should properly mount methods as functions', function () {
// This test doesn't exhaustively check all the method aliases, it just tries a couple.
Expand Down Expand Up @@ -471,14 +471,13 @@ describe('WebClient', function () {
});

describe('has an option to set the retry policy ', function () {

it('retries a request which fails to get a response', function () {
const scope = nock('https://slack.com')
.post(/api/)
.replyWithError('could be a ECONNREFUESD, ENOTFOUND, ETIMEDOUT, ECONNRESET')
.post(/api/)
.reply(200, { ok: true });
const client = new WebClient(token, { retryConfig: fastRetriesForTest });
const client = new WebClient(token, { retryConfig: rapidRetryPolicy });
return client.apiCall('method')
.then((resp) => {
assert.propertyVal(resp, 'ok', true);
Expand All @@ -491,7 +490,7 @@ describe('WebClient', function () {
.reply(500)
.post(/api/)
.reply(200, { ok: true });
const client = new WebClient(token, { retryConfig: fastRetriesForTest });
const client = new WebClient(token, { retryConfig: rapidRetryPolicy });
return client.apiCall('method')
.then((resp) => {
assert.propertyVal(resp, 'ok', true);
Expand All @@ -501,94 +500,42 @@ describe('WebClient', function () {
});

describe('has rate limit handling', function () {
it('should expose retry headers in the response', function() {
const scope = nock('https://slack.com')
.post(/api/)
.reply(200, { ok: true }, {
'retry-after': 1
});
const client = new WebClient(token);
return client.apiCall('method')
.then((result) => {
assert.deepNestedInclude(result, { 'retryAfter': 1000 });
scope.done();
});
});

// NOTE: see retry policy note below
it('should allow rate limit triggered retries to be turned off', function() {
const scope = nock('https://slack.com')
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 0
})
.post(/api/)
.reply(200, { ok: true });
const client = new WebClient(token, { retryConfig: { retries: 0 } });
return client.apiCall('method')
.catch((error) => {
assert.equal(error.statusCode, 429);
assert.isFalse(scope.isDone());
});
});


// NOTE: Check issue #451
it('should expose retry headers in the response');
it('should allow rate limit triggered retries to be turned off');

describe('when a request fails due to rate-limiting', function () {
// NOTE: is this retrying configurable with the retry policy? is it subject to the request concurrency?
it('should automatically retry the request after the specified timeout', function() {
const scope = nock('https://slack.com')
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 1
})
.reply(429, {}, { 'retry-after': 1 })
.post(/api/)
.reply(200, { ok: true });
const client = new WebClient(token, { retryConfig: { retries: 1 } });
const startTime = new Date().getTime();
return client.apiCall('method')
.then((resp) => {
assert.propertyVal(resp, 'ok', true);;
const time = new Date().getTime() - startTime;
assert.isAtLeast(time, 1000, 'elapsed time is at least a second');
assert.propertyVal(resp, 'ok', true);
});
});

it('should pause the remaining requests in queue', function() {
const scope = nock('https://slack.com')
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 0
})
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 1
})
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 0
})
.post(/api/)
.reply(200, { ok: true });
const client = new WebClient(token, { retryConfig: { retries: 3 } });
return client.apiCall('method')
.then((resp) => {
assert.propertyVal(resp, 'ok', true);;
});
});
it('should pause the remaining requests in queue');

it('should emit a rate_limited event on the client', function() {
const spy = sinon.spy();
const scope = nock('https://slack.com')
.post(/api/)
.reply(429, { ok: true }, {
'retry-after': 0
});
.reply(429, {}, { 'retry-after': 0 });
const client = new WebClient(token, { retryConfig: { retries: 0 } });
client.on('rate_limited', spy);
return client.apiCall('method')
.catch((err) => {
sinon.assert.calledOnce(spy);
});
});

});
});

Expand Down
16 changes: 2 additions & 14 deletions src/WebClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,27 +159,15 @@ export class WebClient extends EventEmitter {
} else if (error.name === 'ReadError') {
throw readErrorWithOriginal(error);
} else if (error.name === 'HTTPError') {
// Special case: retry if 429
const result = this.buildResult(error.response);
// Special case: retry if 429;
if (error.statusCode === 429) {
const result = this.buildResult(error.response);
const retryAfterMs = result.retryAfter !== undefined ? result.retryAfter : (60 * 1000);
this.emit('rate_limited', retryAfterMs / 1000);
this.logger.info(`API Call failed due to rate limiting. Will retry in ${retryAfterMs / 1000} seconds.`);
// wait and return the result from calling `task` again after the specified number of seconds
return delay(retryAfterMs).then(task);
}

// If a 429 error is NOT OK, treat as irrecoverable by throwing an AbortError to end retries.
if (!result.ok) {
const fatalErr = errorWithCode(
new Error(`An API error occurred: ${result.error}`),
ErrorCode.PlatformError,
);
fatalErr.data = result;
throw new pRetry.AbortError(fatalErr);
}

// if it's some other http error, just throw as normal
throw httpErrorWithOriginal(error);
} else {
throw error;
Expand Down

0 comments on commit 7086db2

Please sign in to comment.