Added more tests to test_whitelist.py #565
Conversation
…cts with tensorflow
…unning, either using -S or -D
… current daemon to run it
…y cant be used with the daemon
Added more tests to test_http_analyzer.py
…t_flow_alerts_into_smaller_files Split flow alerts into smaller files
…ct_doh Alert on DoH and ignore conn without DNS when DoH is detected
…is_whitelisted_domain_in_flow()
There was a problem hiding this comment.
Hello @Sekhar-Kumar-Dash i added some comments, please don't start resolving them until i push my refactoring for whitelist.py so we can better test large functions. thanksssss
|
|
||
|
|
||
| @patch("slips_files.common.parsers.config_parser.ConfigParser.whitelist_path") | ||
| def test_read_configuration(mock_config_parser, mock_db): |
There was a problem hiding this comment.
hey @Sekhar-Kumar-Dash here you were patching slips_files.common.parsers.config_parser.ConfigParser only, but in this case, we need to patch the whitelist.path() method, not the class, i fixed it. can you check my previous 3 commits to this PR?
| ), | ||
| } | ||
| mock_db.is_whitelisted_tranco_domain.return_value = False | ||
| assert whitelist.is_whitelisted_attacker(attacker_data) == expected_result |
There was a problem hiding this comment.
in the second test case, how come example.com is the attacker domain, and the get_all_whitelist.return_value has example.com as a whitelisted domain, and yet the expected return value is false? should be whitelisted right? if there's something wrong with the original is_whitelisted_attacker() function, can you tell me?
| ), | ||
| } | ||
| mock_db.is_whitelisted_tranco_domain.return_value = False | ||
| assert whitelist.is_whitelisted_victim(victim_data) == expected_result |
There was a problem hiding this comment.
this has the same issue as my comment in the above function. how come example.com is not whitelisted by is_whitelisted_victim??
There was a problem hiding this comment.
I think the there is something wrong with the actual code let's check it
| whitelist = ModuleFactory().create_whitelist_obj(mock_db) | ||
| mock_db.set_org_info = MagicMock() | ||
| actual_result = whitelist.load_org_domains(org) | ||
| for domain in expected_result: |
There was a problem hiding this comment.
we can call it parsed_domains instead of actual_result, it is more readable.
| "flow_data, whitelist_data, expected_result", | ||
| [ | ||
| ( # testing_is_whitelisted_flow_with_whitelisted_organization_but_ip_or_domain_not_whitelisted | ||
| MagicMock( |
There was a problem hiding this comment.
hey you can use spaces as word separators in comments instead of _ :))
There was a problem hiding this comment.
That's just a mistake I made while typing quickly. Sorry about that.
| ) = whitelist.read_whitelist() | ||
| whitelisted_IPs, whitelisted_domains, whitelisted_orgs, whitelisted_mac = ( | ||
| whitelist.read_whitelist() | ||
| ) |
There was a problem hiding this comment.
i think we can use mock file for testing the read_whitelist function, this is an important one so we probably should have more strict tests for it. for example 2 whitelist test files would be enough, and in these files we can check for expected values, missing columns, missing values in a line, etc. what do you think @Sekhar-Kumar-Dash
There was a problem hiding this comment.
yes you are right
| ], | ||
| ) | ||
| def test_is_whitelisted_flow( | ||
| mock_db, flow_data, whitelist_data, expected_result |
There was a problem hiding this comment.
i will split is_whitelisted_flow() into smaller functions and will let you know so we can better test it, it's very long now and this test case is testing too many different things at once
Okay |
Fixes Issue #564
This pull request adds more tests for whitelist.py