From 2d98bf6c5d6aa80295f079138dfb8c6564019318 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 8 Jun 2021 15:33:04 +0100 Subject: [PATCH 1/3] Restore all mocks after each test This is easier than re-assigning the mock functions manually, as we're reusing Jest's in-built behaviour. Because all the mocks are restored, we need to move the ones we had in the beforeAll block into the beforeEach block. Note: "require('./support/teardown.js')" also resets all Jest mocks, but "require" only runs once, so we can't use it in a beforeEach block [1]. We could do a "jest.resetModules()" to fix that, which seems worse on the whole. I think there's a broader discussion here about whether we could / should have a global reset of Jest mocks after each test - I quickly tried this and it causes some existing tests to fail :-|. [1]: https://stackoverflow.com/questions/48989643/how-to-reset-module-imported-between-tests --- .../authenticateSecurityKey.test.js | 35 +++++++++---------- tests/javascripts/registerSecurityKey.test.js | 18 ++++++---- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 110314325..8e4b04f4c 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -2,40 +2,39 @@ beforeAll(() => { window.CBOR = require('../../node_modules/cbor-js/cbor.js') require('../../app/assets/javascripts/authenticateSecurityKey.js') - // disable console.error() so we don't see it in test output - // you might need to comment this out to debug some failures - jest.spyOn(console, 'error').mockImplementation(() => { }) - - // ensure window.alert() is implemented to simplify errors - jest.spyOn(window, 'alert').mockImplementation(() => { }) + // populate missing values to allow consistent jest.spyOn() + window.fetch = () => { } + window.navigator.credentials = { get: () => { } } }) afterAll(() => { require('./support/teardown.js') + + // restore window attributes to their original undefined state + delete window.fetch + delete window.navigator.credentials }) describe('Authenticate with security key', () => { let button beforeEach(() => { + // disable console.error() so we don't see it in test output + // you might need to comment this out to debug some failures + jest.spyOn(console, 'error').mockImplementation(() => { }) + + // ensure window.alert() is implemented to simplify errors + jest.spyOn(window, 'alert').mockImplementation(() => { }) + document.body.innerHTML = ` - - ` + ` + button = document.querySelector('[data-module="authenticate-security-key"]') - - // populate missing values to allow consistent jest.spyOn() - window.fetch = () => { } - window.navigator.credentials = { get: () => { } } - window.alert = () => { } - window.GOVUK.modules.start() }) afterEach(() => { - // restore window attributes to their original undefined state - delete window.fetch - delete window.navigator.credentials - delete window.alert + jest.restoreAllMocks() }) test('authenticates a credential and redirects based on the admin app response', (done) => { diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 14fea8487..043a84224 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -2,13 +2,6 @@ beforeAll(() => { window.CBOR = require('../../node_modules/cbor-js/cbor.js') require('../../app/assets/javascripts/registerSecurityKey.js') - // disable console.error() so we don't see it in test output - // you might need to comment this out to debug some failures - jest.spyOn(console, 'error').mockImplementation(() => {}) - - // ensure window.alert() is implemented to simplify errors - jest.spyOn(window, 'alert').mockImplementation(() => {}) - // populate missing values to allow consistent jest.spyOn() window.fetch = () => {} window.navigator.credentials = { create: () => {} } @@ -26,6 +19,13 @@ describe('Register security key', () => { let button beforeEach(() => { + // disable console.error() so we don't see it in test output + // you might need to comment this out to debug some failures + jest.spyOn(console, 'error').mockImplementation(() => {}) + + // ensure window.alert() is implemented to simplify errors + jest.spyOn(window, 'alert').mockImplementation(() => {}) + document.body.innerHTML = ` Register a key @@ -35,6 +35,10 @@ describe('Register security key', () => { window.GOVUK.modules.start() }) + afterEach(() => { + jest.restoreAllMocks() + }) + test('creates a new credential and reloads', (done) => { jest.spyOn(window, 'fetch').mockImplementationOnce((_url) => { From 1bb49e5456ec55e3dffe5a2976a612db15c6b6d8 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 10 Jun 2021 14:39:48 +0100 Subject: [PATCH 2/3] Remove redundant spies after assertions We only need to assert on the URL for the subsequent POST back to the server, at which point we can call the test "done()". This is a technique we use in the following tests as well, so we don't need to comment about it here. --- tests/javascripts/authenticateSecurityKey.test.js | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 8e4b04f4c..8c2f11cf5 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -131,23 +131,11 @@ describe('Authenticate with security key', () => { // subsequent POST of credential data to server expect(url.toString()).toEqual( 'https://www.notifications.service.gov.uk/webauthn/authenticate?next=%2Ffoo%3Fbar%3Dbaz' - ); + ); - // mark the test as done here as we've finished all our asserts - if something goes wrong later and - // we end up in the alert mock, that `done(msg)` will override this and mark the test as failed done(); - - const loginResponse = window.CBOR.encode({ redirect_url: '/foo' }) - return Promise.resolve({ - ok: true, arrayBuffer: () => Promise.resolve(loginResponse) - }) }) - // make sure we error out if alert is called - jest.spyOn(window, 'alert').mockImplementation((msg) => { - done(msg) - }) - button.click() }); From a2fb92ab74d937d661dd12b4c1b4a8116d1fcc1a Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 10 Jun 2021 14:48:18 +0100 Subject: [PATCH 3/3] Remove redundant semicolons in ES6 tests --- .../authenticateSecurityKey.test.js | 22 +++++++++---------- tests/javascripts/registerSecurityKey.test.js | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/javascripts/authenticateSecurityKey.test.js b/tests/javascripts/authenticateSecurityKey.test.js index 8c2f11cf5..2b9503331 100644 --- a/tests/javascripts/authenticateSecurityKey.test.js +++ b/tests/javascripts/authenticateSecurityKey.test.js @@ -86,7 +86,7 @@ describe('Authenticate with security key', () => { jest.spyOn(window.location, 'assign').mockImplementation((href) => { expect(href).toEqual("/foo") - done(); + done() }) // this will make the test fail if the alert is called @@ -95,11 +95,11 @@ describe('Authenticate with security key', () => { }) button.click() - }); + }) test('authenticates and passes a redirect url through to the authenticate admin endpoint', (done) => { // https://github.com/facebook/jest/issues/890#issuecomment-415202799 - window.history.pushState({}, 'Test Title', '/?next=%2Ffoo%3Fbar%3Dbaz'); + window.history.pushState({}, 'Test Title', '/?next=%2Ffoo%3Fbar%3Dbaz') jest.spyOn(window, 'fetch') .mockImplementationOnce((_url) => { @@ -131,13 +131,13 @@ describe('Authenticate with security key', () => { // subsequent POST of credential data to server expect(url.toString()).toEqual( 'https://www.notifications.service.gov.uk/webauthn/authenticate?next=%2Ffoo%3Fbar%3Dbaz' - ); + ) - done(); + done() }) button.click() - }); + }) test.each([ ['network'], @@ -179,7 +179,7 @@ describe('Authenticate with security key', () => { }) button.click() - }); + }) test.each([ ['network error'], @@ -226,7 +226,7 @@ describe('Authenticate with security key', () => { }) button.click() - }); + }) test('reloads page if POSTing WebAuthn credentials returns 403', (done) => { @@ -263,7 +263,7 @@ describe('Authenticate with security key', () => { // assert that reload is called and the page is refreshed jest.spyOn(window.location, 'reload').mockImplementation(() => { - done(); + done() }) // this will make the test fail if the alert is called @@ -272,7 +272,7 @@ describe('Authenticate with security key', () => { }) button.click() - }); + }) -}); +}) diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 043a84224..1886935fd 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -49,7 +49,7 @@ describe('Register security key', () => { return Promise.resolve({ ok: true, arrayBuffer: () => webauthnOptions }) - }); + }) jest.spyOn(window.navigator.credentials, 'create').mockImplementation((options) => { expect(options).toEqual('options') @@ -71,7 +71,7 @@ describe('Register security key', () => { expect(decodedData.attestationObject).toEqual(new Uint8Array([1,2,3])) expect(options.headers['X-CSRFToken']).toBe() return Promise.resolve({ ok: true }) - }); + }) jest.spyOn(window.location, 'reload').mockImplementation(() => { // signal that the async promise chain was called