From caa128389f0af1ba99f0c56ff512af417ec719ef Mon Sep 17 00:00:00 2001 From: Alex Muller Date: Fri, 14 Apr 2023 16:11:50 +0100 Subject: [PATCH 1/3] Add test to demonstrate silently skipping unknown check types --- test/fixtures/config/unknownType.js | 27 +++++++++++ test/healthchecks.spec.js | 69 +++++++++++++++++++---------- 2 files changed, 72 insertions(+), 24 deletions(-) create mode 100644 test/fixtures/config/unknownType.js diff --git a/test/fixtures/config/unknownType.js b/test/fixtures/config/unknownType.js new file mode 100644 index 0000000..69c34f5 --- /dev/null +++ b/test/fixtures/config/unknownType.js @@ -0,0 +1,27 @@ +'use strict'; + +module.exports = { + name: 'bad checks with unknown type', + description: '', + checks: [ + { + type: 'some-type-of-check-which-does-not-exist', + name: 'Unknown test', + severity: 3, + businessImpact: 'Something bad', + technicalSummary: 'Type things', + panicGuide: 'No need to panic', + interval: '1m' + }, + { + type: 'graphiteThreshold', + name: 'A Graphite check', + severity: 3, + businessImpact: 'Something bad', + technicalSummary: 'Type things', + panicGuide: 'No need to panic', + interval: '1m', + metric: 'foo.bar' + } + ] +}; diff --git a/test/healthchecks.spec.js b/test/healthchecks.spec.js index 881b98d..0c164fa 100644 --- a/test/healthchecks.spec.js +++ b/test/healthchecks.spec.js @@ -8,36 +8,57 @@ describe('Healthchecks', function () { let fixture; let healthchecks; - before(function () { - Healthchecks = require('../src/healthchecks'); - fixture = require('./fixtures/config/paywall.js'); - healthchecks = new Healthchecks(fixture, require('../src/checks/')); - }); + describe('work correctly', function () { + before(function () { + Healthchecks = require('../src/healthchecks'); + fixture = require('./fixtures/config/paywall.js'); + healthchecks = new Healthchecks(fixture, require('../src/checks/')); + }); + + function extract(obj, props) { + const extracted = {}; + props.forEach(function (prop) { + extracted[prop] = obj[prop]; + }); - function extract(obj, props) { - const extracted = {}; - props.forEach(function (prop) { - extracted[prop] = obj[prop]; + return extracted; + } + + it('Should be able to read in the config object', function () { + const props = ['name', 'description']; + expect(extract(healthchecks, props)).to.deep.equal( + extract(fixture, props) + ); }); - return extracted; - } + it('Should create new checks as described in the config', function () { + expect(healthchecks.checks[0]).to.be.an.instanceOf( + GraphiteThresholdCheck + ); + }); - it('Should be able to read in the config object', function () { - const props = ['name', 'description']; - expect(extract(healthchecks, props)).to.deep.equal(extract(fixture, props)); + it('Should report its status correctly', function () { + const status = healthchecks.getStatus(); + expect(status.name).to.equal(fixture.name); + expect(status.description).to.equal(fixture.description); + expect(status.checks.length).to.equal(1); + expect(status.checks[0].name).to.equal(fixture.checks[0].name); + expect(status.checks[0].panicGuide).to.equal( + fixture.checks[0].panicGuide + ); + }); }); - it('Should create new checks as described in the config', function () { - expect(healthchecks.checks[0]).to.be.an.instanceOf(GraphiteThresholdCheck); - }); + describe('with an unknown type', function () { + before(function () { + Healthchecks = require('../src/healthchecks'); + fixture = require('./fixtures/config/unknownType.js'); + healthchecks = new Healthchecks(fixture, require('../src/checks/')); + }); - it('Should report its status correctly', function () { - const status = healthchecks.getStatus(); - expect(status.name).to.equal(fixture.name); - expect(status.description).to.equal(fixture.description); - expect(status.checks.length).to.equal(1); - expect(status.checks[0].name).to.equal(fixture.checks[0].name); - expect(status.checks[0].panicGuide).to.equal(fixture.checks[0].panicGuide); + it('Should ignore the unknown healthcheck silently', function () { + expect(healthchecks.checks.length).to.equal(1); + expect(healthchecks.checks[0].name).to.equal('A Graphite check'); + }); }); }); From e55ae8013e580a79374cd644eba655f2eb0b78ac Mon Sep 17 00:00:00 2001 From: Alex Muller Date: Fri, 14 Apr 2023 16:14:59 +0100 Subject: [PATCH 2/3] Remove unused test fixture --- test/fixtures/config/memcheckFixture.js | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 test/fixtures/config/memcheckFixture.js diff --git a/test/fixtures/config/memcheckFixture.js b/test/fixtures/config/memcheckFixture.js deleted file mode 100644 index ac4ec5c..0000000 --- a/test/fixtures/config/memcheckFixture.js +++ /dev/null @@ -1,19 +0,0 @@ -'use strict'; -module.exports = { - name: 'memory check fixture', - description: '', - checks: [ - { - type: 'memory', - name: 'test1', - severity: 3, - apps: 'all', - businessImpact: 'blah', - technicalSummary: 'god knows', - panicGuide: "Don't Panic", - interval: '1m', - window: '10m', - threshold: 1 - } - ] -}; From 609f0d20da3c6cdf9525d1f89b565901c80ff4ef Mon Sep 17 00:00:00 2001 From: Alex Muller Date: Fri, 14 Apr 2023 16:29:37 +0100 Subject: [PATCH 3/3] Throw an error when someone tries to create an unknown check --- src/healthchecks.js | 16 +++++++++------- .../{config => badConfig}/unknownType.js | 0 test/healthchecks.spec.js | 14 +++++++------- 3 files changed, 16 insertions(+), 14 deletions(-) rename test/fixtures/{config => badConfig}/unknownType.js (100%) diff --git a/src/healthchecks.js b/src/healthchecks.js index 5af6473..383e1bc 100644 --- a/src/healthchecks.js +++ b/src/healthchecks.js @@ -4,13 +4,15 @@ class HealthChecks { constructor(config, healthchecks) { this.name = config.name; this.description = config.description; - this.checks = config.checks - .filter((check) => { - return check.type in healthchecks; - }) - .map((check) => { - return new healthchecks[check.type](check, this); - }); + this.checks = config.checks.map((check) => { + if (!(check.type in healthchecks)) { + throw new Error( + `Attempted to create check '${check.name}' of type ${check.type} which does not exist` + ); + } + + return new healthchecks[check.type](check, this); + }); } start() { diff --git a/test/fixtures/config/unknownType.js b/test/fixtures/badConfig/unknownType.js similarity index 100% rename from test/fixtures/config/unknownType.js rename to test/fixtures/badConfig/unknownType.js diff --git a/test/healthchecks.spec.js b/test/healthchecks.spec.js index 0c164fa..bb4c4c9 100644 --- a/test/healthchecks.spec.js +++ b/test/healthchecks.spec.js @@ -50,15 +50,15 @@ describe('Healthchecks', function () { }); describe('with an unknown type', function () { - before(function () { + it('Should throw an error for unknown check types', function () { Healthchecks = require('../src/healthchecks'); - fixture = require('./fixtures/config/unknownType.js'); - healthchecks = new Healthchecks(fixture, require('../src/checks/')); - }); + fixture = require('./fixtures/badConfig/unknownType.js'); + + const newHealthchecks = () => { + new Healthchecks(fixture, require('../src/checks/')); + }; - it('Should ignore the unknown healthcheck silently', function () { - expect(healthchecks.checks.length).to.equal(1); - expect(healthchecks.checks[0].name).to.equal('A Graphite check'); + expect(newHealthchecks).to.throw(/Attempted to create/); }); }); });