From 495c0b26bb88d568b5d345c20bd8d25dfdc4c1c8 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Thu, 31 May 2018 10:27:57 +0100 Subject: [PATCH 1/8] Check the type for request headers passed via config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change also introduces NTest* error classes so that we can add specific error handling for errors thrown by n-test itself. Fixes issue #63. 🐿 v2.8.0 --- lib/errors/index.js | 7 +++++++ lib/errors/n-test-config-error.js | 17 +++++++++++++++++ lib/errors/n-test-error.js | 15 +++++++++++++++ lib/smoke/puppeteer-page.js | 11 +++++++++-- lib/smoke/verify-url.js | 9 +++++++++ 5 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 lib/errors/index.js create mode 100644 lib/errors/n-test-config-error.js create mode 100644 lib/errors/n-test-error.js diff --git a/lib/errors/index.js b/lib/errors/index.js new file mode 100644 index 0000000..4b115f6 --- /dev/null +++ b/lib/errors/index.js @@ -0,0 +1,7 @@ +const NTestError = require('./n-test-error'); +const NTestConfigError = require('./n-test-config-error'); + +module.exports = { + NTestError, + NTestConfigError, +}; diff --git a/lib/errors/n-test-config-error.js b/lib/errors/n-test-config-error.js new file mode 100644 index 0000000..b7fb07d --- /dev/null +++ b/lib/errors/n-test-config-error.js @@ -0,0 +1,17 @@ +const NTestError = require('./n-test-error'); + +class NTestConfigError extends NTestError { + + constructor (...params) { + super(...params); + + // Maintains proper stack trace for where our error was thrown (only available on V8) + // TODO: Check that this is still required + if (Error.captureStackTrace) { + Error.captureStackTrace(this, NTestConfigError); + } + } + +} + +module.exports = NTestConfigError; diff --git a/lib/errors/n-test-error.js b/lib/errors/n-test-error.js new file mode 100644 index 0000000..ca9f89c --- /dev/null +++ b/lib/errors/n-test-error.js @@ -0,0 +1,15 @@ +class NTestError extends Error { + + constructor (...params) { + super(...params); + + // Maintains proper stack trace for where our error was thrown (only available on V8) + // TODO: Check that this is still required + if (Error.captureStackTrace) { + Error.captureStackTrace(this, NTestError); + } + } + +} + +module.exports = NTestError; diff --git a/lib/smoke/puppeteer-page.js b/lib/smoke/puppeteer-page.js index 716e580..ab98aac 100644 --- a/lib/smoke/puppeteer-page.js +++ b/lib/smoke/puppeteer-page.js @@ -7,6 +7,8 @@ const REDIRECT_CODES = [301, 302, 303, 307, 308]; const DIMENSIONS = require('../helpers/dimensions'); +const { NTestConfigError } = require('../errors'); + class PuppeteerPage { constructor (options) { @@ -69,9 +71,14 @@ class PuppeteerPage { if(this.requestHeaders) { for (const header in this.requestHeaders) { - if (this.requestHeaders.hasOwnProperty(header)) { - this.requestHeaders[header] = this.requestHeaders[header].toString(); + if (!this.requestHeaders.hasOwnProperty(header)) { + continue; + } + const requestHeaderType = typeof this.requestHeaders[header]; + if (['null', 'undefined'].includes(requestHeaderType)) { + throw new NTestConfigError(`The configured request header ${header} has an invalid type (${requestHeaderType}), string expected`); } + this.requestHeaders[header] = this.requestHeaders[header].toString(); } } diff --git a/lib/smoke/verify-url.js b/lib/smoke/verify-url.js index a5bbd2a..0254f8b 100644 --- a/lib/smoke/verify-url.js +++ b/lib/smoke/verify-url.js @@ -1,6 +1,8 @@ /* eslint-disable no-console */ const chalk = require('chalk'); +const { NTestError } = require('../errors'); + const verifyUrl = (testPage, browser, tests) => async () => { let results = { @@ -44,6 +46,13 @@ const verifyUrl = (testPage, browser, tests) => async () => { }); } catch(err) { + + // We want n-test to stop running if we've encountered an error + if (err instanceof NTestError) { + // TODO: Is there anything we need to explicitly cleanup before rethrowing? + throw err; + } + //Sometimes selenium times out. if so - ignore. console.warn(err); results.errors++; From 0adbcd112771b1f5fd9f36c32fb926a4dda212fd Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Thu, 31 May 2018 10:36:07 +0100 Subject: [PATCH 2/8] Fix option descriptions - smoke.js, not smoke.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- tasks/smoke.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tasks/smoke.js b/tasks/smoke.js index 948b4a8..3595121 100644 --- a/tasks/smoke.js +++ b/tasks/smoke.js @@ -12,10 +12,10 @@ module.exports = (program) => { .option('-a, --auth', 'Authenticate with FT_NEXT_BACKEND_KEY') .option('-b, --browsers [value]', 'Selenium browsers to run the test against') .option('-H, --host [value]', 'Set the hostname to use for all tests') - .option('-c, --config [value]', 'Path to config file used to test. Defaults to ./test/smoke.json') + .option('-c, --config [value]', 'Path to config file used to test. Defaults to ./test/smoke.js') .option('-i, --interactive [value]', 'Interactively choose which tests to run. Defaults to false') .option('--header [value]', 'Request headers to be sent with every request. e.g. "X-Api-Key: 1234"', collectHeaders, []) - .description('Tests that a given set of urls for an app respond as expected. Expects the config file ./test/smoke.json to exist') + .description('Tests that a given set of urls for an app respond as expected. Expects the config file ./test/smoke.js to exist') .action((sets, opts) => { const globalHeaders = {}; opts.header.forEach(header => { From ba55eed17a77d5529ec7bfbb4a3e9e0720e6b106 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Fri, 29 Jun 2018 17:00:56 +0100 Subject: [PATCH 3/8] Remove redundant comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- lib/smoke/verify-url.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/smoke/verify-url.js b/lib/smoke/verify-url.js index 0254f8b..fb55bba 100644 --- a/lib/smoke/verify-url.js +++ b/lib/smoke/verify-url.js @@ -49,7 +49,6 @@ const verifyUrl = (testPage, browser, tests) => async () => { // We want n-test to stop running if we've encountered an error if (err instanceof NTestError) { - // TODO: Is there anything we need to explicitly cleanup before rethrowing? throw err; } From b97e6e1aef5138bdcb319ca559788713829713e7 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Wed, 11 Jul 2018 17:19:02 +0100 Subject: [PATCH 4/8] Change Makefile tasks so you can run unit tests by themselves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index d913557..c9a0b55 100644 --- a/Makefile +++ b/Makefile @@ -6,8 +6,9 @@ node_modules/@financial-times/n-gage/index.mk: # The reason for passing --runInBand to jest in CI: # https://facebook.github.io/jest/docs/en/troubleshooting.html#tests-are-extremely-slow-on-docker-and-or-continuous-integration-ci-server -test: +unit-test: export TEST_SESSIONS_URL=https://fuhn0pye67.execute-api.eu-west-1.amazonaws.com/prod; \ export TEST_SESSIONS_API_KEY=mock-api-key; \ jest test/tasks/*.js --forceExit $(if $(CI), --ci --runInBand --testResultsProcessor="jest-junit", ) - make verify + +test: verify unit-test From 009c0179f8da855e731809413eb2be4782d21191 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Thu, 12 Jul 2018 14:28:27 +0100 Subject: [PATCH 5/8] Remove TODOs and ensure that error name is correctly set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- lib/errors/n-test-config-error.js | 3 ++- lib/errors/n-test-error.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/errors/n-test-config-error.js b/lib/errors/n-test-config-error.js index b7fb07d..9dd0bca 100644 --- a/lib/errors/n-test-config-error.js +++ b/lib/errors/n-test-config-error.js @@ -5,8 +5,9 @@ class NTestConfigError extends NTestError { constructor (...params) { super(...params); + this.name = this.constructor.name; + // Maintains proper stack trace for where our error was thrown (only available on V8) - // TODO: Check that this is still required if (Error.captureStackTrace) { Error.captureStackTrace(this, NTestConfigError); } diff --git a/lib/errors/n-test-error.js b/lib/errors/n-test-error.js index ca9f89c..3dd9316 100644 --- a/lib/errors/n-test-error.js +++ b/lib/errors/n-test-error.js @@ -3,8 +3,9 @@ class NTestError extends Error { constructor (...params) { super(...params); + this.name = this.constructor.name; + // Maintains proper stack trace for where our error was thrown (only available on V8) - // TODO: Check that this is still required if (Error.captureStackTrace) { Error.captureStackTrace(this, NTestError); } From 6515772ab172f44757512b59ff3bb674125d5304 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Thu, 12 Jul 2018 14:29:09 +0100 Subject: [PATCH 6/8] Handle the fact that `typeof null === 'object'` (yay, JavaScript!) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- lib/smoke/puppeteer-page.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/smoke/puppeteer-page.js b/lib/smoke/puppeteer-page.js index ab98aac..b18eaf9 100644 --- a/lib/smoke/puppeteer-page.js +++ b/lib/smoke/puppeteer-page.js @@ -74,7 +74,11 @@ class PuppeteerPage { if (!this.requestHeaders.hasOwnProperty(header)) { continue; } - const requestHeaderType = typeof this.requestHeaders[header]; + let requestHeaderType = typeof this.requestHeaders[header]; + if (this.requestHeaders[header] === null) { + requestHeaderType = 'null'; + } + if (['null', 'undefined'].includes(requestHeaderType)) { throw new NTestConfigError(`The configured request header ${header} has an invalid type (${requestHeaderType}), string expected`); } From 3cf493aa21fb21cce88be88ec04e16b85db231a0 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Thu, 12 Jul 2018 15:05:37 +0100 Subject: [PATCH 7/8] Tweak comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- lib/smoke/verify-url.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/smoke/verify-url.js b/lib/smoke/verify-url.js index fb55bba..acf74d5 100644 --- a/lib/smoke/verify-url.js +++ b/lib/smoke/verify-url.js @@ -47,7 +47,7 @@ const verifyUrl = (testPage, browser, tests) => async () => { } catch(err) { - // We want n-test to stop running if we've encountered an error + // We want n-test to stop running if we've thrown an NTestError if (err instanceof NTestError) { throw err; } From 275f38e50b302f8b24694d0c5a7e1dfc96c9b123 Mon Sep 17 00:00:00 2001 From: Simon Plenderleith Date: Fri, 13 Jul 2018 13:00:46 +0100 Subject: [PATCH 8/8] Add tests for validation of config headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🐿 v2.8.0 --- test/fixtures/smoke-invalid-header.js | 23 +++++++++++++ test/fixtures/smoke-valid-headers.js | 11 ++++++ test/tasks/smoke.test.js | 48 +++++++++++++++++++++++++-- 3 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/smoke-invalid-header.js create mode 100644 test/fixtures/smoke-valid-headers.js diff --git a/test/fixtures/smoke-invalid-header.js b/test/fixtures/smoke-invalid-header.js new file mode 100644 index 0000000..971f1f5 --- /dev/null +++ b/test/fixtures/smoke-invalid-header.js @@ -0,0 +1,23 @@ +module.exports = [ + { + // All valid headers + headers: { + 'ft-valid-header-number': 1, + 'ft-valid-header-string': 'some-string', + }, + urls: { + '/status?1': 200 + } + }, + { + // Contains an invalid header (undefined) + headers: { + 'ft-valid-header-number': 1, + 'ft-valid-header-string': 'some-string', + 'ft-invalid-header-undefined': process.env.UNDEFINED_HEADER_VARIABLE, + }, + urls: { + '/status?2': 200 + } + }, +]; diff --git a/test/fixtures/smoke-valid-headers.js b/test/fixtures/smoke-valid-headers.js new file mode 100644 index 0000000..1fed5d3 --- /dev/null +++ b/test/fixtures/smoke-valid-headers.js @@ -0,0 +1,11 @@ +module.exports = [ + { + headers: { + 'ft-valid-header-number': 1, + 'ft-valid-header-string': 'some-string', + }, + urls: { + '/status/200?1': 200, + } + } +]; diff --git a/test/tasks/smoke.test.js b/test/tasks/smoke.test.js index 07dfb7a..a8fe048 100644 --- a/test/tasks/smoke.test.js +++ b/test/tasks/smoke.test.js @@ -2,6 +2,7 @@ const server = require('../server/app'); const SmokeTest = require('../../lib/smoke/smoke-test'); +const { NTestConfigError } = require('../../lib/errors'); const { spawn } = require('child_process'); describe('Smoke Tests of the Smoke', () => { @@ -11,7 +12,8 @@ describe('Smoke Tests of the Smoke', () => { server.listen(3004); }); - describe('status checks', () => { + describe('Status checks', () => { + test('tests should pass if all the urls return the correct status', (done) => { const smoke = new SmokeTest({ host: 'http://localhost:3004', @@ -38,9 +40,10 @@ describe('Smoke Tests of the Smoke', () => { done(); }); }, 10000); + }); - describe('tests that error', () => { + describe('Tests that error', () => { test('should handle non-assertion errors gracefully beyond a threshold', (done) => { const smoke = new SmokeTest({ @@ -72,10 +75,10 @@ describe('Smoke Tests of the Smoke', () => { }); - }); describe('Adding custom checks', () => { + test('should allow adding custom assertions',(done) => { const smoke = new SmokeTest({ @@ -99,10 +102,12 @@ describe('Smoke Tests of the Smoke', () => { done(); }); }); + }); //TODO: figure out how to test the www.ft.com bit! describe('Session tokens', () => { + test('should not run user-based tests on localhost', () => { const smoke = new SmokeTest({ @@ -119,6 +124,7 @@ describe('Smoke Tests of the Smoke', () => { }); describe('CLI task', () => { + test('should exit with the correct code for a passing test', (done) => { const proc = spawn('./bin/n-test.js', ['smoke', '--host', 'http://localhost:3004', '--config', 'test/fixtures/smoke-pass.js']); proc.on('close', (code) => { @@ -134,5 +140,41 @@ describe('Smoke Tests of the Smoke', () => { done(); }); }, 10000); + }); + + describe('Config validation', () => { + + test('tests should run and pass when all headers in a config are valid', (done) => { + + const smoke = new SmokeTest({ + host: 'http://localhost:3004', + config: 'test/fixtures/smoke-valid-headers.js' + }); + + return smoke.run().then((results) => { + expect(results.passed.length).toEqual(1); + expect(results.failed.length).toEqual(0); + done(); + }); + + }, 10000); + + test('should throw an NTestConfigError when config contains an invalid header', async () => { + + const smoke = new SmokeTest({ + host: 'http://localhost:3004', + config: 'test/fixtures/smoke-invalid-header.js' + }); + + await expect(smoke.run()).rejects.toThrowError(NTestConfigError); + + }, 10000); + + // TODO: Add test for a null header once functionality to pass config + // object has been added i.e. config for these validation tests can be + // multiple exports in one config file + + }); + });