#31 - Add Test Coverage #107

ctsstc · 2020-04-11T20:55:42Z

Changes

Add tests for Help command
Refactor Help command
- Easier to read
- Split out some meat into a new method.
Mute and test console error logging from Search command
Change library/command to use Dependency Injection™
- Better tests, better pizza, Papa Yongs
Add additional tests to library/command
Core now pushes a 3rd parameter that Command can optionally use. The 3rd parameter provides the client and the loaded commands.
- Help command can now utilize the already loaded commands without instantiating a new instance.
  - More Dependency Injection.

Related Issues

Write tests to check all bot commands #31

PS: I hope Bryce sleeps a little better knowing a repository out there in the world is slightly closer to 100% coverage. 🌛

…sts.

## Changes - Use Dependency Injection on Commands for CommandClasses - Add some real tests for Commands - Fully covered! 🎉 - Core now injects an a client and commands into every command - Allows for reuse of commands in help function and other functions of the future :) - Commands now take an optional 3rd parameter that includes: - client - commands - Config - commandsPathGlob now lives in the config - commandTemplateFile has a new home - If we decide to later allow for ENV override on these it will be trivial. - These could probably be removed if they're only used in one place, but I also enjoy the dependency injection for testing purposes. Even the tests are using the real file paths This test is more like an integration test I think. This is fine. - Help command is now more readable and easier to test with dependency injection. - Full coverage ;)

I decided to remove the following from the config: - commandsPathGlob - commandTemplateFile They are only being used in a single place anyways. I also made sure they'll be easy to extract later if we ever need to inject them.

Also add some padding to match the original. Now that I can see what it looks like in the snap shot I can easily check :)

Seems to just be my stuff. I need to get my lint extension running on WSL :0!

A larger effort needs to be around this to makes all of these optional since not all commands will utilize all of these in the execute interface.

ctsstc

It's UGE, It's not done, it should have been smaller, but it's ready for a more tested world! ^__^

ctsstc · 2020-04-20T00:50:49Z

__tests__/commands/__snapshots__/help.test.ts.snap

+!two       → Two is not just a number.
+!blueFish  → Not a red fish.
+\`\`\`"
+`;


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.

ctsstc · 2020-04-20T00:56:03Z

__tests__/commands/help.test.ts

+import Commands from '../../src/library/commands';
+import { message as mockMessage, MockedMessage } from '../mocks/discord';
+
+// TODO: These should be in a factory/mock


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
Not sure if there's something like that for Jest...

ctsstc · 2020-04-20T00:57:25Z

__tests__/commands/help.test.ts

+});
+
+let sendMock: MockedMessage;
+let authorSend: MockedMessage;


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

ctsstc · 2020-04-20T00:58:49Z

__tests__/library/commandLoader.test.ts

@@ -16,6 +16,7 @@ describe('CommandLoader', () => {
    });
  });

+  // More of an integration test against real commands


I like this though as it will validate the commands rather than mocking them out.

ctsstc · 2020-04-20T01:01:12Z

__tests__/library/commands.test.ts

+
+  test('.names returns command names', () => {
+    const commandNames = ['hello', 'yac'];
+    expect(commands.names).toEqual(commandNames);


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.

ctsstc · 2020-04-20T01:09:35Z

src/library/core.ts

+const commandsPathGlob = './src/commands/*.ts';
+const commandFiles = glob.sync(commandsPathGlob);
+const commandClasses = CommandLoader.getCommandClasses(commandFiles);
+const commands = new Commands(commandClasses);


🎉 for Dependency Injection 🎈

ctsstc · 2020-04-20T01:11:30Z

src/library/core.ts

+        return command.execute(args, msg, {
+          client: this.client,
+          commands
+        });


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

ctsstc · 2020-04-20T01:12:46Z

src/library/iCommand.ts

+  execute(args: string[], msg: Message, extra?: {
+    client?: Client,
+    commands?: Commands
+  }): void;


Moving towards the optional interface related to #109.
After these tests get merged we can work on getting the args and msg into the object as an optional parameter.

ctsstc · 2020-04-20T01:14:16Z

__tests__/commands/search.test.ts

+    test('Console logs an error', () => {
+      const errorMessage = "Malformed Google Search Response: {}";
+      expect(consoleErrorMock).lastCalledWith(errorMessage);
+    });


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 🤔
Maybe related to #108

ctsstc · 2020-04-20T01:15:13Z

src/library/commands.ts

-    this.commandFiles = glob.sync(COMMANDS_PATH_GLOB);
-    this.all = CommandLoader.getCommandClasses(this.commandFiles);
+  constructor(commandClasses: ICommandClasses) {
+    this.all = commandClasses;


This is so much cleaner, less cohesion, and testable with Dependency Injection™ 🎉

ctsstc added 9 commits April 10, 2020 03:05

Add: Tests for Help Command

51fed73

PS: I hope Bryce sleeps a little better knowing a repository out there in the world is slightly closer to 100% coverage. 🌛

Fix: Prevent console errors from showing in the middle of search's te…

8a4c561

…sts.

Edit: Changed my mind!

7535920

I decided to remove the following from the config: - commandsPathGlob - commandTemplateFile They are only being used in a single place anyways. I also made sure they'll be easy to extract later if we ever need to inject them.

SR: Fix spelling

8e02ac0

SR: Remove unused import

cce729b

SR: Check reference, not deep equals

ba5626b

SR: Add snapshot for help message

219eb11

Also add some padding to match the original. Now that I can see what it looks like in the snap shot I can easily check :)

SR: Remove trailing comma

cb8a998

ctsstc requested a review from BryceGraves April 11, 2020 21:08

ctsstc added 3 commits April 11, 2020 16:18

PR: Lint autofix

a470b82

Seems to just be my stuff. I need to get my lint extension running on WSL :0!

SR: remove unused import

47d3d36

SR: Make linter happy

ad6e528

A larger effort needs to be around this to makes all of these optional since not all commands will utilize all of these in the execute interface.

ctsstc mentioned this pull request Apr 20, 2020

Refactor: Command: Purge - Reduce Cognitive Complexity #110

Open

ctsstc commented Apr 20, 2020

View reviewed changes

Edit: Update version v2.0.4 --> v2.1.0

086e06d

ctsstc mentioned this pull request Apr 20, 2020

RC v2.1.0 #113

Merged

ctsstc merged commit 3d6f57e into master Apr 25, 2020

ctsstc deleted the 31-covered-up branch April 25, 2020 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#31 - Add Test Coverage #107

#31 - Add Test Coverage #107

ctsstc commented Apr 11, 2020 •

edited

Loading

ctsstc left a comment

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

ctsstc Apr 20, 2020

#31 - Add Test Coverage #107

#31 - Add Test Coverage #107

Conversation

ctsstc commented Apr 11, 2020 • edited Loading

Changes

Related Issues

ctsstc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctsstc commented Apr 11, 2020 •

edited

Loading