Merge pull request #65 from Financial-Times/check-request-header-type

Check the type for request headers passed via config

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

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,
+};

lib/errors/n-test-config-error.js

@@ -0,0 +1,18 @@
+const NTestError = require('./n-test-error');
+
+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)
+		if (Error.captureStackTrace) {
+			Error.captureStackTrace(this, NTestConfigError);
+		}
+	}
+
+}
+
+module.exports = NTestConfigError;

lib/errors/n-test-error.js

@@ -0,0 +1,16 @@
+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)
+		if (Error.captureStackTrace) {
+			Error.captureStackTrace(this, NTestError);
+		}
+	}
+
+}
+
+module.exports = NTestError;

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,18 @@ 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;
+				}
+				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`);
 				}
+				this.requestHeaders[header] = this.requestHeaders[header].toString();
 			}
 		}
 

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,12 @@ const verifyUrl = (testPage, browser, tests) => async () => {
 			});
 
 	} catch(err) {
+
+		// We want n-test to stop running if we've thrown an NTestError
+		if (err instanceof NTestError) {
+			throw err;
+		}
+
 		//Sometimes selenium times out. if so - ignore.
 		console.warn(err);
 		results.errors++;

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 => {

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
+		}
+	},
+];

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,
+		}
+	}
+];

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
+
+	});
+
 });