Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up RequestLib tests #162

Merged
merged 8 commits into from
Dec 21, 2023
Merged

Speed up RequestLib tests #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1c94d6e
Speed up `RequestLib` tests
StuAA78 Mar 13, 2023
1c96f85
Export default options in `RequestLib`
StuAA78 Mar 13, 2023
e073163
Shorten `backoffLimit` in tests
StuAA78 Mar 13, 2023
50e4e61
Remove `timeout` option in retry tests
StuAA78 Mar 13, 2023
8ba86ff
Changes needed to get the unit tests passing again
Cruikshanks Dec 19, 2023
71c4580
Update documentation
Cruikshanks Dec 19, 2023
fade4dc
Merge branch 'main' into speed-up-request-lib-tests
Cruikshanks Dec 19, 2023
ad3f655
Merge branch 'main' into speed-up-request-lib-tests
Cruikshanks Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 60 additions & 43 deletions app/lib/request.lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,63 @@ const { HttpsProxyAgent } = require('hpagent')

const requestConfig = require('../../config/request.config.js')

/**
* Returns an object containing the default options.
*
* We want to be able to export these options so we can set specific settings that may be multiple objects deep. For
* example, if we want to set `retry.backoffLimit` then we can't simply pass `{ retry: { backoffLimit: 5 } }` to the lib
* as `additionalOptions` as this would replace the entire `retry` section. We would therefore want to get the exported
* `retry` section and add our setting to it before passing it back as `additionalOptions`.
*
* Note that we have a function here rather than defining a const as this did not allow us to override settings using
* Sinon to stub `requestConfig`; the const appeared to have its values fixed when the file was required, whereas a
* function generates its values each time it's called.
*
* @returns {Object} default options to pass to Got when making a request
*/
function defaultOptions () {
return {
// This uses the ternary operator to give either an `agent` object or an empty object, and the spread operator to
// bring the result back into the top level of the `defaultOptions` object.
...(requestConfig.httpProxy
? {
agent: {
https: new HttpsProxyAgent({ proxy: requestConfig.httpProxy })
}
}
: {}),
// If we don't have this setting Got will throw its own HTTPError unless the result is 2xx or 3xx. That makes it
// impossible to see what the status code was because it doesn't get set on the response object Got provides when
// an error is thrown. With this set Got will treat a 404 in the same way it treats a 204.
throwHttpErrors: false,
// Got has a built in retry mechanism. We have found though you have to be careful with what gets retried. Our
// preference is to only retry in the event of a timeout on assumption the destination server might be busy but has
// a chance to succeed when attempted again
retry: {
// The default is also 2 retries before erroring. We specify it to make this fact visible
limit: 2,
// We ensure that the only network errors Got retries are timeout errors
errorCodes: ['ETIMEDOUT'],
// By default, Got does not retry PATCH and POST requests. As we only retry timeouts there is no risk in retrying
// our PATCH and POST requests. So, we set methods to be Got's defaults plus 'PATCH' and 'POST'
methods: ['GET', 'PATCH', 'POST', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE'],
// We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried
statusCodes: []
},
// Got states
//
// > It is a good practice to set a timeout to prevent hanging requests. By default, there is no timeout set.
timeout: {
request: requestConfig.timeout
},
hooks: {
beforeRetry: [
_beforeRetryHook
]
}
}
}

/**
* Make a GET request to the specified URL
*
Expand Down Expand Up @@ -104,48 +161,7 @@ function _logFailure (method, result, url, additionalOptions) {
* @param {Object} additionalOptions Object of custom options
*/
function _requestOptions (additionalOptions) {
const defaultOptions = {
// This uses the ternary operator to give either an `agent` object or an empty object, and the spread operator to
// bring the result back into the top level of the `defaultOptions` object.
...(requestConfig.httpProxy
? {
agent: {
https: new HttpsProxyAgent({ proxy: requestConfig.httpProxy })
}
}
: {}),
// If we don't have this setting Got will throw its own HTTPError unless the result is 2xx or 3xx. That makes it
// impossible to see what the status code was because it doesn't get set on the response object Got provides when
// an error is thrown. With this set Got will treat a 404 in the same way it treats a 204.
throwHttpErrors: false,
// Got has a built in retry mechanism. We have found though you have to be careful with what gets retried. Our
// preference is to only retry in the event of a timeout on assumption the destination server might be busy but has
// a chance to succeed when attempted again
retry: {
// The default is also 2 retries before erroring. We specify it to make this fact visible
limit: 2,
// We ensure that the only network errors Got retries are timeout errors
errorCodes: ['ETIMEDOUT'],
// By default, Got does not retry PATCH and POST requests. As we only retry timeouts there is no risk in retrying
// our PATCH and POST requests. So, we set methods to be Got's defaults plus 'PATCH' and 'POST'
methods: ['GET', 'PATCH', 'POST', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE'],
// We set statusCodes as an empty array to ensure that 4xx, 5xx etc. errors are not retried
statusCodes: []
},
// Got states
//
// > It is a good practice to set a timeout to prevent hanging requests. By default, there is no timeout set.
timeout: {
request: requestConfig.timeout
},
hooks: {
beforeRetry: [
_beforeRetryHook
]
}
}

return { ...defaultOptions, ...additionalOptions }
return { ...defaultOptions(), ...additionalOptions }
}

async function _sendRequest (method, url, additionalOptions) {
Expand Down Expand Up @@ -178,5 +194,6 @@ async function _sendRequest (method, url, additionalOptions) {
module.exports = {
get,
patch,
post
post,
defaultOptions: defaultOptions()
}
40 changes: 28 additions & 12 deletions test/lib/request.lib.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@ const RequestLib = require('../../app/lib/request.lib.js')

describe('RequestLib', () => {
const testDomain = 'http://example.com'

// NOTE: We make the tests run much faster by setting backoffLimit to 50 in Got's retry options. The time between
// retries in Got is based on a computed value; ((2 ** (attemptCount - 1)) * 1000) + noise
//
// This means the delay increases exponentially between retries. That's great when making requests for real but it
// drastically slows down our test suite. Prior to making this change the tests for this module took an avg of 40 secs
// to finish. The backoffLimit sets an upper limit for the computed value. When applied here it brings the avg down
// to 16 secs.
//
// The behaviour doesn't change; we are still retrying requests. But now we are only waiting a maximum of 50ms between
// them.
const shortBackoffLimitRetryOptions = {
...RequestLib.defaultOptions.retry,
backoffLimit: 50
}

let notifierStub

beforeEach(() => {
Expand Down Expand Up @@ -156,13 +172,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as false", async () => {
const result = await RequestLib.get(testDomain)
const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.false()
})

it("has a 'response' property containing the error", async () => {
const result = await RequestLib.get(testDomain)
const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response).to.be.an.error()
Expand Down Expand Up @@ -191,13 +207,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as true", async () => {
const result = await RequestLib.get(testDomain)
const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.true()
})

it("has a 'response' property containing the web response", async () => {
const result = await RequestLib.get(testDomain)
const result = await RequestLib.get(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response.statusCode).to.equal(200)
Expand Down Expand Up @@ -365,13 +381,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as false", async () => {
const result = await RequestLib.patch(testDomain)
const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.false()
})

it("has a 'response' property containing the error", async () => {
const result = await RequestLib.patch(testDomain)
const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response).to.be.an.error()
Expand Down Expand Up @@ -400,13 +416,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as true", async () => {
const result = await RequestLib.patch(testDomain)
const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.true()
})

it("has a 'response' property containing the web response", async () => {
const result = await RequestLib.patch(testDomain)
const result = await RequestLib.patch(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response.statusCode).to.equal(200)
Expand Down Expand Up @@ -574,13 +590,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as false", async () => {
const result = await RequestLib.post(testDomain)
const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.false()
})

it("has a 'response' property containing the error", async () => {
const result = await RequestLib.post(testDomain)
const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response).to.be.an.error()
Expand Down Expand Up @@ -609,13 +625,13 @@ describe('RequestLib', () => {

describe('the result it returns', () => {
it("has a 'succeeded' property marked as true", async () => {
const result = await RequestLib.post(testDomain)
const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.succeeded).to.be.true()
})

it("has a 'response' property containing the web response", async () => {
const result = await RequestLib.post(testDomain)
const result = await RequestLib.post(testDomain, { retry: shortBackoffLimitRetryOptions })

expect(result.response).to.exist()
expect(result.response.statusCode).to.equal(200)
Expand Down
Loading