minds-ai / zoom-drive-connector

Automatically uploads Zoom meeting recordings to Google Drive.
Other
33 stars 8 forks source link

Application Unit Tests #17

Closed MrFlynn closed 5 years ago

MrFlynn commented 5 years ago

This contains all of the unit tests I was able to create so far. There are a couple of suggestions I would like w.r.t. how to test certain things before this PR is merged.

The first question I have is a pretty minor one. Since the DriveAPI class calls the setup() method when it is instantiated, it makes it hard to test the class without actually authorizing against Google's OAuth service. Additionally, there isn't much in the way to test here other than proper calls of exceptions other than going down the rabbit hole to figure out which method intercept or mock when execute() is called. Essentially, is it worth it to spend the time on figuring out how to extensively test this?

The only other comments I would like I will start conversations for below.

Note: This includes commits from the branch review_fixes because I wanted to create tests with those changes in mind. I would that before this branch.

kracekumar commented 5 years ago

Good work by adding test cases!

When a program executes, Python package coverage gives information about the run lines. Coverage tools integrate well with test runners like the nose, py.test. As a result, during the test run, the programmers can be aware of lines that were missing tests, and later try to achieve 100% test code coverage.

I'd suggest using the test coverage along with the test runner. Here is the documentation for integrating with py.test.

One more suggestion is to move all the code, inside <project> or <src> directory. I remember nowadays; we aren't following this pattern. @jbedorf any idea about this?

jbedorf commented 5 years ago

One more suggestion is to move all the code, inside <project> or <src> directory. I remember nowadays; we aren't following this pattern. @jbedorf any idea about this?

That is a good point yes, it makes it easier to turn it into a pip package.

MrFlynn commented 5 years ago

I've looked over the comments and I'll implement the suggestions later today or tomorrow. Thanks for the help everyone!

MrFlynn commented 5 years ago

In terms of moving everything into a src/ directory, I have no preference either way. I don't think we really need to turn this into a pip package because it's an application and not a library (which is more like what pip is for).

MrFlynn commented 5 years ago

I've finished out the remaining unit tests that I can think of. I think this should at least cover most bases for this application for the moment. I think we can come back and use code coverage tools to find whatever is missing later.

jbedorf commented 5 years ago

LGTM!