Skip to content

Commit

Permalink
Add missing SubmitSendBillRunService unit tests (#851)
Browse files Browse the repository at this point in the history
https://eaflood.atlassian.net/browse/WATER-4390

In [Migrate legacy send bill run functionality](#771) we replaced the mad-as-a-box-of-frogs legacy send functionality with our own!

Only our supposed leader of tech and keeper of quality @Cruikshanks completely missed adding unit tests for the primary `SubmitSendBillRunService`! 🤦

This change adds the missing tests.

---

Note the change in the `SubmitSendService`.

We have been working under the impression that we can return early from an async function by not awaiting the final function.

Whilst in theory this does appear to work it's probably not how we should be doing it. This is because for this service (`SubmitCancelService` is the other place we are following this pattern) we are purposefully throwing an error assuming the `try/catch` in the controller will see it and log it.

When it came to trying to write a unit test for it we found we just kept blowing up [Hapi/Lab](https://hapi.dev/module/lab/). So, we then tried it for real, forcing `ChargingModuleViewBillRunRequest` to throw an error. We confirmed the controller's try/catch doesn't see the error and we caused 'system' to crash.

The only way to catch the error is to chain `.catch()` directly onto the call. We were only intending to log the error so what we're doing is now fine. But it is worth noting and we may need to revisit our foreground/background pattern.
  • Loading branch information
Cruikshanks authored Mar 24, 2024
1 parent d7761cd commit 6dabcfa
Show file tree
Hide file tree
Showing 3 changed files with 195 additions and 9 deletions.
9 changes: 8 additions & 1 deletion app/services/bill-runs/submit-cancel-bill-run.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,14 @@ async function go (billRunId) {

await _updateStatusToCancel(billRunId)

_cancelBillRun(billRun)
// NOTE: We originally believed that should anything error in _cancelBillRun() the try/catch in the controller would
// catch it. However, when testing this theory we found it crashed the test framework. So, we tried it for real and
// again confirmed we'd crash the service. The controller would never see the error. It is because we are not awaiting
// this call that any errors thrown are considered uncaught. However, if we instead use the ES6 variant the error _is_
// caught. All we can do at this point is log it.
_cancelBillRun(billRun).catch((error) => {
global.GlobalNotifier.omfg(error.message, { billRunId }, error)
})
}

async function _cancelBillRun (billRun) {
Expand Down
18 changes: 10 additions & 8 deletions app/services/bill-runs/submit-send-bill-run.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,19 @@ async function go (billRunId) {
const billRun = await _fetchBillRun(billRunId)

if (billRun.status !== 'ready') {
return
throw new ExpandedError('Cannot send a bill run that is not ready', { billRunId })
}

await _updateStatus(billRunId, 'sending')

_sendBillRun(billRun)
// NOTE: We originally believed that should anything error in _sendBillRun() the try/catch in the controller would
// catch it. However, when testing this theory we found it crashed the test framework. So, we tried it for real and
// again confirmed we'd crash the service. The controller would never see the error. It is because we are not awaiting
// this call that any errors thrown are considered uncaught. However, if we instead use the ES6 variant the error _is_
// caught. All we can do at this point is log it.
_sendBillRun(billRun).catch((error) => {
global.GlobalNotifier.omfg(error.message, { billRunId }, error)
})
}

/**
Expand Down Expand Up @@ -68,12 +75,7 @@ async function _fetchChargingModuleBillRun (externalId) {
const result = await ChargingModuleViewBillRunRequest.send(externalId)

if (!result.succeeded) {
const error = new ExpandedError(
'Charging Module view bill run request failed',
{ billRunExternalId: externalId, responseBody: result.response.body }
)

throw error
throw new ExpandedError('Charging Module view bill run request failed', { billRunExternalId: externalId })
}

return result.response.body.billRun
Expand Down
177 changes: 177 additions & 0 deletions test/services/bill-runs/submit-send.service.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
'use strict'

// Test framework dependencies
const Lab = require('@hapi/lab')
const Code = require('@hapi/code')
const Sinon = require('sinon')

const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script()
const { expect } = Code

// Test helpers
const { setTimeout } = require('timers/promises')

const BillHelper = require('../../support/helpers/bill.helper.js')
const BillRunHelper = require('../../support/helpers/bill-run.helper.js')
const DatabaseSupport = require('../../support/database.js')
const ExpandedError = require('../../../app/errors/expanded.error.js')

// Things we need to stub
const ChargingModuleSendBillRunRequest = require('../../../app/requests/charging-module/send-bill-run.request.js')
const ChargingModuleViewBillRunRequest = require('../../../app/requests/charging-module/view-bill-run.request.js')

// Thing under test
const SubmitSendBillBunService = require('../../../app/services/bill-runs/submit-send-bill-run.service.js')

describe('Submit Cancel Bill Run service', () => {
// NOTE: introducing a delay in the tests is not ideal. But the service is written such that the send happens in the
// background and is not awaited. We want to confirm things like the records have been updated. But the only way to do
// so is to give the background process time to complete.
const delay = 500

let chargingModuleSendBillRunRequestStub
let chargingModuleViewBillRunRequestStub
let notifierStub

beforeEach(async () => {
await DatabaseSupport.clean()

chargingModuleSendBillRunRequestStub = Sinon.stub(ChargingModuleSendBillRunRequest, 'send').resolves()
chargingModuleViewBillRunRequestStub = Sinon.stub(ChargingModuleViewBillRunRequest, 'send')

// The service depends on GlobalNotifier to have been set. This happens in app/plugins/global-notifier.plugin.js
// when the app starts up and the plugin is registered. As we're not creating an instance of Hapi server in this
// test we recreate the condition by setting it directly with our own stub
notifierStub = { omg: Sinon.stub(), omfg: Sinon.stub() }
global.GlobalNotifier = notifierStub
})

afterEach(() => {
Sinon.restore()
})

describe('when the bill run exists', () => {
let firstBill
let secondBill
let billRun

describe("and its status is 'ready'", () => {
beforeEach(async () => {
billRun = await BillRunHelper.add({ externalId: '8212003e-059a-4cf2-94c2-da9dfae90a80', status: 'ready' })
})

describe('and the /send request to the Charging Module API succeeds', () => {
beforeEach(async () => {
firstBill = await BillHelper.add({ externalId: 'fe7b0de1-e8c0-42f0-ba66-42f816e67c62' })
secondBill = await BillHelper.add({ externalId: '7b15c45e-9166-40a8-9b3f-1ee394f773dd' })

chargingModuleViewBillRunRequestStub.resolves({
succeeded: true,
response: {
body: {
billRun: {
invoices: [
{ id: firstBill.externalId, transactionReference: 'WAI1000429' },
{ id: secondBill.externalId, transactionReference: 'WAI1000428' }
],
transactionFileReference: 'nalwi50031'
}
}
}
})
})

it('sends a request to the Charging Module API to send its copy', async () => {
await SubmitSendBillBunService.go(billRun.id)

expect(chargingModuleSendBillRunRequestStub.called).to.be.true()
})

it('updates the bill run with the Charging Module transaction file reference', async () => {
await SubmitSendBillBunService.go(billRun.id)

await setTimeout(delay)

const refreshedBillRun = await billRun.$query()

expect(refreshedBillRun.transactionFileReference).to.equal('nalwi50031')
})

it('updates the bills with the Charging Module invoice numbers', async () => {
await SubmitSendBillBunService.go(billRun.id)

await setTimeout(delay)

const refreshedFirstBill = await firstBill.$query()
expect(refreshedFirstBill.invoiceNumber).to.equal('WAI1000429')

const refreshedSecondBill = await secondBill.$query()
expect(refreshedSecondBill.invoiceNumber).to.equal('WAI1000428')
})

it('logs the time taken in milliseconds and seconds', async () => {
await SubmitSendBillBunService.go(billRun.id)

await setTimeout(delay)

const logDataArg = notifierStub.omg.firstCall.args[1]

expect(
notifierStub.omg.calledWith('Send bill run complete')
).to.be.true()
expect(logDataArg.timeTakenMs).to.exist()
expect(logDataArg.timeTakenSs).to.exist()
expect(logDataArg.billRunId).to.exist()
})
})

describe('but the request to view the Charging Module bill run fails', () => {
beforeEach(() => {
chargingModuleViewBillRunRequestStub.resolves({
succeeded: false,
response: new Error('CHA go pop')
})
})

it('logs an error', async () => {
await SubmitSendBillBunService.go(billRun.id)

await setTimeout(delay)

const errorLogArgs = notifierStub.omfg.firstCall.args

expect(
notifierStub.omfg.calledWith('Charging Module view bill run request failed')
).to.be.true()
expect(errorLogArgs[1].billRunId).to.exist()
expect(errorLogArgs[2]).to.be.instanceOf(ExpandedError)
expect(errorLogArgs[2].billRunExternalId).to.equal('8212003e-059a-4cf2-94c2-da9dfae90a80')
})
})
})

describe("but its status is not 'ready'", () => {
beforeEach(async () => {
billRun = await BillRunHelper.add({ status: 'sent' })
})

it('throws as error', async () => {
const result = await expect(SubmitSendBillBunService.go(billRun.id))
.to
.reject()

expect(result).to.be.instanceOf(ExpandedError)
expect(result.message).to.equal('Cannot send a bill run that is not ready')
expect(result.billRunId).to.equal(billRun.id)
})
})
})

describe('when the bill run does not exist', () => {
it('throws an error', async () => {
await expect(SubmitSendBillBunService.go('testId'))
.to
.reject()
})
})
})

0 comments on commit 6dabcfa

Please sign in to comment.