Add ClientState and tests

[MatthewDorner]

Summary

• Generate random ClientState when creating a new subscription
• Tests to verify that ClientState is checked for incoming webhooks
• Fixed bug in mscalendar/notification.go involving conditional call to MakeClient
• Fixed logging typos

Ticket Link

Fixes #13

[MatthewDorner]

I had to copy the definition of the webhook struct type into my test since it's defined in the msgraph package (in remote/msgraph/handle_webhook.go) and isn't exported, and I need it for my test remote.Notification object. What's the correct solution to this? Just change it to be exported?

[mickmister]

I had to copy the definition of the webhook struct type into my test since it's defined in the msgraph package (in remote/msgraph/handle_webhook.go) and isn't exported, and I need it for my test remote.Notification object. What's the correct solution to this? Just change it to be exported?

@MatthewDorner We can create a mock of remote.Notification to be used in the tests. We will need to add a line here to generate the mock, then run make mock in your terminal. Then we can use the mock in tests.

Example init

Example usage

[MatthewDorner]

Never mind, I guess I didn't even need that field for my test.

[MatthewDorner]

Should I also be using MockLogger in the test? It's outputting log messages into the test output, so I'm guessing I should mock it. I just used &bot.TestLogger{TB: t} since I saw it in other tests.

EDIT: Or bot.NilLogger?

[mickmister]

@MatthewDorner I would say that you don't need to mock it unless you want to check the logger's output. This can cause brittle tests that need to be unnecessarily updated if there are more logging additions later.

I think the difference between TestLogger and NilLogger is whether or not you want the logs to show up in the test run's output. 0/5 NilLogger sounds like a fine choice.

[mickmister]

@MatthewDorner Great job on implementing this! No functional issues found, mostly just some suggestions on tests.

[MatthewDorner]

Anything else needed for this PR? I believe all requested changes should be addressed.

[mickmister]

@MatthewDorner Thanks for addressing the testing concerns, LGTM!

[mickmister]

@DHaussermann Testing here will be a smoke test for setting up a subscription and verifying that we can successfully process a notification.

[mickmister]

/update-branch

[mickmister]

@DHaussermann I think we're good to merge this and test on master

[DHaussermann]

Agreed. Thanks @mickmister
This can be covered with smoke testing post merge.