Added more tests to test_vertical_portscans.py #508
Conversation
There was a problem hiding this comment.
Hello @Sekhar-Kumar-Dash Thank you so much for your effort!
I see you spent so much time understanding the code to come up with that many test cases, we appreciate it.
All my comments are related to code style and formatting, i added 1 commit with refactoring that you could learn from, small things like line length and using mark.parametrize could make it easier for anyone reading your code.
Thanks again!:)
tests/test_vertical_portscans.py
Outdated
| IDEACategory, | ||
| Victim, | ||
| Proto, | ||
| Tag |
There was a problem hiding this comment.
You're only using Proto from this import list, why import the following? usually your IDE should warn you about this and can even auto fix them for you so you don't have to worry about it
There was a problem hiding this comment.
Sorry about that. I am actually using VS Code now, so this feature must be disabled. I will fix this mistake and update the pull request.
There was a problem hiding this comment.
Hey no need to apologize. thanks!
| role = "Client" | ||
| type_data = "IPs" | ||
| direction = "Dst" | ||
|
|
There was a problem hiding this comment.
The above 3 lines are unused local variables. usually your IDe should warn you about them or mark them in gray.
There was a problem hiding this comment.
Yes, it is actually getting displayed in a light color. Sorry, I missed that earlier. I am fixing it.
| vertical_ps.get_not_established_dst_ips(protocol, state, profileid, twid) | ||
|
|
||
| def test_check_overlapping_dstports(mock_db): | ||
| vertical_ps = ModuleFactory().create_vertical_portscan_obj(mock_db) |
There was a problem hiding this comment.
Hey @Sekhar-Kumar-Dash I'm having trouble understanding this test
why are you testing overlapping ports? what can go wrong here?
Also i recommend using function docstrings so it's easier to know the answer to the above questions without having to ask you every time :)
There was a problem hiding this comment.
hey @AlyaGomaa The i have included this test is to ensure that the VerticalPortscan module correctly aggregates the packet counts for overlapping destination ports when scanning both TCP and UDP protocols. it is not a critical test we can remove it if you dont want it
am i getting it wrong??
Also thanks for the docstrings suggestion i will keep that in mind for future unittests PRs
Fixes Issue #510
Changes proposed
With this pull request, I have added more tests to test_vertical_portscans.py, which will improve the code coverage
Check List (Check all the applicable boxes)