From 1c94d6e5262d5fdb332b224f26fbe4011f6eb0b4 Mon Sep 17 00:00:00 2001 From: Stuart Adair <43574728+StuAA78@users.noreply.github.com> Date: Mon, 13 Mar 2023 17:34:04 +0000 Subject: [PATCH 1/6] Speed up `RequestLib` tests Currently some of our tests for `RequestLib` are slow. This PR aims to fix this. From 1c96f8510f8b6df47fd45f22d0944f7c994dba7d Mon Sep 17 00:00:00 2001 From: Stuart Adair <43574728+StuAA78@users.noreply.github.com> Date: Mon, 13 Mar 2023 18:10:23 +0000 Subject: [PATCH 2/6] Export default options in `RequestLib` --- app/lib/request.lib.js | 101 +++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 43 deletions(-) diff --git a/app/lib/request.lib.js b/app/lib/request.lib.js index 1f69c5ce47..1f82e43a9c 100644 --- a/app/lib/request.lib.js +++ b/app/lib/request.lib.js @@ -9,6 +9,61 @@ 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. + */ +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 * @@ -104,48 +159,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) { @@ -178,5 +192,6 @@ async function _sendRequest (method, url, additionalOptions) { module.exports = { get, patch, - post + post, + defaultOptions: defaultOptions() } From e073163d29955e793ceb06ee3101c70de33642f4 Mon Sep 17 00:00:00 2001 From: Stuart Adair <43574728+StuAA78@users.noreply.github.com> Date: Mon, 13 Mar 2023 18:11:48 +0000 Subject: [PATCH 3/6] Shorten `backoffLimit` in tests --- test/lib/request.lib.test.js | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 9c85adf746..4998a43355 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -17,6 +17,11 @@ const RequestLib = require('../../app/lib/request.lib.js') describe('RequestLib', () => { const testDomain = 'http://example.com' + const shortBackoffLimitRetryOptions = { + ...RequestLib.defaultOptions.retry, + backoffLimit: 50 + } + let notifierStub beforeEach(() => { @@ -156,13 +161,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() @@ -191,13 +196,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) @@ -365,13 +370,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() @@ -400,13 +405,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) @@ -574,13 +579,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() @@ -609,13 +614,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) From 50e4e6180b8e286875d5c2b23831e5627db1f9f9 Mon Sep 17 00:00:00 2001 From: Stuart Adair <43574728+StuAA78@users.noreply.github.com> Date: Mon, 13 Mar 2023 18:12:08 +0000 Subject: [PATCH 4/6] Remove `timeout` option in retry tests --- test/lib/request.lib.test.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 4998a43355..365f4a1844 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -15,7 +15,7 @@ const requestConfig = require('../../config/request.config.js') // Thing under test const RequestLib = require('../../app/lib/request.lib.js') -describe('RequestLib', () => { +describe.only('RequestLib', () => { const testDomain = 'http://example.com' const shortBackoffLimitRetryOptions = { ...RequestLib.defaultOptions.retry, @@ -141,9 +141,7 @@ describe('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we - // have to override the timeout for this specific test to all it to complete - describe('and all retries fail', { timeout: 5000 }, () => { + describe('and all retries fail', () => { beforeEach(async () => { Nock(testDomain) .get(() => true) @@ -350,9 +348,7 @@ describe('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we - // have to override the timeout for this specific test to all it to complete - describe('and all retries fail', { timeout: 5000 }, () => { + describe('and all retries fail', () => { beforeEach(async () => { Nock(testDomain) .patch(() => true) @@ -559,9 +555,7 @@ describe('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we - // have to override the timeout for this specific test to all it to complete - describe('and all retries fail', { timeout: 5000 }, () => { + describe('and all retries fail', () => { beforeEach(async () => { Nock(testDomain) .post(() => true) From 8ba86ff1077abb95730b54196301eab517f97e64 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 19 Dec 2023 17:03:40 +0000 Subject: [PATCH 5/6] Changes needed to get the unit tests passing again --- test/lib/request.lib.test.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 365f4a1844..4998a43355 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -15,7 +15,7 @@ const requestConfig = require('../../config/request.config.js') // Thing under test const RequestLib = require('../../app/lib/request.lib.js') -describe.only('RequestLib', () => { +describe('RequestLib', () => { const testDomain = 'http://example.com' const shortBackoffLimitRetryOptions = { ...RequestLib.defaultOptions.retry, @@ -141,7 +141,9 @@ describe.only('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - describe('and all retries fail', () => { + // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we + // have to override the timeout for this specific test to all it to complete + describe('and all retries fail', { timeout: 5000 }, () => { beforeEach(async () => { Nock(testDomain) .get(() => true) @@ -348,7 +350,9 @@ describe.only('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - describe('and all retries fail', () => { + // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we + // have to override the timeout for this specific test to all it to complete + describe('and all retries fail', { timeout: 5000 }, () => { beforeEach(async () => { Nock(testDomain) .patch(() => true) @@ -555,7 +559,9 @@ describe.only('RequestLib', () => { Sinon.replace(requestConfig, 'timeout', 50) }) - describe('and all retries fail', () => { + // Because of the fake delay in this test, Lab will timeout (by default tests have 2 seconds to finish). So, we + // have to override the timeout for this specific test to all it to complete + describe('and all retries fail', { timeout: 5000 }, () => { beforeEach(async () => { Nock(testDomain) .post(() => true) From 71c45808374b2bb9204eb44fa2ba076e20ff8d3a Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Tue, 19 Dec 2023 17:33:15 +0000 Subject: [PATCH 6/6] Update documentation --- app/lib/request.lib.js | 2 ++ test/lib/request.lib.test.js | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/app/lib/request.lib.js b/app/lib/request.lib.js index 1f82e43a9c..8e8065f816 100644 --- a/app/lib/request.lib.js +++ b/app/lib/request.lib.js @@ -20,6 +20,8 @@ const requestConfig = require('../../config/request.config.js') * 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 { diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 4998a43355..a768e372b1 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -17,6 +17,17 @@ 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