added more tests to test_update_file_manager.py #606
Conversation
tests/test_update_file_manager.py
Outdated
| """ | ||
| update_manager = ModuleFactory().create_update_manager_obj(mock_db) | ||
| file_path = os.path.join( | ||
| os.getcwd(), "slips_files/ports_info/ports_used_by_specific_orgs.csv" |
There was a problem hiding this comment.
it better to avoid using production files in tests, because if this file changed, the test would need to be updated, right?
i think we should patch get_hash_from_file() and patch get_TI_file_info(), and then compare the old_hash and the new_hash. also we should test for both cases, when this function returns true and when it returns false
tests/test_update_file_manager.py
Outdated
| def test_get_feed_details(mocker, mock_db, mock_data, expected_feeds): | ||
| """Test get_feed_details with different file contents.""" | ||
| update_manager = ModuleFactory().create_update_manager_obj(mock_db) | ||
| m = mock_open(read_data=mock_data) |
There was a problem hiding this comment.
alwayssss use descriptive variable names, always always alwayss. new developer will not know what m means. you may not know what m means 2 weeks from now :))
| update_manager = ModuleFactory().create_update_manager_obj(mock_db) | ||
| test_file = tmp_path / "ports_info.csv" | ||
| test_file.write_text(test_data, encoding="utf-8") | ||
| mocker.patch("builtins.open", mock_open(read_data=test_data)) |
There was a problem hiding this comment.
whats the purpose of write_text if we're gonna mock "builtins.open" anyways?
i tried update_manager.read_ports_info(str(tmp_path / "ports_info.csv")) without lines 229 and 230 and the test passed no issue
tests/test_update_file_manager.py
Outdated
| update_manager = ModuleFactory().create_update_manager_obj(mock_db) | ||
| update_manager.new_hash = "test_hash" | ||
| test_file = tmp_path / file_name | ||
| test_file.write_text(test_data, encoding="utf-8") |
There was a problem hiding this comment.
same comment as the above function, this line can be deleted no issue right?
tests/test_update_file_manager.py
Outdated
| update_manager.online_whitelist = "https://example.com/whitelist.txt" | ||
|
|
||
| mock_requests = mocker.patch("requests.get") | ||
| mock_requests.return_value.status_code = 200 |
There was a problem hiding this comment.
hey @Sekhar-Kumar-Dash using this
update_manager.download_file = Mock()
update_manager.download_file.return_value = Trueinstead of
mock_requests = mocker.patch("requests.get")
mock_requests.return_value.status_code = 200is way mor ereadable, because when i was checking this function, i spent a lot of time debugging how come you didn't mock download_file but yet the tests are passing?, it's because you did mock the "requests.get" right?
so always mock the function call, not the calls inside the function call
tests/test_update_file_manager.py
Outdated
| import pytest | ||
| import time | ||
| import os | ||
| from unittest.mock import Mock, mock_open | ||
|
|
||
|
|
||
| def test_getting_header_fields(mocker, mock_db): |
There was a problem hiding this comment.
we can sefly delete this function since you wrote a better versionof it in test_get_e_tag()
| """ | ||
| Test when RiskIQ API returns an error (e.g., invalid API key) | ||
| """ | ||
| update_manager = ModuleFactory().create_update_manager_obj(mock_db) |
tests/test_update_file_manager.py
Outdated
| example.com,Another description | ||
| """ | ||
| test_file = tmp_path / "test.txt" | ||
| test_file.write_text(test_data, encoding="utf-8") |
| mock_db.get_TI_file_info.return_value = {"hash": cached_hash} | ||
| result = update_manager.check_if_update_org(str(test_file)) | ||
| assert result is expected_result | ||
|
|
There was a problem hiding this comment.
here i would mock get_TI_file_info() and utils.get_hash_from_file() instead of creating a file and writing to it
| result = update_manager.update_mac_db() | ||
| assert result is expected_result | ||
| assert mock_db.set_TI_file_info.call_count == db_call_count | ||
|
|
There was a problem hiding this comment.
we should mock this one
with open(path_to_mac_db, "w")because when update_mac_db() is called, it writes to "databases/macaddress-db.json"
tests/test_update_file_manager.py
Outdated
| test_data = """# ja3_md5,first_seen,last_seen,Listingreason | ||
| 8f52d1ce303fb4a6515836aec3cc16b1,2017-07-15 19:05:11,2019-07-27 20:00:57,TrickBot""" | ||
| test_file = tmp_path / "test_ssl_feed.csv" | ||
| test_file.write_text(test_data, encoding="utf-8") |
tests/test_update_file_manager.py
Outdated
| 2023-03-08 07:58:29,6cec09bcb575352785d313c7e978f26bfbd528ab,AsyncRAT C&C | ||
| 2023-03-09 08:00:00,aaabbbcccdddeeeeffff00001111222233334444,Cobalt Strike C2""" | ||
| test_file = tmp_path / "test_ssl_feed.csv" | ||
| test_file.write_text(test_data, encoding="utf-8") |
There was a problem hiding this comment.
Hey @Sekhar-Kumar-Dash thanks for all the hard work! i left you some comments, you can fix them then i'll merge
hey @AlyaGomaa thank you for reviewing this PR sorry for this many mistakes i was actually facing a lot of issues with this test i will fix this |
Fixes Issue #605
Changes proposed
Added more tests for updatemanager
Check List (Check all the applicable boxes)