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.

Sign up for GitHub

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

Throw error for unknown check types #213

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Throw error for unknown check types #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
caa1283
Add test to demonstrate silently skipping unknown check types
alexmuller Apr 14, 2023
e55ae80
Remove unused test fixture
alexmuller Apr 14, 2023
609f0d2
Throw an error when someone tries to create an unknown check
alexmuller Apr 14, 2023
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
16 changes: 9 additions & 7 deletions src/healthchecks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
);
}
Comment on lines +8 to +12
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: instead of throwing on the first undefined check, this could collect all the undefined checks and throw an error for all of them. would prevent a user playing error whackamole

Copy link
Member

Choose a reason for hiding this comment

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

thought: this would crash the app at startup right? while we'd hope that would be caught by users in local dev, there's still a chance it could creep through to review/staging apps. maybe a safer alternative would be to output a permanently-failing check for undefined checks?


return new healthchecks[check.type](check, this);
});
}

start() {
Expand Down
27 changes: 27 additions & 0 deletions test/fixtures/badConfig/unknownType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'
}
]
};
19 changes: 0 additions & 19 deletions test/fixtures/config/memcheckFixture.js
View file
Open in desktop

This file was deleted.

69 changes: 45 additions & 24 deletions test/healthchecks.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
it('Should throw an error for unknown check types', function () {
Healthchecks = require('../src/healthchecks');
fixture = require('./fixtures/badConfig/unknownType.js');

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);
const newHealthchecks = () => {
new Healthchecks(fixture, require('../src/checks/'));
};

expect(newHealthchecks).to.throw(/Attempted to create/);
});
});
});