Fix verification tool for later Py/Dj versions and add tests #41
Conversation
| try: | ||
| res = request(url, method.lower(), data=qs, headers=headers) | ||
| except ImportError: | ||
| raise CommandError('To use this command you first need to ' |
There was a problem hiding this comment.
I'd like to see this try/except/raise block moved back to the exact import requests statement within request() because otherwise it could obscure other imports.
For testing purposes you could create a get_requests_module() that does the import so you can mock it out.
There was a problem hiding this comment.
This makes perfect sense. Done.
|
|
||
| def handle(self, *args, **options): | ||
| hawk_log = logging.getLogger('mohawk') | ||
| hawk_log.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
It looks like you removed this. Was the logging too noisy? I had found it helpful in the past because it shows exactly what was used to create MACs, etc. If you still want to remove it, maybe there can be an option to re-enable it?
There was a problem hiding this comment.
Thanks for pointing this out. It actually has more to do with me not fully grasping how getLogger works. At the time, I thought we were creating a new logger and that it wasn't being used, but now it's clear to me that we're listening in on the logger of the underlying mohawk lib.
I added it back in; however, I thought it more conventional to add this logging config to the top of the module beneath the imports.
tests/test_command.py
Outdated
| 'Server-Authorization': 'xyz', | ||
| 'Content-Type': 'text/plain' | ||
| } | ||
| self.text = 'Authorized' |
There was a problem hiding this comment.
This looks like it might be easy enough with a real Response. If it's not easy then faking it is in fine but this is worth a try:
response = Response()
response.headers['Server-Authorization'] = 'xyz'
response.headers['Content-Type'] = 'text/plain'
response._content = 'Authorized'There was a problem hiding this comment.
Simpler, more realistic, and works great. Thank you.
|
|
||
| def handle(self, *args, **options): | ||
| hawk_log = logging.getLogger('mohawk') | ||
| hawk_log.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
Thanks for pointing this out. It actually has more to do with me not fully grasping how getLogger works. At the time, I thought we were creating a new logger and that it wasn't being used, but now it's clear to me that we're listening in on the logger of the underlying mohawk lib.
I added it back in; however, I thought it more conventional to add this logging config to the top of the module beneath the imports.
| try: | ||
| res = request(url, method.lower(), data=qs, headers=headers) | ||
| except ImportError: | ||
| raise CommandError('To use this command you first need to ' |
There was a problem hiding this comment.
This makes perfect sense. Done.
tests/test_command.py
Outdated
| 'Server-Authorization': 'xyz', | ||
| 'Content-Type': 'text/plain' | ||
| } | ||
| self.text = 'Authorized' |
There was a problem hiding this comment.
Simpler, more realistic, and works great. Thank you.
|
|
||
|
|
||
| hawk_log = logging.getLogger('mohawk') | ||
| hawk_log.setLevel(logging.DEBUG) |
There was a problem hiding this comment.
This shouldn't execute at import time because debug logging might get turned on through regular usage of the hawrest middleware which is not what we want. It should only get turned on when the management command script runs so it should move back to the handle() function.
Maybe it would help to add a comment to this code. I'd suggest something like: This will configure the mohawk library for debug logging so you can see inputs to signature functions and other useful stuff.
There was a problem hiding this comment.
Makes sense, I added it back.
tests/test_command.py
Outdated
| @mock.patch('mohawk.Sender.accept_response') | ||
| def test_response_verified_with_auth_header(self, mk_accept, mk_resp): | ||
| response = Response() | ||
| response.headers = { |
There was a problem hiding this comment.
It might not be a big deal but I'd feel better if you re-used the existing dict like this:
response.headers['Server-Authorization'] = 'xyz'
response.headers['Content-Type'] = 'text/plain'Overwriting the dict makes me nervous because it's a customized case-insensitive mapping.
|
Addressed your comments. Thanks for the feedback, @kumar303! |
Resolves #33 and #40 and adds unit tests for the management command