From 85183120bbc7f930573b21080e9bdfdaeb29cf9e Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Thu, 17 Oct 2024 17:32:35 -0600 Subject: [PATCH 1/7] Extend the test coverage --- tests/test_fixity.py | 56 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index 4bd3e41..011625b 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -14,6 +14,7 @@ from fixity import reporting from fixity.models import Report from fixity.models import Session +from fixity.storage_service import StorageServiceError SESSION = Session() STORAGE_SERVICE_URL = "http://localhost:8000/" @@ -634,3 +635,58 @@ def test_scanall_if_sort_argument_is_passed( params={"username": STORAGE_SERVICE_USER, "api_key": STORAGE_SERVICE_KEY}, ), ] + + +@mock.patch("requests.get") +def test_main_handles_exception_if_environment_key_is_missing( + _get: mock.Mock, mock_check_fixity: List[mock.Mock] +) -> None: + _get.side_effect = mock_check_fixity + aip_id = uuid.uuid4() + stream = io.StringIO() + + fixity.main(["scan", str(aip_id)], stream=stream) + + assert fixity.ArgumentError("Missing environment variable: STORAGE_SERVICE_URL") + + +@mock.patch("requests.get") +def test_scanall_handles_exception_if_storage_serrvice_is_not_connected( + _get: mock.Mock, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("STORAGE_SERVICE_URL", STORAGE_SERVICE_URL) + monkeypatch.setenv("STORAGE_SERVICE_USER", "") + monkeypatch.setenv("STORAGE_SERVICE_KEY", "") + aip_id1 = str(uuid.uuid4()) + aip_id2 = str(uuid.uuid4()) + _get.side_effect = [ + mock.Mock( + **{ + "status_code": 401, + "json.return_value": { + "meta": {"next": None}, + "objects": [ + { + "package_type": "AIP", + "status": "UPLOADED", + "uuid": f"{aip_id1}", + }, + { + "package_type": "AIP", + "status": "UPLOADED", + "uuid": f"{aip_id2}", + }, + ], + }, + }, + spec=requests.Response, + side_effect=ConnectionError, + ) + ] + stream = io.StringIO() + + fixity.main(["scanall"], stream=stream) + + assert StorageServiceError( + f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' + ) From 0d86364a7c2f7e9fff023e2c9c3422bee4ba3f7c Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Fri, 18 Oct 2024 09:49:13 -0600 Subject: [PATCH 2/7] Fix the linting --- tests/test_fixity.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index 011625b..1d54b24 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -12,9 +12,9 @@ from fixity import fixity from fixity import reporting +from fixity import storage_service from fixity.models import Report from fixity.models import Session -from fixity.storage_service import StorageServiceError SESSION = Session() STORAGE_SERVICE_URL = "http://localhost:8000/" @@ -651,7 +651,7 @@ def test_main_handles_exception_if_environment_key_is_missing( @mock.patch("requests.get") -def test_scanall_handles_exception_if_storage_serrvice_is_not_connected( +def test_scanall_handles_exception_if_storage_service_is_not_connected( _get: mock.Mock, monkeypatch: pytest.MonkeyPatch ) -> None: monkeypatch.setenv("STORAGE_SERVICE_URL", STORAGE_SERVICE_URL) @@ -687,6 +687,6 @@ def test_scanall_handles_exception_if_storage_serrvice_is_not_connected( fixity.main(["scanall"], stream=stream) - assert StorageServiceError( + assert storage_service.StorageServiceError( # type: ignore f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' ) From 006811ca36efd290e66cdb52ad3011e2f026ef51 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Fri, 18 Oct 2024 16:02:26 -0600 Subject: [PATCH 3/7] Add the suggestions --- fixity/storage_service.py | 3 ++- tests/test_fixity.py | 7 ++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/fixity/storage_service.py b/fixity/storage_service.py index b9dadef..835dd6d 100644 --- a/fixity/storage_service.py +++ b/fixity/storage_service.py @@ -1,6 +1,7 @@ import calendar import json from datetime import datetime +from typing import Dict import requests from sqlalchemy.orm.exc import NoResultFound @@ -23,7 +24,7 @@ class StorageServiceError(Exception): itself will not return, but the caller still needs access to it. """ - def __init__(self, message, report=None): + def __init__(self, message: str, report: Dict[str, str] = None) -> None: self.report = report super().__init__(message) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index 1d54b24..b7dae7c 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -652,11 +652,8 @@ def test_main_handles_exception_if_environment_key_is_missing( @mock.patch("requests.get") def test_scanall_handles_exception_if_storage_service_is_not_connected( - _get: mock.Mock, monkeypatch: pytest.MonkeyPatch + _get: mock.Mock, environment: None ) -> None: - monkeypatch.setenv("STORAGE_SERVICE_URL", STORAGE_SERVICE_URL) - monkeypatch.setenv("STORAGE_SERVICE_USER", "") - monkeypatch.setenv("STORAGE_SERVICE_KEY", "") aip_id1 = str(uuid.uuid4()) aip_id2 = str(uuid.uuid4()) _get.side_effect = [ @@ -687,6 +684,6 @@ def test_scanall_handles_exception_if_storage_service_is_not_connected( fixity.main(["scanall"], stream=stream) - assert storage_service.StorageServiceError( # type: ignore + assert storage_service.StorageServiceError( f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' ) From 6283b798428489389931f8d3e9bcea314a537364 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 22 Oct 2024 14:04:40 -0600 Subject: [PATCH 4/7] Fix the assertion statements --- tests/test_fixity.py | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index b7dae7c..1e944e9 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -12,9 +12,10 @@ from fixity import fixity from fixity import reporting -from fixity import storage_service +from fixity.fixity import ArgumentError from fixity.models import Report from fixity.models import Session +from fixity.storage_service import StorageServiceError SESSION = Session() STORAGE_SERVICE_URL = "http://localhost:8000/" @@ -644,46 +645,30 @@ def test_main_handles_exception_if_environment_key_is_missing( _get.side_effect = mock_check_fixity aip_id = uuid.uuid4() stream = io.StringIO() + response = fixity.main(["scan", str(aip_id)], stream=stream) - fixity.main(["scan", str(aip_id)], stream=stream) - - assert fixity.ArgumentError("Missing environment variable: STORAGE_SERVICE_URL") + assert str(response) == "Missing environment variable: STORAGE_SERVICE_URL" + assert isinstance(response, ArgumentError) @mock.patch("requests.get") def test_scanall_handles_exception_if_storage_service_is_not_connected( _get: mock.Mock, environment: None ) -> None: - aip_id1 = str(uuid.uuid4()) - aip_id2 = str(uuid.uuid4()) _get.side_effect = [ mock.Mock( **{ "status_code": 401, - "json.return_value": { - "meta": {"next": None}, - "objects": [ - { - "package_type": "AIP", - "status": "UPLOADED", - "uuid": f"{aip_id1}", - }, - { - "package_type": "AIP", - "status": "UPLOADED", - "uuid": f"{aip_id2}", - }, - ], - }, }, spec=requests.Response, - side_effect=ConnectionError, ) ] stream = io.StringIO() - fixity.main(["scanall"], stream=stream) + response = fixity.main(["scanall"], stream=stream) - assert storage_service.StorageServiceError( - f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' + assert ( + str(response) + == f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' ) + assert isinstance(response, StorageServiceError) From 3bb0b2fbc220df8294403439c40d417f8bb97c02 Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 22 Oct 2024 15:27:38 -0600 Subject: [PATCH 5/7] Fix the changes and add some more tests --- fixity/storage_service.py | 3 +-- tests/test_fixity.py | 48 ++++++++++++++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/fixity/storage_service.py b/fixity/storage_service.py index 835dd6d..b9dadef 100644 --- a/fixity/storage_service.py +++ b/fixity/storage_service.py @@ -1,7 +1,6 @@ import calendar import json from datetime import datetime -from typing import Dict import requests from sqlalchemy.orm.exc import NoResultFound @@ -24,7 +23,7 @@ class StorageServiceError(Exception): itself will not return, but the caller still needs access to it. """ - def __init__(self, message: str, report: Dict[str, str] = None) -> None: + def __init__(self, message, report=None): self.report = report super().__init__(message) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index 1e944e9..83d6bf0 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -643,16 +643,15 @@ def test_main_handles_exception_if_environment_key_is_missing( _get: mock.Mock, mock_check_fixity: List[mock.Mock] ) -> None: _get.side_effect = mock_check_fixity - aip_id = uuid.uuid4() - stream = io.StringIO() - response = fixity.main(["scan", str(aip_id)], stream=stream) + + response = fixity.main(["scan", str(uuid.uuid4())]) assert str(response) == "Missing environment variable: STORAGE_SERVICE_URL" assert isinstance(response, ArgumentError) @mock.patch("requests.get") -def test_scanall_handles_exception_if_storage_service_is_not_connected( +def test_scanall_handles_exception_if_storage_service_raises_exception( _get: mock.Mock, environment: None ) -> None: _get.side_effect = [ @@ -663,12 +662,49 @@ def test_scanall_handles_exception_if_storage_service_is_not_connected( spec=requests.Response, ) ] - stream = io.StringIO() - response = fixity.main(["scanall"], stream=stream) + response = fixity.main(["scanall"]) assert ( str(response) == f'Storage service at "{STORAGE_SERVICE_URL}" failed authentication while requesting AIPs' ) assert isinstance(response, StorageServiceError) + + +@mock.patch("requests.get") +def test_main_verifies_urls_with_trailing_slash( + _get: mock.Mock, + mock_check_fixity: List[mock.Mock], + monkeypatch: pytest.MonkeyPatch, +) -> None: + _get.side_effect = mock_check_fixity + aip_id = uuid.uuid4() + stream = io.StringIO() + monkeypatch.setenv("STORAGE_SERVICE_URL", "http://foo") + monkeypatch.setenv("STORAGE_SERVICE_USER", STORAGE_SERVICE_USER) + monkeypatch.setenv("STORAGE_SERVICE_KEY", STORAGE_SERVICE_KEY) + report_url = "http://bar" + monkeypatch.setenv("REPORT_URL", report_url) + monkeypatch.setenv("REPORT_USERNAME", "test") + monkeypatch.setenv("REPORT_PASSWORD", "test123") + + response = fixity.main(["scan", str(aip_id)], stream=stream) + + assert response == 0 + + _assert_stream_content_matches( + stream, + [ + f"Unable to POST pre-scan report to {report_url}/", + f"Fixity scan succeeded for AIP: {aip_id}", + f"Unable to POST report for AIP {aip_id} to remote service", + ], + ) + + +def test_main_validate_arguments() -> None: + response = fixity.main(["scan"]) + + assert str(response) == "An AIP UUID must be specified when scanning a single AIP" + assert isinstance(response, ArgumentError) From 6f1b1825f075ac049baf0099ca73c78efa0181dc Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Tue, 22 Oct 2024 16:36:50 -0600 Subject: [PATCH 6/7] Fix the changes --- tests/test_fixity.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/test_fixity.py b/tests/test_fixity.py index 83d6bf0..e056f5c 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -678,10 +678,14 @@ def test_main_verifies_urls_with_trailing_slash( mock_check_fixity: List[mock.Mock], monkeypatch: pytest.MonkeyPatch, ) -> None: - _get.side_effect = mock_check_fixity + _get.side_effect = [ + mock.Mock(**{"status_code": 200}, spec=requests.Response), + mock.Mock(**{"status_code": 401}, spec=requests.Response), + ] aip_id = uuid.uuid4() stream = io.StringIO() - monkeypatch.setenv("STORAGE_SERVICE_URL", "http://foo") + ss_url = "http://foo" + monkeypatch.setenv("STORAGE_SERVICE_URL", ss_url) monkeypatch.setenv("STORAGE_SERVICE_USER", STORAGE_SERVICE_USER) monkeypatch.setenv("STORAGE_SERVICE_KEY", STORAGE_SERVICE_KEY) report_url = "http://bar" @@ -691,14 +695,13 @@ def test_main_verifies_urls_with_trailing_slash( response = fixity.main(["scan", str(aip_id)], stream=stream) - assert response == 0 + assert response is None _assert_stream_content_matches( stream, [ f"Unable to POST pre-scan report to {report_url}/", - f"Fixity scan succeeded for AIP: {aip_id}", - f"Unable to POST report for AIP {aip_id} to remote service", + f'Storage service at "{ss_url}/" failed authentication while scanning AIP {aip_id}', ], ) From 96705bb1ab780ad23074830e2058e61cc842b16a Mon Sep 17 00:00:00 2001 From: Dhwani Patel Date: Wed, 23 Oct 2024 15:53:24 -0600 Subject: [PATCH 7/7] Fix type hints issues --- fixity/fixity.py | 4 ++-- tests/test_fixity.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/fixity/fixity.py b/fixity/fixity.py index 8aa6ad2..cb44b99 100644 --- a/fixity/fixity.py +++ b/fixity/fixity.py @@ -312,9 +312,9 @@ def get_handler(stream, timestamps, log_level=None): def main( argv: Optional[List[str]] = None, - logger: Union[logging.Logger] = None, + logger: Optional[logging.Logger] = None, stream: Optional[TextIO] = None, -) -> Union[int, bool, Type[Exception]]: +) -> Optional[Union[int, bool, Type[Exception]]]: if logger is None: logger = get_logger() if stream is None: diff --git a/tests/test_fixity.py b/tests/test_fixity.py index e056f5c..40fe76c 100644 --- a/tests/test_fixity.py +++ b/tests/test_fixity.py @@ -675,7 +675,6 @@ def test_scanall_handles_exception_if_storage_service_raises_exception( @mock.patch("requests.get") def test_main_verifies_urls_with_trailing_slash( _get: mock.Mock, - mock_check_fixity: List[mock.Mock], monkeypatch: pytest.MonkeyPatch, ) -> None: _get.side_effect = [ @@ -706,7 +705,7 @@ def test_main_verifies_urls_with_trailing_slash( ) -def test_main_validate_arguments() -> None: +def test_main_validates_arguments() -> None: response = fixity.main(["scan"]) assert str(response) == "An AIP UUID must be specified when scanning a single AIP"