Add tests for translator.py #40
Conversation
| @@ -277,6 +281,232 @@ def test_parse_raw_request_no_path(self): | |||
| with self.assertRaises(ValueError): | |||
| translator.parse_raw_request(raw_request) | |||
|
|
|||
| def test_process_arguments_with_no_arguments(self): | |||
| # Arguments | |||
| args = argparse.Namespace( | |||
There was a problem hiding this comment.
This is exactly what I was trying to avoid. Using a hardcoded Namespace means that we'll have to update this in sync with any changes to made to the parser defined in take_arguments. This also prevents testing of the parser.
There was a problem hiding this comment.
Well, this can be avoided by making a function over it, and passing only the required attributes.
Everything else, would be set to None. Then the updation part will be bypassed.
We can write separate tests for parser(which is what principle of unit testing says)
What do you think ?
There was a problem hiding this comment.
I'm not sure I get what you mean. Can you please just code the function you're talking about. 😕
There was a problem hiding this comment.
I'm saying, a function like namespace_loader(**kwargs) to which you provide something like
data="sample data",
request="GET HTTP/1.1 Host: www.google.com".
so, the namespace loader returns you an object with the kwargs provided. rest everything can be set to a default value, which is None here.
There was a problem hiding this comment.
Ah, Yes! That'll surely simplify the code a bit, but still, keeping all this separate from the actual parser feels a bit odd to me, tbh. :/
There was a problem hiding this comment.
you want to check through the complete process, is it ?
There was a problem hiding this comment.
Yes! The parser and the process_arguments functions are too tightly coupled, IMO and testing them separately is a bit weird.
There was a problem hiding this comment.
I would completely agree with you on that point. but still the tests shouldn't depend on them, and my aim was to just lay down some cases. Putting some utility functions like namespace_loader should make these tests independent of the way anything's written. Moreover, as far as your reason is concerned to make it go through the parser. the namespace_loader can be written in such a way that it goes through the parser as well. Modularizing the things will definitely help.
…passed to process_arguments method
| import argparse | ||
|
|
||
|
|
||
| def load_args(**kwargs): |
There was a problem hiding this comment.
@dufferzafar I was talking about something like this.
There was a problem hiding this comment.
Yeah, this definitely improves the code!
There was a problem hiding this comment.
and does it help in solving the problem you stated ?
There was a problem hiding this comment.
Not in its current state, no.
There was a problem hiding this comment.
umm. ok can you describe, what are you expecting ideally ?
|
@dhruvagarwal Is this ready to review? Or should I wait until the second item in your list is checked? |
|
@DePierre yes, this is ready for review. I'll add the tests for take_body in another pull |
| search_regex=None, | ||
| search_string=None | ||
| ) | ||
| for key, value in kwargs.iteritems(): |
|
@dhruvagarwal Do you think that is it important to test for other languages as well? I see that you used |
|
@dhruvagarwal Are you still concerned by your PR? |
These two methods cover a big part of the module. Right now, the test coverage is 77%.