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.

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

SLM-85: Added tests using supertest that spin up and test the express application #222

Merged
merged 3 commits into from
Mar 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions integration_tests/mockApis/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const redirect = () =>
'Content-Type': 'text/html',
Location: 'http://localhost:3007/sign-in/callback?code=codexxxx&state=stateyyyy',
},
body: '<html><head><title>SignIn page</title></head><body><h1>Sign in</h1><span class="govuk-visually-hidden" id="sign-in"></span></body></html>',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span in the markup with the pageId has been changed, so these mocks (for pages that are not in our codebase) need to change

body: '<html><head><title>SignIn page</title></head><body><h1>Sign in</h1><span class="govuk-visually-hidden" id="pageId" data-qa="sign-in"></span></body></html>',
},
})

Expand All @@ -74,7 +74,7 @@ const signOut = () =>
headers: {
'Content-Type': 'text/html',
},
body: '<html><head><title>SignIn page</title></head><body><h1>Sign in</h1><span class="govuk-visually-hidden" id="sign-in"></span></body></html>',
body: '<html><head><title>SignIn page</title></head><body><h1>Sign in</h1><span class="govuk-visually-hidden" id="pageId" data-qa="sign-in"></span></body></html>',
},
})

Expand All @@ -89,7 +89,7 @@ const manageDetails = () =>
headers: {
'Content-Type': 'text/html',
},
body: '<html><body><h1>Your account details</h1><span class="govuk-visually-hidden" id="account-details"></span></body></html>',
body: '<html><body><h1>Your account details</h1><span class="govuk-visually-hidden" id="pageId" data-qa="account-details"></span></body></html>',
},
})

Expand Down
2 changes: 1 addition & 1 deletion integration_tests/pages/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export default abstract class Page {
}

checkOnPage = (): void => {
cy.get(`#${this.pageId}`).should('exist')
cy.get('#pageId').should('have.attr', 'data-qa').should('equal', this.pageId)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span with the pageId has changed in the template - see below 👇


checkCsfrTokenForFormBasedPages = (): void => {
Expand Down
18 changes: 17 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@
"json",
"node",
"ts"
]
],
"testRunner": "jest-jasmine2"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to include the jasmine runner to be able to use fail in tests - its basically a bug in jest-circus - see github issue and the line in the description:

Can circumvent in 27.x with testRunner: "jest-jasmine2" in jest.config.js

},
"nodemonConfig": {
"ignore": [
Expand Down Expand Up @@ -168,8 +169,9 @@
"nock": "^13.1.3",
"nodemon": "^2.0.13",
"prettier": "^2.4.1",
"redis-mock": "^0.56.3",
"sass": "^1.42.1",
"supertest": "^6.1.6",
"supertest": "^6.2.2",
"ts-jest": "^27.0.5",
"typescript": "^4.4.4"
}
Expand Down
28 changes: 28 additions & 0 deletions server/routes/barcode/pdf/selectEnvelopeSize.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import SuperTestWrapper from '../../fixtures/SuperTestWrapper'
import assertThat from '../../fixtures/supertest-assertions'

describe('Select envelope size', () => {
const superTest = new SuperTestWrapper()

beforeEach(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a test that requires the user to be authenticated. The test spec itself could do with some more functional tests adding (ie. submitting the envelope size etc), but I wanted to make sure we could send requests as an authenticated user

await superTest.authenticateAsLegalSenderUser()
})

it('should redirect to request a link given user is not authenticated', async () => {
await superTest.unauthenticated()

const response = await superTest.request //
.get('/barcode/pdf/select-envelope-size')

assertThat(response).isOk().hasPageId('request-link').hasNoErrors()
})

it('should redirect to find recipient by prison number given no recipients have been added', async () => {
superTest.request.redirects(2) // two redirects happen with this request/response

const response = await superTest.request //
.get('/barcode/pdf/select-envelope-size')

assertThat(response).isOk().hasPageId('find-recipient-by-prison-number').hasNoErrors()
})
})
40 changes: 40 additions & 0 deletions server/routes/fixtures/SuperTestWrapper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import request from 'supertest'
// eslint-disable-next-line import/no-extraneous-dependencies
import nock from 'nock'
import express from 'express'
import app from '../../index'
import config from '../../config'
import mockHmppsAuth from './mock-hmpps-auth'

jest.mock('redis', () => jest.requireActual('redis-mock'))

export default class SuperTestWrapper {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main class you are going to use when writing supertest tests - basically new up a SuperTestWrapper and use it's request property to issue requests.
It also has helper methods to make the test context authenticated or unauthenticated (need to add a method to authenticate as a mail room user, but that will come ....)

request: request.SuperAgentTest

constructor() {
const application: express.Application = app()
this.request = request.agent(application)
this.request //
.set('Content-Type', 'application/x-www-form-urlencoded')
.redirects(1)
}

authenticateAsLegalSenderUser = async () => {
mockHmppsAuth()
const mockedSendLegalMailApi = nock(config.apis.sendLegalMail.url)
mockedSendLegalMailApi.post('/link/verify', { secret: 'some-secret' }).reply(201, {
token:
'eyJhbGciOiJSUzI1NiJ9.eyJqdGkiOiJkZTE3NGM0Yi0xY2M2LTQxYWYtYTczYi01ZTE2YmI5YzE1ZWIiLCJzdWIiOiJtaWtlLmhhbG1hQGRpZ2l0YWwuanVzdGljZS5nb3YudWsiLCJleHAiOjQ3ODc1Njk3MjF9.WTqNajHRgZCbNe0g20lK5a7s_5-VeWD-FViu6gTgQaEsavimH_wEz1wZ4sj5osCDkCaLIgjYxGFt_p2IAsr7x0pI5b3CenN4_EMrz2pVVxAXOEEI8Q8QVfTy-iBGyO9W95rFGtmxbdsmYpr7LIr6DxJDUCCrCoeH8f4Dl-4QfKLUn-x_9_Bfum1rtAJ38B5pwiwhlzxeHD58C5XIc7swURGpCA97gtog7kEbyrCDF5AkIM4oYC1ViTMfDypnIJaDAU2ggxkaV5EkiIOB386POjUXkePQDnPajX3C-ugbJlKUPHp9z0CL_ngw5iK3wf9mEy2mWi9VHbUnyqVzfhrbIJK2PKQ0Fb8ZJIZhlB_rD68bgpaKskJwGy3lCMqDV5hiK5rUMsw_6n0asdYIhOvrEkXHrwmR4eRfobkLmtXGGRBswWuMhVXbYxBfZPU4PSkReTnbGRxSub-_UmMIvI_CXXaMdyRv0ixG4R3R7HfgLyZiTffN0p8nKmzKDXWWmPVJ',
})

await this.request.get('/link/verify-link?secret=some-secret')
nock.cleanAll()
}

unauthenticated = async () => {
mockHmppsAuth()
await this.request.get('/link/request-link?force=true') // log out legal sender user
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest way to be unauthenticated is to hit the Legal Sender sign out link - it doesnt matter whether you were previously logged in as a LS or not; it will log you out 👍
As and when we have tests that authenticate as a mail room user we'll need to change this to log them out as well

nock.cleanAll()
}
}
12 changes: 12 additions & 0 deletions server/routes/fixtures/mock-hmpps-auth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import nock from 'nock'
import config from '../../config'

export default function mockHmppsAuth() {
const mockedHmppsAuthApi = nock(config.apis.hmppsAuth.url)
mockedHmppsAuthApi.post('/oauth/token').reply(201, {
access_token:
'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImRwcy1jbGllbnQta2V5In0.eyJzdWIiOiJzZW5kLWxlZ2FsLW1haWwtdG8tcHJpc29ucy1jbGllbnQiLCJncmFudF90eXBlIjoiY2xpZW50X2NyZWRlbnRpYWxzIiwic2NvcGUiOlsicmVhZCIsIndyaXRlIl0sImF1dGhfc291cmNlIjoibm9uZSIsImlzcyI6Imh0dHA6Ly9sb2NhbGhvc3Q6OTA5MC9hdXRoL2lzc3VlciIsImV4cCI6MTY0NjY3NDEzMywiYXV0aG9yaXRpZXMiOlsiUk9MRV9TTE1fRU1BSUxfTElOSyIsIlJPTEVfU0xNX1NDQU5fQkFSQ09ERSIsIlJPTEVfU0xNX0NSRUFURV9CQVJDT0RFIl0sImp0aSI6Im1lMk5QcXUzME52MnRncEdGM1hNZ2FIcUpMcyIsImNsaWVudF9pZCI6InNlbmQtbGVnYWwtbWFpbC10by1wcmlzb25zLWNsaWVudCJ9.n2HPheK6qtXHPQLbqmGiDCFMUQK67Nel7GtWZl_rUbe5TOkx27rs2CU8OkgixsUWCD5mfdyoZj23kvMYbiZ3ZDMeOAefKp7FerA3EP81bTVLOKOFUPo_sKFe7jKzNcC4tjcFgeniZ4BS7o0pzK6Lg7iJiEn7rgLuQx1-7XbODK2Y3ylo_0BBvEpkZCdqoC4jvWX8zkKNqmB7_cWyUsiXTpgoHXSizZMECjIU0IoiQeWWKDaUgBqOGCexzMBkT_Prt5qq-hIhZsATllMqb4qTiFPPB5J0aP9xWNivJvGZR83RnjczkJk-a7tQ77SFeODWvURVaKd6w-IKZQYcOHav1Q',
expires_in: 3599,
})
}
69 changes: 69 additions & 0 deletions server/routes/fixtures/supertest-assertions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import * as superagent from 'superagent'
// eslint-disable-next-line import/no-extraneous-dependencies
import cheerio, { CheerioAPI } from 'cheerio'

export default function assertThat(response: superagent.Response) {
return new SupertestAssertions(response)
}

class SupertestAssertions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is effectively our DSL for making assertions. All methods are chainable and is modelled heavily on assertJ (mainly because I couldnt work out how to write my own expect method that didnt conflict with jest's expect)

Also, in most assertion methods I have a try/catch with a fail (re: the change to package.json to bring in jasmine)
Its a bit long winded but its because jest's expect method doesnt support failure messages 😢
Long winded? yes; but its nicely abstracted away into this class, so test methods dont need to worry about it

private $: CheerioAPI

constructor(private readonly response: superagent.Response) {
this.$ = cheerio.load(response.text)
}

isOk = (): SupertestAssertions => {
this.hasStatusCode(200)
return this
}

isNotFound = (): SupertestAssertions => {
this.hasStatusCode(404)
return this
}

hasPageId = (expected: string): SupertestAssertions => {
const actual = this.$('#pageId').attr('data-qa')
try {
expect(actual).toBe(expected)
} catch (err) {
fail(`Expected pageId value to be ${expected} but was ${actual}`)
}
return this
}

hasNoErrors = (): SupertestAssertions => {
const actual = this.$('.govuk-error-summary__list').length
try {
expect(actual).toBe(0)
} catch (err) {
fail(`Expected page to have 0 error messages but had ${actual}`)
}
return this
}

hasError = (expected: string): SupertestAssertions => {
this.hasAllErrors(expected)
return this
}

hasAllErrors = (...expected: string[]): SupertestAssertions => {
const actual = this.$('.govuk-error-summary__list').text()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor point - might be better to split this by newlines before testing? Meh.

try {
expected.forEach(expectedMessage => expect(actual).toContain(expectedMessage))
} catch (err) {
fail(`Expected page to have error[s] containing all of [${expected}] but it did not`)
}
return this
}

hasStatusCode = (expected: number) => {
const actual = this.response.statusCode
try {
expect(actual).toBe(expected)
} catch (err) {
fail(`Expected a status code of ${expected} but was ${actual}`)
}
}
}
5 changes: 3 additions & 2 deletions server/routes/link/VerifyLinkController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ export default class VerifyLinkController {
req.flash('errors', [
{ href: '#email', text: 'The link you used is no longer valid. Request a new one to sign in.' },
])
res.redirect('/link/request-link')
return res.redirect('/link/request-link')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we also need this on line 28? bit confused as to how this ever worked!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll explain it tomorrow, but basically it worked more by luck than judgement, by virtue of the fact that the error message rendered in the flash object for both use cases is the same (lines 26 and 36).
If they had different error messages we would have noticed this bug sooner.
The only reason I noticed it here is that res.render was being called twice (I'll explain why tomorrow) and the supertest http client (superagent) complained bitterly that the response had already been written to and that it couldnt do it again!

}

req.session.validSlmToken = true
req.session.barcodeUserEmail = payload.sub
req.session.barcodeUserOrganisation = payload.organisation
Expand All @@ -46,8 +47,8 @@ export default class VerifyLinkController {
req.session.regenerate(() => {
// put the old session back - but keep the new session ID
Object.assign(req.session, sessionWithoutId)
res.redirect('/barcode/find-recipient')
})
return res.redirect('/barcode/find-recipient')
})
}
}
87 changes: 87 additions & 0 deletions server/routes/link/requestLink.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// eslint-disable-next-line import/no-extraneous-dependencies
import nock from 'nock'
import config from '../../config'
import mockHmppsAuth from '../fixtures/mock-hmpps-auth'
import assertThat from '../fixtures/supertest-assertions'
import SuperTestWrapper from '../fixtures/SuperTestWrapper'

describe('Request Link', () => {
const superTest = new SuperTestWrapper()
let mockedSendLegalMailApi: nock.Scope

beforeEach(() => {
mockHmppsAuth()
mockedSendLegalMailApi = nock(config.apis.sendLegalMail.url)
})

afterEach(() => {
nock.cleanAll()
})

it('should render page to request a link', async () => {
const response = await superTest.request //
Copy link
Contributor

@mikehalmamoj mikehalmamoj Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the // at the end mean anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's just a way I can shoehorn my desire to lay out the code into a more readable manner (IMHO) rather than being at the whim of the draconian prettier rules 😁
To be fair this particular instance is not that offensive because there is only one method call being chained:

const response = await superTest.request //
      .get('/link/request-link')

Without the comment markers prettier would force me to do:

const response = await superTest.request.get('/link/request-link')

That's no so bad, but there are other cases below where I chain more method calls:

const response = await superTest.request //
      .post('/link/request-link')
      .send({ email: '[email protected]' })

If prettier had it's way it would force me to do:

const response = await superTest.request.post('/link/request-link').send({ email: '[email protected]' })

Horrible 🤮

.get('/link/request-link')

assertThat(response).isOk().hasPageId('request-link').hasNoErrors()
})

it('should redirect to email-sent page', async () => {
mockedSendLegalMailApi.post('/link/email', { email: '[email protected]' }).reply(201)

const response = await superTest.request //
.post('/link/request-link')
.send({ email: '[email protected]' })

assertThat(response).isOk().hasPageId('email-sent').hasNoErrors()
})

it('should redirect to request-link page with errors given non CJSM address', async () => {
mockedSendLegalMailApi.post('/link/email', { email: '[email protected]' }).reply(400, {
status: 400,
errorCode: {
code: 'INVALID_CJSM_EMAIL',
userMessage: `Enter an email address which ends with 'cjsm.net'`,
},
})

const response = await superTest.request //
.post('/link/request-link')
.send({ email: '[email protected]' })

assertThat(response).isOk().hasPageId('request-link').hasError('email address in the correct format')
})

it('should redirect to request-link page with errors given email address too long', async () => {
mockedSendLegalMailApi.post('/link/email', { email: '[email protected]' }).reply(400, {
status: 400,
errorCode: {
code: 'EMAIL_TOO_LONG',
userMessage: `The email address can have a maximum length of 254`,
},
})

const response = await superTest.request //
.post('/link/request-link')
.send({ email: '[email protected]' })

assertThat(response).isOk().hasPageId('request-link').hasError('email address in the correct format')
})

it('should redirect to request-link page with errors given unhandled API failure', async () => {
mockedSendLegalMailApi.post('/link/email', { email: '[email protected]' }).reply(404)

const response = await superTest.request //
.post('/link/request-link')
.send({ email: '[email protected]' })

assertThat(response).isOk().hasPageId('request-link').hasError('error generating your sign in link')
})

it('should redirect to request-link page with errors given validation errors', async () => {
const response = await superTest.request //
.post('/link/request-link')
.send({ email: 'an invalid email address' })

assertThat(response).isOk().hasPageId('request-link').hasError('email address in the correct format')
})
})
Loading