From cbf3a7da10a341cc654d8df0f079c576632994ca Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 12:18:13 +0000 Subject: [PATCH 01/17] notify timeout --- src/server/common/connectors/notify/notify.js | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/server/common/connectors/notify/notify.js b/src/server/common/connectors/notify/notify.js index abdfe16..02f97fd 100644 --- a/src/server/common/connectors/notify/notify.js +++ b/src/server/common/connectors/notify/notify.js @@ -6,6 +6,7 @@ import { createToken } from '~/src/server/common/connectors/notify/notify-token- * @typedef {{ content: string}} NotifyContent */ const notifyConfig = config.get('notify') +const requestTimeout = 10000 /** * @param {NotifyContent} data @@ -17,16 +18,31 @@ export async function sendNotification(data) { personalisation: data }) - const response = await proxyFetch( - 'https://api.notifications.service.gov.uk/v2/notifications/email', - { - method: 'POST', - body, - headers: { - Authorization: 'Bearer ' + createToken(notifyConfig.apiKey) + let response + + try { + response = await proxyFetch( + 'https://api.notifications.service.gov.uk/v2/notifications/email', + { + method: 'POST', + body, + headers: { + Authorization: 'Bearer ' + createToken(notifyConfig.apiKey) + }, + signal: AbortSignal.timeout(requestTimeout) } + ) + } catch (err) { + if (err.code && err.code === err.TIMEOUT_ERR) { + throw new Error( + `Request to GOV.uk notify timed out after ${requestTimeout}ms` + ) + } else { + throw new Error( + `Request to GOV.uk notify failed with error: ${err.message}` + ) } - ) + } if (!response.ok) { const responseBody = await response.json() @@ -35,5 +51,6 @@ export async function sendNotification(data) { `HTTP failure from GOV.uk notify: status ${response.status} with the following errors: ${errors.join(', ')}` ) } + return response } From ad6c897bea286884fc6640043e60b9efbf000da0 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 12:45:21 +0000 Subject: [PATCH 02/17] testing --- .../common/connectors/notify/notify.test.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 5598f36..407b3b8 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -68,4 +68,26 @@ describe('sendNotification', () => { "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" ) }) + + describe('timeout with steve', () => { + it('should abort if timeout is hit', async () => { + jest.useFakeTimers() + + global.fetch = jest.fn( + () => + new Promise(() => { + // testing + }) + ) + + const result = sendNotification(testData) + + jest.advanceTimersByTime(11000) + + await expect(result).rejects.toThrow( + 'Request to GOV.uk notify timed out after 10000ms' + ) + jest.useRealTimers() + }) + }) }) From 2325d68b0b35834ddc1f0201cfcdbb022384c662 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 13:00:14 +0000 Subject: [PATCH 03/17] testing --- src/server/common/connectors/notify/notify.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 407b3b8..cce9c10 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -2,10 +2,10 @@ import { sendNotification } from './notify.js' import { proxyFetch } from '~/src/server/common/helpers/proxy.js' import { config } from '~/src/config/config.js' -jest.mock('~/src/server/common/helpers/proxy.js', () => ({ - proxyFetch: jest.fn() -})) -const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) +// jest.mock('~/src/server/common/helpers/proxy.js', () => ({ +// proxyFetch: jest.fn() +// })) +// const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) jest.mock( '~/src/server/common/connectors/notify/notify-token-utils.js', From 56020bccf0404821cd71948bc95510becd4fea4b Mon Sep 17 00:00:00 2001 From: Steve Coleman-Williams Date: Wed, 22 Jan 2025 13:53:50 +0000 Subject: [PATCH 04/17] timeout test triggering --- src/config/config.js | 6 ++++ src/server/common/connectors/notify/notify.js | 10 +++--- .../common/connectors/notify/notify.test.js | 33 +++++++++---------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index e0646d0..ed9bd93 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -233,6 +233,12 @@ export const config = convict({ default: null, nullable: true, env: 'NOTIFY_CASE_DELIVERY_EMAIL_ADDRESS' + }, + timeout: { + doc: 'Timeout for notify requests', + format: Number, + default: 10000, + env: 'NOTIFY_TIMEOUT' } } }) diff --git a/src/server/common/connectors/notify/notify.js b/src/server/common/connectors/notify/notify.js index 02f97fd..8f3e732 100644 --- a/src/server/common/connectors/notify/notify.js +++ b/src/server/common/connectors/notify/notify.js @@ -5,13 +5,13 @@ import { createToken } from '~/src/server/common/connectors/notify/notify-token- /** * @typedef {{ content: string}} NotifyContent */ -const notifyConfig = config.get('notify') -const requestTimeout = 10000 /** * @param {NotifyContent} data */ export async function sendNotification(data) { + const { timeout, ...notifyConfig } = config.get('notify') + const body = JSON.stringify({ template_id: notifyConfig.templateId, email_address: notifyConfig.caseDeliveryEmailAddress, @@ -29,14 +29,12 @@ export async function sendNotification(data) { headers: { Authorization: 'Bearer ' + createToken(notifyConfig.apiKey) }, - signal: AbortSignal.timeout(requestTimeout) + signal: AbortSignal.timeout(timeout) } ) } catch (err) { if (err.code && err.code === err.TIMEOUT_ERR) { - throw new Error( - `Request to GOV.uk notify timed out after ${requestTimeout}ms` - ) + throw new Error(`Request to GOV.uk notify timed out after ${timeout}ms`) } else { throw new Error( `Request to GOV.uk notify failed with error: ${err.message}` diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index cce9c10..483752d 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -7,12 +7,12 @@ import { config } from '~/src/config/config.js' // })) // const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) -jest.mock( - '~/src/server/common/connectors/notify/notify-token-utils.js', - () => ({ - createToken: jest.fn().mockReturnValue('mocked-jwt-token') - }) -) +// jest.mock( +// '~/src/server/common/connectors/notify/notify-token-utils.js', +// () => ({ +// createToken: jest.fn().mockReturnValue('mocked-jwt-token') +// }) +// ) const testData = { content: 'test' } @@ -70,22 +70,19 @@ describe('sendNotification', () => { }) describe('timeout with steve', () => { - it('should abort if timeout is hit', async () => { - jest.useFakeTimers() - - global.fetch = jest.fn( - () => - new Promise(() => { - // testing - }) - ) + beforeEach(() => { + const notifyConfig = config.get('notify') + config.set('notify', { + ...notifyConfig, + timeout: 0 + }) + }) + it('should abort if timeout is hit', async () => { const result = sendNotification(testData) - jest.advanceTimersByTime(11000) - await expect(result).rejects.toThrow( - 'Request to GOV.uk notify timed out after 10000ms' + 'Request to GOV.uk notify timed out after 0ms' ) jest.useRealTimers() }) From 6cbabef72d5da56508b09048b3875dbc0eb391f2 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:51:43 +0000 Subject: [PATCH 05/17] tests with and without mocks --- .../common/connectors/notify/notify.test.js | 113 +++++++++--------- 1 file changed, 57 insertions(+), 56 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 483752d..2abb2e1 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -2,74 +2,76 @@ import { sendNotification } from './notify.js' import { proxyFetch } from '~/src/server/common/helpers/proxy.js' import { config } from '~/src/config/config.js' -// jest.mock('~/src/server/common/helpers/proxy.js', () => ({ -// proxyFetch: jest.fn() -// })) -// const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) - -// jest.mock( -// '~/src/server/common/connectors/notify/notify-token-utils.js', -// () => ({ -// createToken: jest.fn().mockReturnValue('mocked-jwt-token') -// }) -// ) - const testData = { content: 'test' } describe('sendNotification', () => { - it('should send a notification successfully', async () => { - const mockResponse = { ok: true } - mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) - const response = await sendNotification(testData) + describe('with mocks', () => { + jest.mock('~/src/server/common/helpers/proxy.js', () => ({ + proxyFetch: jest.fn() + })) + const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) - const [url, options] = mockProxyFetch.mock.calls[0] - - expect(url).toBe( - 'https://api.notifications.service.gov.uk/v2/notifications/email' + jest.mock( + '~/src/server/common/connectors/notify/notify-token-utils.js', + () => ({ + createToken: jest.fn().mockReturnValue('mocked-jwt-token') + }) ) - expect(options.method).toBe('POST') - expect(JSON.parse(options.body)).toEqual({ - personalisation: testData, - template_id: config.get('notify').templateId, - email_address: config.get('notify').caseDeliveryEmailAddress - }) - expect(options.headers).toEqual({ - Authorization: 'Bearer mocked-jwt-token' - }) + it('should send a notification successfully', async () => { + const mockResponse = { ok: true } + mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) + const response = await sendNotification(testData) - expect(response).toEqual(mockResponse) - }) + const [url, options] = mockProxyFetch.mock.calls[0] - it('should throw an error if the response is not ok', async () => { - const mockResponse = { - ok: false, - status: 400, + expect(url).toBe( + 'https://api.notifications.service.gov.uk/v2/notifications/email' + ) - // eslint-disable-next-line @typescript-eslint/require-await - json: async () => ({ - status_code: 400, - errors: [ - { - error: 'BadRequestError', - message: "Can't send to this recipient using a team-only API key" - }, - { - error: 'BadRequestError', - message: - "Can't send to this recipient when service is in trial mode" - } - ] + expect(options.method).toBe('POST') + expect(JSON.parse(options.body)).toEqual({ + personalisation: testData, + template_id: config.get('notify').templateId, + email_address: config.get('notify').caseDeliveryEmailAddress + }) + expect(options.headers).toEqual({ + Authorization: 'Bearer mocked-jwt-token' }) - } - mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) - await expect(sendNotification(testData)).rejects.toThrow( - "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" - ) + expect(response).toEqual(mockResponse) + }) + + it('should throw an error if the response is not ok', async () => { + const mockResponse = { + ok: false, + status: 400, + + // eslint-disable-next-line @typescript-eslint/require-await + json: async () => ({ + status_code: 400, + errors: [ + { + error: 'BadRequestError', + message: "Can't send to this recipient using a team-only API key" + }, + { + error: 'BadRequestError', + message: + "Can't send to this recipient when service is in trial mode" + } + ] + }) + } + mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) + + await expect(sendNotification(testData)).rejects.toThrow( + "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" + ) + }) }) - describe('timeout with steve', () => { + describe('without mocks', () => { beforeEach(() => { const notifyConfig = config.get('notify') config.set('notify', { @@ -84,7 +86,6 @@ describe('sendNotification', () => { await expect(result).rejects.toThrow( 'Request to GOV.uk notify timed out after 0ms' ) - jest.useRealTimers() }) }) }) From c70fd8360f8a0271bd058d75a013a1abc4d012fd Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 14:53:32 +0000 Subject: [PATCH 06/17] mocking token function in second describe --- src/server/common/connectors/notify/notify.test.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 2abb2e1..60f21fe 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -5,7 +5,7 @@ import { config } from '~/src/config/config.js' const testData = { content: 'test' } describe('sendNotification', () => { - describe('with mocks', () => { + describe('with mocked proxyFetch', () => { jest.mock('~/src/server/common/helpers/proxy.js', () => ({ proxyFetch: jest.fn() })) @@ -71,7 +71,14 @@ describe('sendNotification', () => { }) }) - describe('without mocks', () => { + describe('without mocked proxyFetch', () => { + jest.mock( + '~/src/server/common/connectors/notify/notify-token-utils.js', + () => ({ + createToken: jest.fn().mockReturnValue('mocked-jwt-token') + }) + ) + beforeEach(() => { const notifyConfig = config.get('notify') config.set('notify', { From 477b30919df17e68cd9fa53e7b7f73fae0863c0a Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 15:01:48 +0000 Subject: [PATCH 07/17] moved mocked create token function to top --- .../common/connectors/notify/notify.test.js | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 60f21fe..5c6d2b2 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -4,6 +4,13 @@ import { config } from '~/src/config/config.js' const testData = { content: 'test' } +jest.mock( + '~/src/server/common/connectors/notify/notify-token-utils.js', + () => ({ + createToken: jest.fn().mockReturnValue('mocked-jwt-token') + }) +) + describe('sendNotification', () => { describe('with mocked proxyFetch', () => { jest.mock('~/src/server/common/helpers/proxy.js', () => ({ @@ -11,13 +18,6 @@ describe('sendNotification', () => { })) const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) - jest.mock( - '~/src/server/common/connectors/notify/notify-token-utils.js', - () => ({ - createToken: jest.fn().mockReturnValue('mocked-jwt-token') - }) - ) - it('should send a notification successfully', async () => { const mockResponse = { ok: true } mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) @@ -72,13 +72,6 @@ describe('sendNotification', () => { }) describe('without mocked proxyFetch', () => { - jest.mock( - '~/src/server/common/connectors/notify/notify-token-utils.js', - () => ({ - createToken: jest.fn().mockReturnValue('mocked-jwt-token') - }) - ) - beforeEach(() => { const notifyConfig = config.get('notify') config.set('notify', { From 80e86e28682698f904d3152822a71d425d1a2ab5 Mon Sep 17 00:00:00 2001 From: Steve Coleman-Williams Date: Wed, 22 Jan 2025 15:22:30 +0000 Subject: [PATCH 08/17] mocked fetch --- .../common/connectors/notify/notify.test.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 5c6d2b2..a3d4f75 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -1,5 +1,5 @@ import { sendNotification } from './notify.js' -import { proxyFetch } from '~/src/server/common/helpers/proxy.js' +import * as proxyFetchObject from '~/src/server/common/helpers/proxy.js' import { config } from '~/src/config/config.js' const testData = { content: 'test' } @@ -13,29 +13,27 @@ jest.mock( describe('sendNotification', () => { describe('with mocked proxyFetch', () => { - jest.mock('~/src/server/common/helpers/proxy.js', () => ({ - proxyFetch: jest.fn() - })) - const mockProxyFetch = /** @type {jest.Mock} */ (proxyFetch) - it('should send a notification successfully', async () => { const mockResponse = { ok: true } - mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) + const mockProxyFetch = jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) + const response = await sendNotification(testData) - const [url, options] = mockProxyFetch.mock.calls[0] + const [url, { body, method, headers }] = mockProxyFetch.mock.calls[0] expect(url).toBe( 'https://api.notifications.service.gov.uk/v2/notifications/email' ) - expect(options.method).toBe('POST') - expect(JSON.parse(options.body)).toEqual({ + expect(method).toBe('POST') + expect(JSON.parse(body)).toEqual({ personalisation: testData, template_id: config.get('notify').templateId, email_address: config.get('notify').caseDeliveryEmailAddress }) - expect(options.headers).toEqual({ + expect(headers).toEqual({ Authorization: 'Bearer mocked-jwt-token' }) From d436f0f0dc27241335e3d2b7c730d01455483ce5 Mon Sep 17 00:00:00 2001 From: Steve Coleman-Williams Date: Wed, 22 Jan 2025 15:35:03 +0000 Subject: [PATCH 09/17] all tests passing --- .../common/connectors/notify/notify.test.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index a3d4f75..d3edf47 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -21,19 +21,24 @@ describe('sendNotification', () => { const response = await sendNotification(testData) - const [url, { body, method, headers }] = mockProxyFetch.mock.calls[0] + const [url, options] = mockProxyFetch.mock.calls[0] + /** + * @type {string | undefined} + */ + // @ts-expect-error: options.body might note be a string + const body = options.body expect(url).toBe( 'https://api.notifications.service.gov.uk/v2/notifications/email' ) - expect(method).toBe('POST') - expect(JSON.parse(body)).toEqual({ + expect(options.method).toBe('POST') + expect(JSON.parse(body ?? '')).toEqual({ personalisation: testData, template_id: config.get('notify').templateId, email_address: config.get('notify').caseDeliveryEmailAddress }) - expect(headers).toEqual({ + expect(options.headers).toEqual({ Authorization: 'Bearer mocked-jwt-token' }) @@ -61,7 +66,9 @@ describe('sendNotification', () => { ] }) } - mockProxyFetch.mockImplementation(() => Promise.resolve(mockResponse)) + jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) await expect(sendNotification(testData)).rejects.toThrow( "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" @@ -71,6 +78,7 @@ describe('sendNotification', () => { describe('without mocked proxyFetch', () => { beforeEach(() => { + jest.restoreAllMocks() const notifyConfig = config.get('notify') config.set('notify', { ...notifyConfig, From 96b388d2ab9ac33d6960035ed59a4a89d02c4d19 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 16:23:08 +0000 Subject: [PATCH 10/17] Unit test to check correct timeout set --- src/server/common/connectors/notify/notify.js | 22 ++++++------- .../common/connectors/notify/notify.test.js | 33 +++++++++++++++---- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/server/common/connectors/notify/notify.js b/src/server/common/connectors/notify/notify.js index 8f3e732..1c2c866 100644 --- a/src/server/common/connectors/notify/notify.js +++ b/src/server/common/connectors/notify/notify.js @@ -6,6 +6,9 @@ import { createToken } from '~/src/server/common/connectors/notify/notify-token- * @typedef {{ content: string}} NotifyContent */ +export const NOTIFY_URL = + 'https://api.notifications.service.gov.uk/v2/notifications/email' + /** * @param {NotifyContent} data */ @@ -21,17 +24,14 @@ export async function sendNotification(data) { let response try { - response = await proxyFetch( - 'https://api.notifications.service.gov.uk/v2/notifications/email', - { - method: 'POST', - body, - headers: { - Authorization: 'Bearer ' + createToken(notifyConfig.apiKey) - }, - signal: AbortSignal.timeout(timeout) - } - ) + response = await proxyFetch(NOTIFY_URL, { + method: 'POST', + body, + headers: { + Authorization: 'Bearer ' + createToken(notifyConfig.apiKey) + }, + signal: AbortSignal.timeout(timeout) + }) } catch (err) { if (err.code && err.code === err.TIMEOUT_ERR) { throw new Error(`Request to GOV.uk notify timed out after ${timeout}ms`) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index d3edf47..1f1ffbf 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -1,4 +1,4 @@ -import { sendNotification } from './notify.js' +import { NOTIFY_URL, sendNotification } from './notify.js' import * as proxyFetchObject from '~/src/server/common/helpers/proxy.js' import { config } from '~/src/config/config.js' @@ -28,9 +28,7 @@ describe('sendNotification', () => { // @ts-expect-error: options.body might note be a string const body = options.body - expect(url).toBe( - 'https://api.notifications.service.gov.uk/v2/notifications/email' - ) + expect(url).toBe(NOTIFY_URL) expect(options.method).toBe('POST') expect(JSON.parse(body ?? '')).toEqual({ @@ -79,14 +77,37 @@ describe('sendNotification', () => { describe('without mocked proxyFetch', () => { beforeEach(() => { jest.restoreAllMocks() + }) + + it('should call proxyFetch passing the correct timeout', async () => { + const expectedTimeout = 10000 + const fetchSpy = jest.spyOn(proxyFetchObject, 'proxyFetch') + const abortSignalTimeoutSpy = jest.spyOn(global.AbortSignal, 'timeout') + + try { + await sendNotification(testData) + } catch (e) { + // ignore error + } finally { + expect(abortSignalTimeoutSpy).toHaveBeenCalledWith(expectedTimeout) + + const mockSignal = abortSignalTimeoutSpy.mock.results[0].value + expect(fetchSpy).toHaveBeenCalledWith( + NOTIFY_URL, + expect.objectContaining({ + signal: mockSignal + }) + ) + } + }) + + it('should abort if timeout is hit', async () => { const notifyConfig = config.get('notify') config.set('notify', { ...notifyConfig, timeout: 0 }) - }) - it('should abort if timeout is hit', async () => { const result = sendNotification(testData) await expect(result).rejects.toThrow( From d7f29f128659cc2c655f8ccc2858c7419be0ca01 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Wed, 22 Jan 2025 16:43:10 +0000 Subject: [PATCH 11/17] Added another test --- src/server/common/connectors/notify/notify.test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 1f1ffbf..529d7ba 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -72,6 +72,17 @@ describe('sendNotification', () => { "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" ) }) + + it('should throw an error on reject', async () => { + const errorMessage = 'test error' + jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.reject(new Error(errorMessage))) + + await expect(sendNotification(testData)).rejects.toThrow( + `Request to GOV.uk notify failed with error: ${errorMessage}` + ) + }) }) describe('without mocked proxyFetch', () => { From 5ab66706c36d7c6d5bf4d8cbe649bc8bfa20a1db Mon Sep 17 00:00:00 2001 From: Hugh FD Jackson Date: Thu, 23 Jan 2025 06:23:39 +0000 Subject: [PATCH 12/17] DSFAAP-747: ensure proxyFetch is mocked Without a `mockImplementation` call, jest will delegate to the underlying proxyFetch implementation, causing an error to be thrown (presumably on a bad request to gateway). There's no need for a real HTTP request to be issued for this test, so mocking it is a cleaner approach. --- .../common/connectors/notify/notify.test.js | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 529d7ba..c38c20a 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -92,24 +92,22 @@ describe('sendNotification', () => { it('should call proxyFetch passing the correct timeout', async () => { const expectedTimeout = 10000 - const fetchSpy = jest.spyOn(proxyFetchObject, 'proxyFetch') + const mockResponse = { ok: true } + const fetchSpy = jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) const abortSignalTimeoutSpy = jest.spyOn(global.AbortSignal, 'timeout') - try { - await sendNotification(testData) - } catch (e) { - // ignore error - } finally { - expect(abortSignalTimeoutSpy).toHaveBeenCalledWith(expectedTimeout) - - const mockSignal = abortSignalTimeoutSpy.mock.results[0].value - expect(fetchSpy).toHaveBeenCalledWith( - NOTIFY_URL, - expect.objectContaining({ - signal: mockSignal - }) - ) - } + await sendNotification(testData) + expect(abortSignalTimeoutSpy).toHaveBeenCalledWith(expectedTimeout) + + const mockSignal = abortSignalTimeoutSpy.mock.results[0].value + expect(fetchSpy).toHaveBeenCalledWith( + NOTIFY_URL, + expect.objectContaining({ + signal: mockSignal + }) + ) }) it('should abort if timeout is hit', async () => { From 530bee0e236cd8901537e9a76084421311bd1d59 Mon Sep 17 00:00:00 2001 From: Hugh FD Jackson Date: Thu, 23 Jan 2025 06:29:14 +0000 Subject: [PATCH 13/17] DSFAAP-747: make tests responsible for their own cleanup --- src/server/common/connectors/notify/notify.test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index c38c20a..558cc64 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -12,6 +12,10 @@ jest.mock( ) describe('sendNotification', () => { + afterEach(() => { + jest.restoreAllMocks() + }) + describe('with mocked proxyFetch', () => { it('should send a notification successfully', async () => { const mockResponse = { ok: true } @@ -83,12 +87,6 @@ describe('sendNotification', () => { `Request to GOV.uk notify failed with error: ${errorMessage}` ) }) - }) - - describe('without mocked proxyFetch', () => { - beforeEach(() => { - jest.restoreAllMocks() - }) it('should call proxyFetch passing the correct timeout', async () => { const expectedTimeout = 10000 @@ -109,7 +107,9 @@ describe('sendNotification', () => { }) ) }) + }) + describe('without mocked proxyFetch', () => { it('should abort if timeout is hit', async () => { const notifyConfig = config.get('notify') config.set('notify', { From b30509fe494841d28888c4bbb93ff482256aa0e4 Mon Sep 17 00:00:00 2001 From: Hugh FD Jackson Date: Thu, 23 Jan 2025 06:32:38 +0000 Subject: [PATCH 14/17] DSFAAP-747: separate integration tests clearly An integration test is a substantially different kind of test, so I think it makes sense to separate them out using another convention. --- .../notify/notify.integration.test.js | 19 ++ .../common/connectors/notify/notify.test.js | 180 ++++++++---------- 2 files changed, 100 insertions(+), 99 deletions(-) create mode 100644 src/server/common/connectors/notify/notify.integration.test.js diff --git a/src/server/common/connectors/notify/notify.integration.test.js b/src/server/common/connectors/notify/notify.integration.test.js new file mode 100644 index 0000000..1184629 --- /dev/null +++ b/src/server/common/connectors/notify/notify.integration.test.js @@ -0,0 +1,19 @@ +import { sendNotification } from './notify.js' +import { config } from '~/src/config/config.js' + +describe('sendNotification (integration)', () => { + it('should abort if the configured timeout is hit', async () => { + const notifyConfig = config.get('notify') + config.set('notify', { + ...notifyConfig, + timeout: 0 + }) + const testData = { content: 'test' } + + const result = sendNotification(testData) + + await expect(result).rejects.toThrow( + 'Request to GOV.uk notify timed out after 0ms' + ) + }) +}) diff --git a/src/server/common/connectors/notify/notify.test.js b/src/server/common/connectors/notify/notify.test.js index 558cc64..7f32a65 100644 --- a/src/server/common/connectors/notify/notify.test.js +++ b/src/server/common/connectors/notify/notify.test.js @@ -16,112 +16,94 @@ describe('sendNotification', () => { jest.restoreAllMocks() }) - describe('with mocked proxyFetch', () => { - it('should send a notification successfully', async () => { - const mockResponse = { ok: true } - const mockProxyFetch = jest - .spyOn(proxyFetchObject, 'proxyFetch') - .mockImplementation(() => Promise.resolve(mockResponse)) - - const response = await sendNotification(testData) - - const [url, options] = mockProxyFetch.mock.calls[0] - /** - * @type {string | undefined} - */ - // @ts-expect-error: options.body might note be a string - const body = options.body - - expect(url).toBe(NOTIFY_URL) - - expect(options.method).toBe('POST') - expect(JSON.parse(body ?? '')).toEqual({ - personalisation: testData, - template_id: config.get('notify').templateId, - email_address: config.get('notify').caseDeliveryEmailAddress - }) - expect(options.headers).toEqual({ - Authorization: 'Bearer mocked-jwt-token' - }) - - expect(response).toEqual(mockResponse) + it('should send a notification successfully', async () => { + const mockResponse = { ok: true } + const mockProxyFetch = jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) + + const response = await sendNotification(testData) + + const [url, options] = mockProxyFetch.mock.calls[0] + /** + * @type {string | undefined} + */ + // @ts-expect-error: options.body might note be a string + const body = options.body + + expect(url).toBe(NOTIFY_URL) + + expect(options.method).toBe('POST') + expect(JSON.parse(body ?? '')).toEqual({ + personalisation: testData, + template_id: config.get('notify').templateId, + email_address: config.get('notify').caseDeliveryEmailAddress }) - - it('should throw an error if the response is not ok', async () => { - const mockResponse = { - ok: false, - status: 400, - - // eslint-disable-next-line @typescript-eslint/require-await - json: async () => ({ - status_code: 400, - errors: [ - { - error: 'BadRequestError', - message: "Can't send to this recipient using a team-only API key" - }, - { - error: 'BadRequestError', - message: - "Can't send to this recipient when service is in trial mode" - } - ] - }) - } - jest - .spyOn(proxyFetchObject, 'proxyFetch') - .mockImplementation(() => Promise.resolve(mockResponse)) - - await expect(sendNotification(testData)).rejects.toThrow( - "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" - ) - }) - - it('should throw an error on reject', async () => { - const errorMessage = 'test error' - jest - .spyOn(proxyFetchObject, 'proxyFetch') - .mockImplementation(() => Promise.reject(new Error(errorMessage))) - - await expect(sendNotification(testData)).rejects.toThrow( - `Request to GOV.uk notify failed with error: ${errorMessage}` - ) + expect(options.headers).toEqual({ + Authorization: 'Bearer mocked-jwt-token' }) - it('should call proxyFetch passing the correct timeout', async () => { - const expectedTimeout = 10000 - const mockResponse = { ok: true } - const fetchSpy = jest - .spyOn(proxyFetchObject, 'proxyFetch') - .mockImplementation(() => Promise.resolve(mockResponse)) - const abortSignalTimeoutSpy = jest.spyOn(global.AbortSignal, 'timeout') - - await sendNotification(testData) - expect(abortSignalTimeoutSpy).toHaveBeenCalledWith(expectedTimeout) - - const mockSignal = abortSignalTimeoutSpy.mock.results[0].value - expect(fetchSpy).toHaveBeenCalledWith( - NOTIFY_URL, - expect.objectContaining({ - signal: mockSignal - }) - ) - }) + expect(response).toEqual(mockResponse) }) - describe('without mocked proxyFetch', () => { - it('should abort if timeout is hit', async () => { - const notifyConfig = config.get('notify') - config.set('notify', { - ...notifyConfig, - timeout: 0 + it('should throw an error if the response is not ok', async () => { + const mockResponse = { + ok: false, + status: 400, + + // eslint-disable-next-line @typescript-eslint/require-await + json: async () => ({ + status_code: 400, + errors: [ + { + error: 'BadRequestError', + message: "Can't send to this recipient using a team-only API key" + }, + { + error: 'BadRequestError', + message: + "Can't send to this recipient when service is in trial mode" + } + ] }) + } + jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) + + await expect(sendNotification(testData)).rejects.toThrow( + "HTTP failure from GOV.uk notify: status 400 with the following errors: Can't send to this recipient using a team-only API key, Can't send to this recipient when service is in trial mode" + ) + }) - const result = sendNotification(testData) + it('should throw an error on reject', async () => { + const errorMessage = 'test error' + jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.reject(new Error(errorMessage))) - await expect(result).rejects.toThrow( - 'Request to GOV.uk notify timed out after 0ms' - ) - }) + await expect(sendNotification(testData)).rejects.toThrow( + `Request to GOV.uk notify failed with error: ${errorMessage}` + ) + }) + + it('should call proxyFetch passing the correct timeout', async () => { + const expectedTimeout = 10000 + const mockResponse = { ok: true } + const fetchSpy = jest + .spyOn(proxyFetchObject, 'proxyFetch') + .mockImplementation(() => Promise.resolve(mockResponse)) + const abortSignalTimeoutSpy = jest.spyOn(global.AbortSignal, 'timeout') + + await sendNotification(testData) + expect(abortSignalTimeoutSpy).toHaveBeenCalledWith(expectedTimeout) + + const mockSignal = abortSignalTimeoutSpy.mock.results[0].value + expect(fetchSpy).toHaveBeenCalledWith( + NOTIFY_URL, + expect.objectContaining({ + signal: mockSignal + }) + ) }) }) From bfff44207cffed53d343c3700f5d8e18f21bee54 Mon Sep 17 00:00:00 2001 From: Hugh FD Jackson Date: Thu, 23 Jan 2025 06:42:20 +0000 Subject: [PATCH 15/17] DSFAAP-747: avoid mutating config object --- .../connectors/notify/notify.integration.test.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/server/common/connectors/notify/notify.integration.test.js b/src/server/common/connectors/notify/notify.integration.test.js index 1184629..aa7bf46 100644 --- a/src/server/common/connectors/notify/notify.integration.test.js +++ b/src/server/common/connectors/notify/notify.integration.test.js @@ -3,11 +3,19 @@ import { config } from '~/src/config/config.js' describe('sendNotification (integration)', () => { it('should abort if the configured timeout is hit', async () => { - const notifyConfig = config.get('notify') - config.set('notify', { - ...notifyConfig, + const configGet = config.get.bind(config) + const notifyConfig = { + ...config.get('notify'), timeout: 0 + } + jest.spyOn(config, 'get').mockImplementation((name) => { + if (name === 'notify') { + return notifyConfig + } else { + return configGet(name) + } }) + const testData = { content: 'test' } const result = sendNotification(testData) From 5020199f6c0b48b5b8b01c9b2fa8cecd3c1a4328 Mon Sep 17 00:00:00 2001 From: Hugh FD Jackson Date: Thu, 23 Jan 2025 06:43:56 +0000 Subject: [PATCH 16/17] DSFAAP-747: make unit for timeout clear --- src/config/config.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index ed9bd93..b659cc7 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -235,9 +235,9 @@ export const config = convict({ env: 'NOTIFY_CASE_DELIVERY_EMAIL_ADDRESS' }, timeout: { - doc: 'Timeout for notify requests', + doc: 'Timeout for notify requests in milliseconds', format: Number, - default: 10000, + default: 10_000, env: 'NOTIFY_TIMEOUT' } } From 61efa17e040764eaf035ec36e0ef5f1dbb4bdc83 Mon Sep 17 00:00:00 2001 From: Jose Luis Garcia Ramos <11478086+joseluisgraa@users.noreply.github.com> Date: Thu, 23 Jan 2025 10:21:55 +0000 Subject: [PATCH 17/17] Mock create token function --- .../common/connectors/notify/notify.integration.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/server/common/connectors/notify/notify.integration.test.js b/src/server/common/connectors/notify/notify.integration.test.js index aa7bf46..4902eec 100644 --- a/src/server/common/connectors/notify/notify.integration.test.js +++ b/src/server/common/connectors/notify/notify.integration.test.js @@ -1,6 +1,13 @@ import { sendNotification } from './notify.js' import { config } from '~/src/config/config.js' +jest.mock( + '~/src/server/common/connectors/notify/notify-token-utils.js', + () => ({ + createToken: jest.fn().mockReturnValue('mocked-jwt-token') + }) +) + describe('sendNotification (integration)', () => { it('should abort if the configured timeout is hit', async () => { const configGet = config.get.bind(config)