Added more tests to test_slips_utils.py #524
Conversation
There was a problem hiding this comment.
hey @Sekhar-Kumar-Dash i will re-review your PR after using @pytest.mark.parametrize everywhere,
you can use it whenever you start calling the same function you're testing more than once in the same unit tests with different params.
you can read more about it here
https://docs.pytest.org/en/7.1.x/how-to/parametrize.html#pytest-mark-parametrize-parametrizing-test-functions
Thanks for the hard work! we appreciate it.
…ts test_slips_utils.py
Hey @AlyaGomaa , Thanks for the suggestion , I will use parametrize and update this PR soon. I will also keep this in mind for future unit test PRs. First, I thought you wanted me to use parametrize for each test you commented, but now I understand that you want me to combine similar tests into one using parametrize. Please ignore my recent changes to this PR |
|
hey @AlyaGomaa just updated the PR please take a look |
|
Hello @Sekhar-Kumar-Dash the unit tests for slips_utils.py are failing |
|
hey @AlyaGomaa I was not getting these errors locally, but I fixed it by looking into the assertion errors |
# Conflicts: # modules/threat_intelligence/threat_intelligence.py
| assert utils.detect_data_type(data) == expected_type | ||
|
|
||
|
|
||
| def test_get_first_octet(): |
There was a problem hiding this comment.
mark.parametrize can be used to improve this function
There was a problem hiding this comment.
hey @Sekhar-Kumar-Dash this comment wasnt resolved right? you didn't use mark.parametrize in test_get_first_octet()
|
|
||
| def test_convert_format(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert ( |
There was a problem hiding this comment.
we can use mark.parametrize here
There was a problem hiding this comment.
also this comment wasnt resolved @Sekhar-Kumar-Dash
|
|
||
| def test_assert_microseconds(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert utils.assert_microseconds("1680788096.789") == "1680788096.789000" |
There was a problem hiding this comment.
we can use mark.parametrize here
There was a problem hiding this comment.
This comment wasnt resolved
|
|
||
| def test_is_private_ip(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert utils.is_private_ip(ipaddress.ip_address("192.168.1.1")) |
There was a problem hiding this comment.
we can mark.parametrize here. we can use it anywhere where there's repetition. you can check the rest of the file, you get the point :'D
There was a problem hiding this comment.
Okay I will use parametrize in this cases and update this PR
| ), | ||
| ], | ||
| ) | ||
| def test_get_own_IPs(side_effect, expected_result): |
There was a problem hiding this comment.
first thing i do when i find uppercase leters in a function name, is refactor the original function name to use lowercase, remember to use the refactor feature in your IDE instead of just renaming the function, to be able to refactor all instances of the function you're refactoring right?
There was a problem hiding this comment.
That will look strange for this one. "IPs" is more readable than "the ips."
There was a problem hiding this comment.
Check this one
https://peps.python.org/pep-0008/#function-and-variable-names
we should follow this style as much as possible
get_own_ips is as readable as get_own_IPs in my opinion :'D
|
hey @AlyaGomaa I made the required changes please review it |
There was a problem hiding this comment.
Hello @Sekhar-Kumar-Dash you marked most of my comments as resolved when you didn't do the changes, i recommend you mark them as resolved when you are done making the changes instead of when you read the changes, so you wouldn't forget the comments you read right?
Also quick reminder to pull the latest develop into your PR :))
| assert utils.detect_data_type(data) == expected_type | ||
|
|
||
|
|
||
| def test_get_first_octet(): |
There was a problem hiding this comment.
hey @Sekhar-Kumar-Dash this comment wasnt resolved right? you didn't use mark.parametrize in test_get_first_octet()
|
|
||
| def test_convert_format(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert ( |
There was a problem hiding this comment.
also this comment wasnt resolved @Sekhar-Kumar-Dash
|
|
||
| def test_assert_microseconds(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert utils.assert_microseconds("1680788096.789") == "1680788096.789000" |
There was a problem hiding this comment.
This comment wasnt resolved
|
|
||
| def test_is_private_ip(): | ||
| utils = ModuleFactory().create_utils_obj() | ||
| assert utils.is_private_ip(ipaddress.ip_address("192.168.1.1")) |
| ), | ||
| ], | ||
| ) | ||
| def test_get_own_IPs(side_effect, expected_result): |
There was a problem hiding this comment.
Check this one
https://peps.python.org/pep-0008/#function-and-variable-names
we should follow this style as much as possible
get_own_ips is as readable as get_own_IPs in my opinion :'D
Fixes Issue #523
Changes proposed
This pull request adds more tests for slips_utils.py
Check List (Check all the applicable boxes)