-
Notifications
You must be signed in to change notification settings - Fork 8
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
#31 - Add Test Coverage #107
Changes from 12 commits
51fed73
8a4c561
a7e516e
7535920
8e02ac0
cce729b
ba5626b
219eb11
cb8a998
a470b82
47d3d36
ad6e528
086e06d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Help Command Execute DMs commands with prefix and descriptions it still looks the same 1`] = ` | ||
"I am here to help! Well...mostly just make you chuckle at this point, let's be honest. | ||
|
||
Here is a list of the commands that we've got right now: | ||
\`\`\` | ||
!one → I am number one. | ||
!two → Two is not just a number. | ||
!blueFish → Not a red fish. | ||
\`\`\`" | ||
`; | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
import Help from '../../src/commands/help'; | ||
import Commands from '../../src/library/commands'; | ||
import { message as mockMessage, MockedMessage } from '../mocks/discord'; | ||
|
||
// TODO: These should be in a factory/mock | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what the best way to do this with Jest is. I know there's a mock folder that can autoload but I need to learn more about that, and probably refactor our current mixed variations of mock directories. Some may not even be considered mocks or need to be reevaluated. Since they are factories, they probably just need their own directory and to be included was needed. I'm just used to beautiful Rspec factories with factorybot < 3 |
||
const oneCommand = { | ||
name: 'one', | ||
description: 'I am number one.', | ||
execute: jest.fn() | ||
}; | ||
|
||
const twoCommand = { | ||
name: 'two', | ||
description: 'Two is not just a number.', | ||
execute: jest.fn() | ||
}; | ||
|
||
const blueFishCommand = { | ||
name: 'blueFish', | ||
description: 'Not a red fish.', | ||
execute: jest.fn() | ||
}; | ||
|
||
const commands = new Commands({ | ||
one: oneCommand, | ||
two: twoCommand, | ||
blueFish: blueFishCommand | ||
}); | ||
|
||
let sendMock: MockedMessage; | ||
let authorSend: MockedMessage; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish there was lazy evaluated variables [like rspec], there are a few JS libraries that'll do this, but then you loose typing and autocomplete/typeahead, so I would prefer this nasty over that. As long as we're defining their typining with TS here then it's much better than the typeless Vanilla JS™ route |
||
beforeEach(() => { | ||
sendMock = jest.fn(); | ||
mockMessage.reply = sendMock; | ||
authorSend = jest.fn(); | ||
// @ts-ignore | ||
mockMessage.author = { | ||
send: authorSend | ||
}; | ||
}); | ||
|
||
describe('Help Command', () => { | ||
describe('Execute', () => { | ||
beforeEach(() => { | ||
Help.execute([], mockMessage, { commands }); | ||
}); | ||
|
||
test('Lets you know to check your DMs', () => { | ||
expect(sendMock).lastCalledWith('sliding into your DMs...'); | ||
}); | ||
|
||
describe('DMs commands with prefix and descriptions', () => { | ||
let message: string; | ||
beforeEach(() => { | ||
message = authorSend.mock.calls[0][0]; | ||
}); | ||
|
||
test('Snarky', () => { | ||
const snark = "I am here to help! Well...mostly just make you chuckle " + | ||
"at this point, let's be honest."; | ||
expect(message).toContain(snark); | ||
}); | ||
|
||
test('Command pretext header', () => { | ||
const pretext = "Here is a list of the commands that we've got right now:"; | ||
expect(message).toContain(pretext); | ||
}); | ||
|
||
test('Code block start', () => { | ||
expect(message).toContain('```\n'); | ||
}); | ||
|
||
test('Code block end', () => { | ||
const lines = message.split('\n'); | ||
const lastLine = lines[lines.length - 1]; | ||
expect(lastLine).toEqual('```'); | ||
}); | ||
|
||
test('it still looks the same', () => { | ||
expect(message).toMatchSnapshot(); | ||
}); | ||
|
||
describe('Commands', () => { | ||
test('one command', () => { | ||
expect(message).toContain('!one'); | ||
}); | ||
|
||
test('one description', () => { | ||
expect(message).toContain(oneCommand.description); | ||
}); | ||
|
||
test('two command', () => { | ||
expect(message).toContain('!two'); | ||
}); | ||
|
||
test('two description', () => { | ||
expect(message).toContain(twoCommand.description); | ||
}); | ||
|
||
test('blueFish command', () => { | ||
expect(message).toContain('!blueFish'); | ||
}); | ||
|
||
test('BlueFish description', () => { | ||
expect(message).toContain(blueFishCommand.description); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,10 +43,24 @@ describe('Search Command', () => { | |
await Search.execute(['dingusy'], mockMessage); | ||
expect(sendMock).lastCalledWith(results.items[0].link); | ||
}); | ||
test('Malformed Response', async () => { | ||
const mockedData = Promise.resolve({ data: {} }); | ||
axiosMock.get.mockResolvedValueOnce(mockedData); | ||
await Search.execute(['NOPE'], mockMessage); | ||
expect(sendMock).lastCalledWith("I'm Sorry Dave, I'm afraid I can't do that..."); | ||
describe('Malformed Response', () => { | ||
let consoleErrorMock: jest.SpyInstance<void, any>; | ||
beforeEach(async () => { | ||
const mockedData = Promise.resolve({ data: {} }); | ||
axiosMock.get.mockResolvedValueOnce(mockedData); | ||
consoleErrorMock = jest.spyOn(console, 'error') | ||
.mockImplementation(() => undefined); // Prevent it from spewing into the test results | ||
await Search.execute(['NOPE'], mockMessage); | ||
}); | ||
afterEach(() => { | ||
consoleErrorMock.mockRestore(); | ||
}); | ||
test('Responds with error message', async () => { | ||
expect(sendMock).lastCalledWith("I'm Sorry Dave, I'm afraid I can't do that..."); | ||
}); | ||
test('Console logs an error', () => { | ||
const errorMessage = "Malformed Google Search Response: {}"; | ||
expect(consoleErrorMock).lastCalledWith(errorMessage); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've flip flopped between vertical spacing on this project. I think last time I removed a lot of it tests, now I'm ready to put it back 😂 Maybe there's some linting that can be done around this 🤔 |
||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
import glob from 'glob'; | ||
import CommandLoader, { ICommandClasses } from '../../src/library/commandLoader'; | ||
import { COMMANDS_PATH_GLOB } from './../../src/library/commands'; | ||
|
||
describe('CommandLoader', () => { | ||
let commandClasses: ICommandClasses; | ||
const files = glob.sync(COMMANDS_PATH_GLOB); | ||
const commandsPathGlob = './src/commands/*.ts'; | ||
const files = glob.sync(commandsPathGlob); | ||
|
||
beforeEach(() => { | ||
commandClasses = CommandLoader.getCommandClasses(files); | ||
|
@@ -16,6 +16,7 @@ describe('CommandLoader', () => { | |
}); | ||
}); | ||
|
||
// More of an integration test against real commands | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this though as it will validate the commands rather than mocking them out. |
||
describe('Class names match their file names', () => { | ||
test('they match their key name', () => { | ||
for (let commandName of Object.keys(commandClasses)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,43 @@ | ||
import { ICommandClasses } from '../../src/library/commandLoader'; | ||
import Commands from '../../src/library/commands'; | ||
import ICommand from '../../src/library/iCommand'; | ||
|
||
// TODO: I feel like this class is too basic to test, | ||
// and ultimately a wrapper for other classes that have been tested | ||
// Might just remove it some day | ||
describe('Commands', () => { | ||
const commands = new Commands(); | ||
test('Has a command', () => { | ||
expect(Object.keys(commands.all).length).toBeGreaterThan(0); | ||
expect(commands.names.length).toBeGreaterThan(0); | ||
let mockHelloCommand: ICommand; | ||
let mockYetAnotherCommand: ICommand; | ||
let mockCommands: ICommandClasses; | ||
let commands: Commands; | ||
|
||
beforeEach(() => { | ||
// TODO: these should probably go into a factory/mock | ||
mockHelloCommand = { | ||
name: 'Hello', | ||
description: 'Hello World', | ||
execute: jest.fn() | ||
}; | ||
mockYetAnotherCommand = { | ||
name: 'YAC', | ||
description: 'Yet Another Command!', | ||
execute: jest.fn() | ||
}; | ||
mockCommands = { | ||
hello: mockHelloCommand, | ||
yac: mockYetAnotherCommand | ||
}; | ||
commands = new Commands(mockCommands); | ||
}); | ||
|
||
test('.names returns command names', () => { | ||
const commandNames = ['hello', 'yac']; | ||
expect(commands.names).toEqual(commandNames); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I should have a test block that says that command names are transformed. I would have to double check the code, but I think that they are turned into lower case, and are converted from kebab case to camel case if I recall correctly. Maybe those tests belong in a different test or are already tested,, would be good to verify. Ultimately it may be implicitly tested here, but I would like a block to explicitly state the business logic behind it for the sake of documentation. |
||
}); | ||
|
||
test('Can fetch a command', () => { | ||
const first = commands.names[0]; | ||
expect(commands.get(first)).not.toBeUndefined(); | ||
const helloCommand = commands.get('hello'); | ||
expect(helloCommand).toBe(mockHelloCommand); | ||
}); | ||
|
||
test('Finds the longest name', () => { | ||
expect(commands.longestNameLength()).toEqual(5); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,11 @@ export default Purge = class { | |
return 'Purges the channel it is called within. Restricted to Board Members and Administrators.'; | ||
} | ||
|
||
public static execute(args: string[], msg: Message, bot: Client) { | ||
public static execute( | ||
args: string[], | ||
msg: Message, | ||
{ client: bot }: { client: Client } | ||
) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rest of this file has a high cognitive complexity (rated 9 by code climate out of a max of 5). Opened #110 |
||
const { guild } = msg; | ||
|
||
/* global bot */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ export default class CommandLoader { | |
} | ||
|
||
private static removeTemplateFile(files: string[]) { | ||
return files.filter(file => file !== './src/commands/_template.ts'); | ||
const commandTemplateFile = './src/commands/_template.ts'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I flip flopped about throwing this into the config file. It was nice to have it centralized, but then I thought it shouldn't really change or be different per-environment, so I decided it can just live in here then. It also doesn't get reused as well so it can live here until that changes. |
||
return files.filter(file => file !== commandTemplateFile); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,15 @@ | ||
import glob from 'glob'; | ||
import config from '../config'; | ||
import CommandLoader, { ICommandClasses } from './commandLoader'; | ||
import { ICommandClasses } from './commandLoader'; | ||
import Command from './iCommand'; | ||
|
||
export const COMMANDS_PATH_GLOB = './src/commands/*.ts'; | ||
|
||
// TODO: debateable whether we even need this wrapper class | ||
/** | ||
* @class Commands | ||
*/ | ||
export default class Commands { | ||
|
||
public readonly all: ICommandClasses; | ||
private commandFiles: string[]; | ||
|
||
constructor() { | ||
this.commandFiles = glob.sync(COMMANDS_PATH_GLOB); | ||
this.all = CommandLoader.getCommandClasses(this.commandFiles); | ||
constructor(commandClasses: ICommandClasses) { | ||
this.all = commandClasses; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is so much cleaner, less cohesion, and testable with Dependency Injection™ 🎉 |
||
} | ||
|
||
get names() { | ||
|
@@ -27,15 +20,10 @@ export default class Commands { | |
return this.all[commandName]; | ||
} | ||
|
||
public longestName() { | ||
public longestNameLength() { | ||
// Find the longest synopsis | ||
return this.names.reduce((max, commandName) => { | ||
commandName = `${config.messagePrefix}${commandName}`; | ||
if (commandName.length + 1 > max) { | ||
max = commandName.length + 1; | ||
} | ||
return max; | ||
}, 0); | ||
const longest = this.names.sort((a, b) => b.length - a.length)[0]; | ||
return longest.length; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
|
||
import { Client, GuildMember, Message, TextChannel } from 'discord.js'; | ||
import glob from 'glob'; | ||
import config from '../config'; | ||
import CommandLoader from './commandLoader'; | ||
import CommandParser from './commandParser'; | ||
import Commands from './commands'; | ||
|
||
const cmdParser = new CommandParser(config.messagePrefix); | ||
const commands = new Commands(); | ||
const commandsPathGlob = './src/commands/*.ts'; | ||
const commandFiles = glob.sync(commandsPathGlob); | ||
const commandClasses = CommandLoader.getCommandClasses(commandFiles); | ||
const commands = new Commands(commandClasses); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 for Dependency Injection 🎈 |
||
|
||
export default class Core { | ||
|
||
|
@@ -32,7 +36,10 @@ export default class Core { | |
try { | ||
const command = commands.get(commandName); | ||
if (command) { | ||
return command.execute(args, msg, this.client); | ||
return command.execute(args, msg, { | ||
client: this.client, | ||
commands | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working towards optional parameters. This is the only place that passes the data down, but not all commands utilize all the parameters, so I'm working to get these into an object and have the interface allow them all to be optional for the commands that are implementing it. An issue related to this has been opened at: #109 |
||
} | ||
else { | ||
return channel.send(`Command not found: ${commandName}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { Client, Message } from "discord.js"; | ||
import Commands from "./commands"; | ||
|
||
/** | ||
* An interface for all commands to extend, representing the API that all | ||
|
@@ -9,5 +10,8 @@ import { Client, Message } from "discord.js"; | |
export default interface ICommand { | ||
readonly name: string; | ||
readonly description: string; | ||
execute(args: string[], msg: Message, client?: Client): void; | ||
execute(args: string[], msg: Message, extra?: { | ||
client?: Client, | ||
commands?: Commands | ||
}): void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving towards the optional interface related to #109. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I've been told the war stories of snapshots, this is nice because it gives us an idea what it actually looks like, and covers a lot of small semantics that would otherwise be a ton of tests to write. I'm still going to keep the other unit tests in place though. I flip flopped between whether or not to do snapshots, but for now I'll do mixed even though there's some redundancies.