-
-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test tanner-parse-response #192
Conversation
64bb49f
to
ea38aa9
Compare
self.loop.run_until_complete(test()) | ||
expected_header = {"content-type": "multipart/form-data"} | ||
real_result = [self.res1, self.res2, self.res3, self.res4] | ||
expected_result = [b'test.png', 'image/png', expected_header, 200] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use variables self.expected_content
, self.content_type
and assign after detection
. Also I think to maintain consistency we can make self.expected_header
and self.expected_status_code
in setUp method.
@rjt-gupta Can you try to add the remaining cases to make coverage 100%? |
Yes, Im working on it. |
The expected status code is |
Look here. |
Oh yes, I missed this as this part is not covered in the test yet. Will do the change surely :) |
Done. |
|
||
def test_parse_type_one_error(self): | ||
self.detection = {"type": 1} | ||
meta_content = {"/index.html": {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even though I have simulated the test-case using something/
but there is no test that demonstrates this case which is also a possibility.
But your decision would be final, should I remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case need not be handled here, it can be checked in cloner.py
. So I think it's good to keep it simple here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay,
self.detection = {"type": 1} | ||
meta_content = {"/index.html": {}} | ||
self.requested_name = 'something/' | ||
self.handler = TannerHandler(self.args, meta_content, self.uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed too.
@@ -105,17 +185,5 @@ def test_call_handle_html(self): | |||
self.loop.run_until_complete(test()) | |||
self.handler.html_handler.handle_content.assert_called_with(self.call_content) | |||
|
|||
def test_parse_exception(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coverage is reached using new tests so no need for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is for this when detection is empty then raises a keyError
. Since the code to handle is not written (which is because is unlikely to happen), that's why it's not present in the coverage. So I guess no harm in keeping this.
self.detection = { | ||
"type": 2, | ||
"payload": { | ||
"page": "/index.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case also you can simulate the error using
"page": "/index.html", | |
"page": "/something", |
and remove line 142 and line 143
meta_content = {"/index.html": {}} | ||
self.handler = TannerHandler(self.args, meta_content, self.uuid) | ||
self.expected_content = b'<html><body><div>test</div></body></html>' | ||
self.content_type = r'text\html' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be
self.content_type = r'text\html' | |
self.content_type = r'text/html' |
Right?
And can you first make a separate PR to fix this
Line 109 in c049841
content_type = r'text\html' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR submitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it.
After this, how much is the test coverage? |
08574bf
to
0516d72
Compare
Coverage is at 99%. How should I test this line? |
@@ -105,17 +185,5 @@ def test_call_handle_html(self): | |||
self.loop.run_until_complete(test()) | |||
self.handler.html_handler.handle_content.assert_called_with(self.call_content) | |||
|
|||
def test_parse_exception(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is for this when detection is empty then raises a keyError
. Since the code to handle is not written (which is because is unlikely to happen), that's why it's not present in the coverage. So I guess no harm in keeping this.
meta_content = {"/index.html": {}} | ||
self.handler = TannerHandler(self.args, meta_content, self.uuid) | ||
self.expected_content = b'<html><body><div>test</div></body></html>' | ||
self.content_type = r'text\html' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it.
I guess you can make |
0516d72
to
c5e66af
Compare
c5e66af
to
127f99e
Compare
Updated the changes. |
snare/tests/test_submit_data.py
Outdated
from snare.utils.asyncmock import AsyncMock | ||
from snare.tanner_handler import TannerHandler | ||
from snare.utils.page_path_generator import generate_unique_path | ||
|
||
logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you place it in setUp
?
snare/tests/test_submit_data.py
Outdated
logger.error('Error submitting data: JSONDecodeError {}'.format(data)) | ||
|
||
self.handler = mock.Mock() | ||
self.handler.submit_data = mock_json_decode_error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use this instead of mocking the entire submit_data
method.
snare/tests/test_submit_data.py
Outdated
def test_submit_data_error(self): | ||
|
||
async def mock_json_decode_error(data): | ||
logger.error('Error submitting data: JSONDecodeError {}'.format(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, logger creates a file for errors, is this also doing the same? If so please do the cleanup.
Done. 100% coverage reached. |
Added test for
parse_tanner_response
.Increased test coverage of
tanner_handler.py
to 92%.Refs - #187