Make homu testable

[jdm]

Right now homu is built to interact directly with the github API and buildbot. We want to refactor homu so that its hooks produce a list of actions, and those actions can be run separately to actually invoke them. This would be a similar transformation as servo/upstream-wpt-sync-webhook@4b0f370, which is a flask server that interacts with the github API and was redesigned to be more testable.

[alexrs]

I'd like to help with this!

[jdm]

Fantastic! You can see examples of homu in action as the @bors-servo account in #142. A lot of the work happens in the parse_commands method, and you can see examples of the bot immediately acting through calls like add_comment that directly affect the state. We're interested in transforming this code so that's it's easier to test, so we can simulate these actions without actually interacting with github. Does that make sense?

[alexrs]

Yeah, makes sense! I'll start to work on this ASAP

[alexrs]

Hey @jdm. I've been reading the code and I think that it needs a big refactor to be fully testable, although it will be a big project. This is my idea:

• We should divide this issue into smaller issues, starting with testing the current code as much as we can.
• Once the code is tested, we can start with the refactor, moving common logic into classes and extracting some classes to other files.
• Finally, we will be able to test the integration with Github and other third parties.

This approach will allow us to merge code into master while we are working on the refactor, so you would be able to work on new features if needed.

Let me know what do you think about this, and if you think if it's worth investing in a big refactor.

[jdm]

My first question is what kind of testing you believe is possible with the current code. That's always been the biggest challenge, which is why there are no tests yet.

[alexrs]

Yeah, that's definitely tricky. I still have to understand the code better to be able to answer that question properly.

[jdm]

My point of view is that it's possible to do a relatively small amount of refactoring to remove the direct state updates from the current code and turn it into something that we can start writing simple tests for. That's the same goal that you described, so I don't see why we need a separate plan in order to get there :)

[alexrs]

Hey! I'm still working on this. I've extracted some logic from parse_commands to its own methods to be able to mock the state. This is only the first step to improve the code. I've tried to change the code as less as possible, but once this work is done, I'd like to refactor some other parts. I'll try to create a PR soon.
You can find my fork here if you want to take a look: https://github.com/alexrs/homu/tree/testability

[jdm]

That looks like a good start! I didn't realize how mocking could give us more confidence about the current behaviour without requiring a bunch of invasive changes, so thanks for demonstrating that! The only change that I disagree with right now is having a boolean verified argument for all of the methods - the tests for unverified behaviour aren't really useful, since what actually matters is whether the callers pass the appropriate value. I think it would make sense to either reintroduce verification into the methods themselves, or else remove the argument and associated tests entirely.

[alexrs]

Hi!

the tests for unverified behavior aren't really useful

I agree with that, but reintroducing verification into the methods isn't possible because I can't mock the verification function for the tests.
I can remove the argument, but the verification should happen before calling to the method associated with the command we're executing.

By the way, is there any easy way I can deploy my version somewhere to test it?

[jdm]

Is the problem with mocking the verification function because of the partial? If so, we should be able to fix that with a simple change:

def _call_verify_auth(username, repo_cfg, state, auth, realtime, my_username):
   return verify_auth(username, repo_cfg, state, auth, realtime, my_username)

Then make the partials use _call_verify_auth instead of verify_auth, and mocking verify_auth will work as expected.

As for deploying, I suspect heroku is your best best.

[alexrs]

I'll try that!
Anyway, if I understand correctly (and correct me if I'm wrong), the use of partial isn't necessary. We can just call verify_auth with the same arguments we are passing to partial, as the signature of verify_auth is:

def verify_auth(username, repo_cfg, state, auth, realtime, my_username):

And we are passing all those arguments to partial. So we will be able to do the verification into the methods. (That now it's not possible because the partial is done inside parse_commands, so we can't access to _reviewer_auth_verified and _try_auth_verified outside of parse_commands)