-
Notifications
You must be signed in to change notification settings - Fork 165
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
Added more tests to test_http_analyzer.py #572
Added more tests to test_http_analyzer.py #572
Conversation
Hey @AlyaGomaa Will this be enough? It is not showing a PEP8 error in PyCharm. Please tell me if further shortening of lines is required. |
Hello @Sekhar-Kumar-Dash thanks for reformatting your code and the awesome PR! quick question, are pre-commit hooks working for you? you can install them using
i highly recommend you set them up in pycharm like so |
Hey @AlyaGomaa , thank you for this great suggestion. I am working on it. I will let you know. |
Hey @AlyaGomaa , I just removed the duplicates manually as I'm having some issues running the pre-commit right now. Please take a look |
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.
Hey @Sekhar-Kumar-Dash Thanks for your PR!
i added a few comments, and will re-review once they're resolved.
also remember to pull my commits before making more modifications to this PR.
thanksss!
], | ||
) | ||
def test_read_configuration(mock_db, mocker, config_value, expected_exception): | ||
http_analyzer = ModuleFactory().create_http_analyzer_obj(mock_db) |
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.
is it better to have the 2 cases as different functions? i think we're testing 2 different behaviours using 1 function.
for example test_read_configuration_valid, and test_read_configuration_with_exception would be better
hey @AlyaGomaa just made the changes please review it |
hey @Sekhar-Kumar-Dash thanks for the changes, i think everything looks good now, i'll merge once the tests pass! |
Fixes Issue #571
This pull request adds more tests for http_analyzer.py