From b4270934a0969a8f4d2b9cb4f7574ac8dd2db82a Mon Sep 17 00:00:00 2001 From: Chelsea Troy Date: Thu, 9 May 2024 12:56:55 -0500 Subject: [PATCH 1/4] Add an example of unit testing the API Client --- src/mozmlops/cloud_storage_api_client.py | 9 +++-- tests/__init__.py | 6 +++ tests/integration/__init__.py | 6 --- tests/unit/test_cloud_storage_api_client.py | 42 +++++++++++++++++++++ tests/unit/test_example.py | 6 --- 5 files changed, 54 insertions(+), 15 deletions(-) create mode 100644 tests/__init__.py create mode 100644 tests/unit/test_cloud_storage_api_client.py delete mode 100644 tests/unit/test_example.py diff --git a/src/mozmlops/cloud_storage_api_client.py b/src/mozmlops/cloud_storage_api_client.py index 6d7394b..5bee48c 100644 --- a/src/mozmlops/cloud_storage_api_client.py +++ b/src/mozmlops/cloud_storage_api_client.py @@ -24,7 +24,7 @@ def __init__(self, project_name: str, bucket_name: str): self.gcs_project_name = project_name self.gcs_bucket_name = bucket_name - def store(self, data: bytes, storage_path: str) -> str: + def store(self, data: bytes, storage_path: str, testing=False) -> str: """ Arguments: data (bytes): The data to be stored in the cloud. @@ -51,7 +51,7 @@ def store(self, data: bytes, storage_path: str) -> str: # object is expected to be new. We don't expect collisions, # so setting this to 0 seems good. try: - blob.upload_from_file(f, if_generation_match=0) + upload_value = blob.upload_from_file(f, if_generation_match=0) log_line = f"The model is stored at {storage_path}" logging.info(log_line) except GoogleCloudError as e: @@ -59,7 +59,10 @@ def store(self, data: bytes, storage_path: str) -> str: raise Exception("The object you tried to upload is already in the GCS bucket. Currently, the .store() function's implementation dictates this behavior.").with_traceback(e.__traceback__) raise e - return storage_path + if testing: + return bucket, blob, upload_value, storage_path + else: + return storage_path def fetch(self, remote_path: str, local_path: str) -> str: """ diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..5c9188c --- /dev/null +++ b/tests/__init__.py @@ -0,0 +1,6 @@ +import sys +import os + +# To avoid ModuleNotFound errors in tests while attempting to import test subjects. +# To be investigated here: https://mozilla-hub.atlassian.net/browse/DENG-3667 +sys.path.append(f"{os.getcwd()}/src") diff --git a/tests/integration/__init__.py b/tests/integration/__init__.py index 5c9188c..e69de29 100644 --- a/tests/integration/__init__.py +++ b/tests/integration/__init__.py @@ -1,6 +0,0 @@ -import sys -import os - -# To avoid ModuleNotFound errors in tests while attempting to import test subjects. -# To be investigated here: https://mozilla-hub.atlassian.net/browse/DENG-3667 -sys.path.append(f"{os.getcwd()}/src") diff --git a/tests/unit/test_cloud_storage_api_client.py b/tests/unit/test_cloud_storage_api_client.py new file mode 100644 index 0000000..07dadad --- /dev/null +++ b/tests/unit/test_cloud_storage_api_client.py @@ -0,0 +1,42 @@ +from datetime import datetime +from google.cloud import storage + +from mozmlops.cloud_storage_api_client import CloudStorageAPIClient + +import pytest + +class MockGoogleCloudStorageClient(): + def __init__(self, **kwargs): + self.initializer_args = kwargs + self.call_list = [f"__init__ with {kwargs}"] + + def get_bucket(self, attr): + return MockGoogleCloudBucket() + +class MockGoogleCloudBucket(): + def blob(self, storage_path): + return MockBlob() + +class MockBlob(): + def upload_from_file(self, bytes, if_generation_match): + return 'fake value to test that we call blob.upload_from_file' + +def get_mock_storage_client(**kwargs): + return MockGoogleCloudStorageClient(**kwargs) + +def test_store__calls_gcloud(monkeypatch): + monkeypatch.setattr(storage, 'Client', get_mock_storage_client) + + storage_client = CloudStorageAPIClient(project_name="mozdata", bucket_name="mozdata-tmp") + + string_to_store = "Ada Lovelace" + filename_to_store_it_at = f"first_computer_programmer.txt" + encoded_string = string_to_store.encode(encoding='utf-8') + + # When we use .store() to call for her name to be stored on GCS, the command calls google cloud the way we expect: + + bucket, blob, upload_value, call_list = storage_client.store(data=encoded_string, storage_path=filename_to_store_it_at, testing=True) + assert isinstance(bucket, MockGoogleCloudBucket) + assert isinstance(blob, MockBlob) + assert upload_value == 'fake value to test that we call blob.upload_from_file' + assert call_list == filename_to_store_it_at, "The model was not stored as we expect." diff --git a/tests/unit/test_example.py b/tests/unit/test_example.py deleted file mode 100644 index 22be9ff..0000000 --- a/tests/unit/test_example.py +++ /dev/null @@ -1,6 +0,0 @@ -def test_example(): - """ - Example test for kicking off the unit test suite. - Used to check that pytest --ignore=tests/integration works as expected. - """ - assert 1==1 From eb79545ae5017fd9a213e3e6879e405279ab6ad4 Mon Sep 17 00:00:00 2001 From: Chelsea Troy Date: Thu, 9 May 2024 15:26:59 -0500 Subject: [PATCH 2/4] Switch from handmade mocks to GCS mocking library --- poetry.lock | 22 ++++++- pyproject.toml | 3 +- src/mozmlops/cloud_storage_api_client.py | 12 ++-- tests/unit/test_cloud_storage_api_client.py | 70 +++++++++++++-------- 4 files changed, 71 insertions(+), 36 deletions(-) diff --git a/poetry.lock b/poetry.lock index cf634c6..1f8d70d 100644 --- a/poetry.lock +++ b/poetry.lock @@ -209,6 +209,24 @@ files = [ [package.dependencies] colorama = {version = "*", markers = "platform_system == \"Windows\""} +[[package]] +name = "cloud-storage-mocker" +version = "0.3.4" +description = "Mocker library of Google Cloud Storage with local filesystem mounting." +optional = false +python-versions = "<3.13,>=3.8" +files = [ + {file = "cloud_storage_mocker-0.3.4-py3-none-any.whl", hash = "sha256:87be5ad2fa6f34f27e4aec9654ccd4f82e26f988a4267d076ff72f21ec1e1715"}, + {file = "cloud_storage_mocker-0.3.4.tar.gz", hash = "sha256:a16bac982fbf2810340295c6d556e5e95b79a90671151d8657b868c0583ac64f"}, +] + +[package.dependencies] +google-cloud-storage = ">=2.7.0" + +[package.extras] +dev = ["black (>=22.10)", "flake8 (>=5.0)", "isort (>=5.10)", "mypy (>=0.991)", "pyproject-flake8 (>=5.0)", "pytest (>=7.1)"] +mypy = ["mypy (>=0.991)", "pytest (>=7.1)"] + [[package]] name = "colorama" version = "0.4.6" @@ -1611,5 +1629,5 @@ testing = ["big-O", "jaraco.functools", "jaraco.itertools", "more-itertools", "p [metadata] lock-version = "2.0" -python-versions = "^3.10" -content-hash = "3f1eec08ebc6ac6b59b3b81761274cba71bf359f22981440d3af0ca67b7cbcfe" +python-versions = ">=3.10,<3.13" +content-hash = "ef11eb8fb80249022bab13891c8e931e5695c6f4e1a5afda987d1f7f338d6cdd" diff --git a/pyproject.toml b/pyproject.toml index 55e4947..356952c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,7 +7,7 @@ license = "MPL-2.0" readme = "README.md" [tool.poetry.dependencies] -python = "^3.10" +python = ">=3.10,<3.13" google-api-core = "^2.19.0" google-cloud-storage = "^2.16.0" wandb = "^0.16.6" @@ -18,6 +18,7 @@ ruff = "^0.4.3" [tool.poetry.group.dev.dependencies] pytest = "^8.2.0" +cloud-storage-mocker = "^0.3.4" [tool.pytest.ini_options] markers = [ diff --git a/src/mozmlops/cloud_storage_api_client.py b/src/mozmlops/cloud_storage_api_client.py index 5bee48c..8ff078f 100644 --- a/src/mozmlops/cloud_storage_api_client.py +++ b/src/mozmlops/cloud_storage_api_client.py @@ -24,7 +24,7 @@ def __init__(self, project_name: str, bucket_name: str): self.gcs_project_name = project_name self.gcs_bucket_name = bucket_name - def store(self, data: bytes, storage_path: str, testing=False) -> str: + def store(self, data: bytes, storage_path: str) -> str: """ Arguments: data (bytes): The data to be stored in the cloud. @@ -40,7 +40,7 @@ def store(self, data: bytes, storage_path: str, testing=False) -> str: client = storage.Client(project=self.gcs_project_name) # Raises an exception if the bucket name cannot be found - bucket = client.get_bucket(self.gcs_bucket_name) + bucket = client.bucket(self.gcs_bucket_name) blob = bucket.blob(storage_path) @@ -59,10 +59,7 @@ def store(self, data: bytes, storage_path: str, testing=False) -> str: raise Exception("The object you tried to upload is already in the GCS bucket. Currently, the .store() function's implementation dictates this behavior.").with_traceback(e.__traceback__) raise e - if testing: - return bucket, blob, upload_value, storage_path - else: - return storage_path + return storage_path def fetch(self, remote_path: str, local_path: str) -> str: """ @@ -77,7 +74,7 @@ def fetch(self, remote_path: str, local_path: str) -> str: from google.cloud import storage client = storage.Client(project=self.gcs_project_name) - bucket = client.get_bucket(self.gcs_bucket_name) + bucket = client.bucket(self.gcs_bucket_name) blob = bucket.blob(remote_path) @@ -86,6 +83,7 @@ def fetch(self, remote_path: str, local_path: str) -> str: p.parent.mkdir(parents=True, exist_ok=True) blob.download_to_filename(local_path) + return local_path def __delete(self, remote_path: str) -> str: """ diff --git a/tests/unit/test_cloud_storage_api_client.py b/tests/unit/test_cloud_storage_api_client.py index 07dadad..2711146 100644 --- a/tests/unit/test_cloud_storage_api_client.py +++ b/tests/unit/test_cloud_storage_api_client.py @@ -1,3 +1,4 @@ +import io from datetime import datetime from google.cloud import storage @@ -5,38 +6,55 @@ import pytest -class MockGoogleCloudStorageClient(): - def __init__(self, **kwargs): - self.initializer_args = kwargs - self.call_list = [f"__init__ with {kwargs}"] +import google.cloud.storage # type: ignore[import] - def get_bucket(self, attr): - return MockGoogleCloudBucket() +from cloud_storage_mocker import BlobMetadata, Mount +from cloud_storage_mocker import patch as gcs_patch -class MockGoogleCloudBucket(): - def blob(self, storage_path): - return MockBlob() +import pathlib +def test_store__stores_file_on_gcs(tmp_path: pathlib.Path) -> None: + # Mounts directories. Empty list is allowed if no actual access is required. + with gcs_patch( + [ + Mount("testbucket", tmp_path / "src", readable=True, writable=True), + ], + ): + storage_client = CloudStorageAPIClient(project_name="testproject", bucket_name="testbucket") -class MockBlob(): - def upload_from_file(self, bytes, if_generation_match): - return 'fake value to test that we call blob.upload_from_file' + string_to_store = "Ada Lovelace" + timestamp = datetime.now().strftime("%Y-%m-%d-%H-%M-%S") + filename_to_store_it_at = f"first_computer_programmer_{timestamp}.txt" + encoded_string = string_to_store.encode(encoding='utf-8') -def get_mock_storage_client(**kwargs): - return MockGoogleCloudStorageClient(**kwargs) + filepath = storage_client.store(data=encoded_string, storage_path=filename_to_store_it_at) + assert filepath == filename_to_store_it_at -def test_store__calls_gcloud(monkeypatch): - monkeypatch.setattr(storage, 'Client', get_mock_storage_client) + mock_client = google.cloud.storage.Client() + blob = mock_client.bucket("testbucket").blob(filename_to_store_it_at) + assert blob.download_as_text() == string_to_store - storage_client = CloudStorageAPIClient(project_name="mozdata", bucket_name="mozdata-tmp") +def test_fetch__gets_file_off_gcs(tmp_path: pathlib.Path) -> None: + # Mounts directories. Empty list is allowed if no actual access is required. + with gcs_patch( + [ + Mount("testbucket", tmp_path / "src", readable=True, writable=True), + ], + ): + storage_client = CloudStorageAPIClient(project_name="testproject", bucket_name="testbucket") + + string_to_store = "Ada Lovelace" + timestamp = datetime.now().strftime("%Y-%m-%d-%H-%M-%S") + filename_to_store_it_at = f"first_computer_programmer_{timestamp}.txt" + encoded_string = string_to_store.encode(encoding='utf-8') + + + mock_client = google.cloud.storage.Client() + blob = mock_client.bucket("testbucket").blob(filename_to_store_it_at) + with io.BytesIO(encoded_string) as data: + blob.upload_from_file(data) + + storage_client.fetch(remote_path=filename_to_store_it_at, local_path=tmp_path / filename_to_store_it_at) + assert (tmp_path / filename_to_store_it_at).read_text() == string_to_store - string_to_store = "Ada Lovelace" - filename_to_store_it_at = f"first_computer_programmer.txt" - encoded_string = string_to_store.encode(encoding='utf-8') - # When we use .store() to call for her name to be stored on GCS, the command calls google cloud the way we expect: - bucket, blob, upload_value, call_list = storage_client.store(data=encoded_string, storage_path=filename_to_store_it_at, testing=True) - assert isinstance(bucket, MockGoogleCloudBucket) - assert isinstance(blob, MockBlob) - assert upload_value == 'fake value to test that we call blob.upload_from_file' - assert call_list == filename_to_store_it_at, "The model was not stored as we expect." From 3228ba9856854d8b6515a6a0daabea6d53297948 Mon Sep 17 00:00:00 2001 From: Chelsea Troy Date: Thu, 9 May 2024 16:09:52 -0500 Subject: [PATCH 3/4] Add comments and docstrings to unit tests --- tests/unit/test_cloud_storage_api_client.py | 54 ++++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/tests/unit/test_cloud_storage_api_client.py b/tests/unit/test_cloud_storage_api_client.py index 2711146..dc94042 100644 --- a/tests/unit/test_cloud_storage_api_client.py +++ b/tests/unit/test_cloud_storage_api_client.py @@ -1,24 +1,37 @@ import io +import pathlib from datetime import datetime -from google.cloud import storage +import google.cloud.storage # type: ignore[import] +from cloud_storage_mocker import Mount +from cloud_storage_mocker import patch as gcs_patch from mozmlops.cloud_storage_api_client import CloudStorageAPIClient -import pytest -import google.cloud.storage # type: ignore[import] +def test_store__stores_file_on_gcs(tmp_path: pathlib.Path) -> None: + """ + This is a unit test that checks whether we called the GCS API + according to the expectations of this GCS mocking library: -from cloud_storage_mocker import BlobMetadata, Mount -from cloud_storage_mocker import patch as gcs_patch + https://github.com/odashi/cloud-storage-mocker -import pathlib -def test_store__stores_file_on_gcs(tmp_path: pathlib.Path) -> None: - # Mounts directories. Empty list is allowed if no actual access is required. + The library does not support mocks for all GCS operations, + and it is not maintained by the team that builds the GCS API. + + If this test fails and it's unclear why, + run the integration tests to check our integration behavior for real + to see if something is wrong: + + pytest -m integration + """ + + # Given the following mocked bucket with gcs_patch( [ Mount("testbucket", tmp_path / "src", readable=True, writable=True), ], ): + # When our API Client stores data in a file on GCS: storage_client = CloudStorageAPIClient(project_name="testproject", bucket_name="testbucket") string_to_store = "Ada Lovelace" @@ -29,30 +42,47 @@ def test_store__stores_file_on_gcs(tmp_path: pathlib.Path) -> None: filepath = storage_client.store(data=encoded_string, storage_path=filename_to_store_it_at) assert filepath == filename_to_store_it_at + # Then we can do the steps to download the file from mock GCS: mock_client = google.cloud.storage.Client() blob = mock_client.bucket("testbucket").blob(filename_to_store_it_at) assert blob.download_as_text() == string_to_store def test_fetch__gets_file_off_gcs(tmp_path: pathlib.Path) -> None: - # Mounts directories. Empty list is allowed if no actual access is required. + """ + This is a unit test that checks whether we called the GCS API + according to the expectations of this GCS mocking library: + + https://github.com/odashi/cloud-storage-mocker + + The library does not support mocks for all GCS operations, + and it is not maintained by the team that builds the GCS API. + + If this test fails and it's unclear why, + run the integration tests to check our integration behavior for real + to see if something is wrong: + + pytest -m integration + """ + + # Given the following mocked bucket with gcs_patch( [ Mount("testbucket", tmp_path / "src", readable=True, writable=True), ], ): - storage_client = CloudStorageAPIClient(project_name="testproject", bucket_name="testbucket") - + # When we do the steps to upload a file to mock GCS: string_to_store = "Ada Lovelace" timestamp = datetime.now().strftime("%Y-%m-%d-%H-%M-%S") filename_to_store_it_at = f"first_computer_programmer_{timestamp}.txt" encoded_string = string_to_store.encode(encoding='utf-8') - mock_client = google.cloud.storage.Client() blob = mock_client.bucket("testbucket").blob(filename_to_store_it_at) with io.BytesIO(encoded_string) as data: blob.upload_from_file(data) + # Then Our API client is able to fetch them: + storage_client = CloudStorageAPIClient(project_name="testproject", bucket_name="testbucket") storage_client.fetch(remote_path=filename_to_store_it_at, local_path=tmp_path / filename_to_store_it_at) assert (tmp_path / filename_to_store_it_at).read_text() == string_to_store From f0fe2a2d4e36f09c15e5328bab1d66f1f9bcefae Mon Sep 17 00:00:00 2001 From: Chelsea Troy Date: Thu, 9 May 2024 16:17:13 -0500 Subject: [PATCH 4/4] remove unused variable --- src/mozmlops/cloud_storage_api_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mozmlops/cloud_storage_api_client.py b/src/mozmlops/cloud_storage_api_client.py index 8ff078f..9729fac 100644 --- a/src/mozmlops/cloud_storage_api_client.py +++ b/src/mozmlops/cloud_storage_api_client.py @@ -51,7 +51,7 @@ def store(self, data: bytes, storage_path: str) -> str: # object is expected to be new. We don't expect collisions, # so setting this to 0 seems good. try: - upload_value = blob.upload_from_file(f, if_generation_match=0) + blob.upload_from_file(f, if_generation_match=0) log_line = f"The model is stored at {storage_path}" logging.info(log_line) except GoogleCloudError as e: