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.

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

Add tests for helpers readSettings #378

Merged
merged 1 commit into from
Oct 11, 2020

Conversation

TonyBrobston
Copy link
Contributor

@TonyBrobston TonyBrobston commented Oct 9, 2020

Fixes #371

Description of changes

In general, I try my best to create small, functional, concise pull requests. I wrote tests to cover all of the current functionality in helpers readSettings. This pull request will serve as a good base to create a follow up pull request adding mustache templating. This pull request will also serve as a safety net, when I add mustache templating, these tests should be unmodified and should still pass.

Let me know what you think of this, I'm definitely open to feedback.

Checklist

Comment on lines 5 to 115

describe("helpers", () => {
const settingsFilePath = `${__dirname}/settings.json`;

beforeEach(() => {
closeSync(openSync(settingsFilePath, 'w'));
});

afterEach(() => {
jest.clearAllMocks();
if (existsSync(settingsFilePath)) {
unlinkSync(settingsFilePath);
}
});

test("Verify can load settings.json", () => {
const serviceName = "Settings";
const expectedSettings = {"foo": "bar"};
writeFileSync(settingsFilePath, JSON.stringify(expectedSettings));

const actualSettings = helpers.readSettings(serviceName, settingsFilePath);

expect(actualSettings).toEqual(expectedSettings);
});

test("Verify cannot load settings.json because it does not exist", () => {
//eslint-disable-next-line no-console
console.log = jest.fn();
const serviceName = "Settings";
unlinkSync(settingsFilePath);

const actualSettings = helpers.readSettings(serviceName, settingsFilePath);

//eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledWith(expect.stringContaining("[Settings] Unable to read the configuration file: ENOENT: no such file or directory"));
expect(actualSettings).toBeNull();
});

test("Verify throws if rawConfig empty", () => {
const serviceName = "Settings";
const expectedSettings = "";
writeFileSync(settingsFilePath, expectedSettings);

expect(() => {helpers.readSettings(serviceName, settingsFilePath)}).toThrow(Error);
});

test("Verify throws with message if rawConfig empty", () => {
const serviceName = "Settings";
const expectedSettings = "";
writeFileSync(settingsFilePath, expectedSettings);

try {
helpers.readSettings(serviceName, settingsFilePath);
} catch (error) {
expect(error.message).toBe(`[${serviceName}] Unable to load configuration file ${settingsFilePath}.`);
}
});

test("Verify throws if json invalid", () => {
const serviceName = "Settings";
const expectedSettings = {};
writeFileSync(settingsFilePath, JSON.stringify(expectedSettings));
const mockAddListener = jest.spyOn(JSONC, 'parse');
mockAddListener.mockImplementation((rawConfig, parseErrors) => {
parseErrors.push({
error: 1,
offset: 2,
length: 3,
});
return {};
});

expect(() => {helpers.readSettings(serviceName, settingsFilePath)}).toThrow(Error);
});

test("Verify throws with message if json invalid", () => {
const serviceName = "Settings";
const expectedSettings = {};
writeFileSync(settingsFilePath, JSON.stringify(expectedSettings));
const mockAddListener = jest.spyOn(JSONC, 'parse');
mockAddListener.mockImplementation((rawConfig, parseErrors) => {
parseErrors = [{
error: 1,
offset: 2,
length: 3,
}];
return {};
});
//eslint-disable-next-line no-console
console.log = jest.fn();

try {
helpers.readSettings(serviceName, settingsFilePath);
} catch (error) {
//eslint-disable-next-line no-console
expect(console.log).toHaveBeenCalledWith(expect.stringContaining("[Settings] 1"));
expect(error.message).toBe(`[${serviceName}] Unable to load configuration file: `);
}
});
});
Copy link
Contributor Author

@TonyBrobston TonyBrobston Oct 9, 2020

Choose a reason for hiding this comment

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

I based a lot of this testing on some previous work I did here: https://github.com/TonyBrobston/logs-to-mqtt-publisher/blob/master/tests/index.test.ts

However, I did my best to follow the current code style of the repo.

Copy link
Owner

Choose a reason for hiding this comment

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

Do you have the Prettier extension installed in VSCode? That'll make sure the style matches everything else automatically.

Copy link
Contributor Author

@TonyBrobston TonyBrobston Oct 9, 2020

Choose a reason for hiding this comment

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

I'm developing in vim.

Also I really just meant like, test vs. it, test naming convention, test file naming convention, etc.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah good old vim. I used to have a vi reference coffee mug.

Can you please run npm run format then and push any test files that get changed? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The next time I'm in front of my computer I'll do this 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danecreekphotography Ok, I ran that.

@TonyBrobston TonyBrobston force-pushed the allow-secrets-in-settings branch from 034fb04 to 0a572e6 Compare October 9, 2020 18:38
@TonyBrobston TonyBrobston force-pushed the allow-secrets-in-settings branch from 0a572e6 to ed321e3 Compare October 9, 2020 18:59
Comment on lines +72 to +76
parseErrors.push({
error: 1,
offset: 2,
length: 3,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried:

parseErrors = [{
  error: 1,
  offset: 2,
  length: 3,
}]

but it seems like parseErrors is readonly or something... as I would console.log(parseErrors) and it would output undefined.

@@ -27,18 +27,17 @@ export function readSettings<T>(serviceName: string, settingsFileName: string):
throw new Error(`[${serviceName}] Unable to load configuration file ${settingsFileName}.`);
}

let parseErrors: JSONC.ParseError[];
const parseErrors: JSONC.ParseError[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed because of https://github.com/danecreekphotography/node-deepstackai-trigger/pull/378/files#r502622394

I wanted to leave this alone, as I'm not sure what will happen at run time of the actual application.

@TonyBrobston TonyBrobston changed the title Allow secrets in settings Add tests for helpers readSettings Oct 9, 2020
Comment on lines +46 to +64
test("Verify throws if rawConfig empty", () => {
const serviceName = "Settings";
const expectedSettings = "";
writeFileSync(settingsFilePath, expectedSettings);

expect(() => {helpers.readSettings(serviceName, settingsFilePath)}).toThrow(Error);
});

test("Verify throws with message if rawConfig empty", () => {
const serviceName = "Settings";
const expectedSettings = "";
writeFileSync(settingsFilePath, expectedSettings);

try {
helpers.readSettings(serviceName, settingsFilePath);
} catch (error) {
expect(error.message).toBe(`[${serviceName}] Unable to load configuration file ${settingsFilePath}.`);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify a bit here. I write two tests, where one makes for sure that the function threw and the other tests the error message. If I deleted the first of the two and left the second, it's possible that the function does not throw and it goes unnoticed because it wouldn't go into the catch and no assertions is considered a successful test, making it a false-positive.

@TonyBrobston
Copy link
Contributor Author

TonyBrobston commented Oct 9, 2020

Alright, I think I'm mostly done making changes.

Lmk if you have any other feedback.

Once we get this merged I'll start to think about what implementing mustache templates will look like. Probably something like:

test("Verify can load settings.json", () => {
  const serviceName = "Settings";
  const secrets = {"someSecret": "bar"};
  writeFileSync(secretsFilePath, JSON.stringify(secrets));
  const settings = {"foo": "{{secret.someSecret}}"};
  writeFileSync(settingsFilePath, JSON.stringify(settings));

  const actualSettings = helpers.readSettings(serviceName, settingsFilePath, secretsFilePath);

  expect(actualSettings).toEqual({"foo": "bar"});
});

Also it looks like I will have to modify all the callers of helpers readSettings, since we'll probably pass secretsFilePath in, just like we do with settingsFilePath.

@TonyBrobston TonyBrobston force-pushed the allow-secrets-in-settings branch from ed321e3 to 70b497b Compare October 11, 2020 14:23
@neilenns
Copy link
Owner

Insert heavy sigh here, looks like my npm run format command doesn't actually touch the tests directory. I'll take care of the formatting in a separate pull request.

@neilenns neilenns merged commit 344db38 into neilenns:main Oct 11, 2020
@TonyBrobston
Copy link
Contributor Author

TonyBrobston commented Oct 13, 2020

@danecreekphotography Are you familiar with Hacktoberfest? Would you mind adding a label of hacktoberfest-accepted to this PR? (since it was accepted and merged) You could also add hacktoberfest as a topic for the entire repo.

Details here on why this is needed this year: https://hacktoberfest.digitalocean.com/hacktoberfest-update
More details on Hacktoberfest: https://hacktoberfest.digitalocean.com/

@neilenns
Copy link
Owner

I added the label to the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

triggerUri exposes plain text Blue Iris Username and Password
2 participants