From 7379d1ce28ca6f9e3c123805d711af9b5a8b5704 Mon Sep 17 00:00:00 2001 From: Michal Franczel Date: Thu, 26 Mar 2026 09:40:30 +0100 Subject: [PATCH 1/3] fix(cache-downloader): Skip download if already cached Enhances the download_dependency function to check for an existing "done" file before initiating a download, preventing unnecessary downloads of already cached dependencies. Adds unit tests to verify this behavior, ensuring that downloads are skipped when the cache is present and triggered when it is absent. --- .../cache-downloader/cache-downloader.py | 8 +++- tests/unit/test_cache_downloader.py | 44 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/dockerfiles/cache-downloader/cache-downloader.py b/dockerfiles/cache-downloader/cache-downloader.py index 49f4367..207a36c 100644 --- a/dockerfiles/cache-downloader/cache-downloader.py +++ b/dockerfiles/cache-downloader/cache-downloader.py @@ -19,6 +19,12 @@ def download_dependency( version_path = os.path.join(BASE_PATH, release_name, f"python{python_version}") done_file = os.path.join(version_path, f"{python_version}-done") + if os.path.isfile(done_file): + print( + f"{datetime.datetime.now()}: {release_name} python{python_version} already cached, skipping download" + ) + return + # Create the version directory if it doesn't exist os.makedirs(version_path, exist_ok=True) @@ -57,7 +63,7 @@ def download_dependency( f"{datetime.datetime.now()}: Done downloading {release_name} {s3_path} and extracting to {version_path}" ) # Create the "done" file - open(done_file, "a").close() + Path(done_file).touch() def submit_downloading( diff --git a/tests/unit/test_cache_downloader.py b/tests/unit/test_cache_downloader.py index 485727d..f7f3b01 100644 --- a/tests/unit/test_cache_downloader.py +++ b/tests/unit/test_cache_downloader.py @@ -1,7 +1,7 @@ import importlib.util import os import shutil -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -21,6 +21,7 @@ _spec.loader.exec_module(_mod) cleanup_old_versions = _mod.cleanup_old_versions +download_dependency = _mod.download_dependency class TestCleanupOldVersions: @@ -169,3 +170,44 @@ def _fail_on_v1(path, *args, **kwargs): assert (tmp_path / "v1").exists() assert (tmp_path / "v2").exists() assert (tmp_path / "v3").exists() + + +class TestDownloadDependency: + def test_skips_download_when_done_file_exists(self, tmp_path): + """Already-cached version should not trigger a download.""" + release = "v1.0.0" + py_ver = "3.11" + version_path = tmp_path / release / f"python{py_ver}" + version_path.mkdir(parents=True) + (version_path / f"{py_ver}-done").write_text("ok") + + with patch.object(_mod, "BASE_PATH", str(tmp_path)): + with patch.object(_mod.subprocess, "Popen") as mock_popen: + download_dependency(release, py_ver, "fake-bucket") + + mock_popen.assert_not_called() + + def test_downloads_when_done_file_missing(self, tmp_path): + """Missing done file should trigger the download.""" + release = "v1.0.0" + py_ver = "3.11" + version_path = tmp_path / release / f"python{py_ver}" + version_path.mkdir(parents=True) + + mock_aws = MagicMock() + mock_aws.stdout = MagicMock() + mock_aws.stderr = MagicMock(read=MagicMock(return_value=b"")) + mock_aws.wait.return_value = 0 + + mock_tar = MagicMock() + mock_tar.communicate.return_value = (b"", b"") + mock_tar.returncode = 0 + + with patch.object(_mod, "BASE_PATH", str(tmp_path)): + with patch.object( + _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar] + ) as mock_popen: + download_dependency(release, py_ver, "fake-bucket") + + assert mock_popen.call_count == 2 + assert (version_path / f"{py_ver}-done").exists() From 7e637bb0e7e3e8e78368e72e3fc2a183d0230ff3 Mon Sep 17 00:00:00 2001 From: Michal Franczel Date: Thu, 26 Mar 2026 09:56:07 +0100 Subject: [PATCH 2/3] refactor(cache-downloader): Use pathlib for file existence check Replaces os.path.isfile with pathlib.Path.is_file in the download_dependency function to improve code readability and adhere to project standards. Updates unit tests to maintain functionality while ensuring consistent use of context managers for patching. --- dockerfiles/cache-downloader/cache-downloader.py | 2 +- tests/unit/test_cache_downloader.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dockerfiles/cache-downloader/cache-downloader.py b/dockerfiles/cache-downloader/cache-downloader.py index 207a36c..0e6fd6d 100644 --- a/dockerfiles/cache-downloader/cache-downloader.py +++ b/dockerfiles/cache-downloader/cache-downloader.py @@ -19,7 +19,7 @@ def download_dependency( version_path = os.path.join(BASE_PATH, release_name, f"python{python_version}") done_file = os.path.join(version_path, f"{python_version}-done") - if os.path.isfile(done_file): + if Path(done_file).is_file(): print( f"{datetime.datetime.now()}: {release_name} python{python_version} already cached, skipping download" ) diff --git a/tests/unit/test_cache_downloader.py b/tests/unit/test_cache_downloader.py index f7f3b01..fc791f8 100644 --- a/tests/unit/test_cache_downloader.py +++ b/tests/unit/test_cache_downloader.py @@ -181,9 +181,10 @@ def test_skips_download_when_done_file_exists(self, tmp_path): version_path.mkdir(parents=True) (version_path / f"{py_ver}-done").write_text("ok") - with patch.object(_mod, "BASE_PATH", str(tmp_path)): - with patch.object(_mod.subprocess, "Popen") as mock_popen: - download_dependency(release, py_ver, "fake-bucket") + with patch.object(_mod, "BASE_PATH", str(tmp_path)), patch.object( + _mod.subprocess, "Popen" + ) as mock_popen: + download_dependency(release, py_ver, "fake-bucket") mock_popen.assert_not_called() @@ -203,11 +204,10 @@ def test_downloads_when_done_file_missing(self, tmp_path): mock_tar.communicate.return_value = (b"", b"") mock_tar.returncode = 0 - with patch.object(_mod, "BASE_PATH", str(tmp_path)): - with patch.object( - _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar] - ) as mock_popen: - download_dependency(release, py_ver, "fake-bucket") + with patch.object(_mod, "BASE_PATH", str(tmp_path)), patch.object( + _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar] + ) as mock_popen: + download_dependency(release, py_ver, "fake-bucket") assert mock_popen.call_count == 2 assert (version_path / f"{py_ver}-done").exists() From d96544439344bedc87657881ac1abe138ccb38a8 Mon Sep 17 00:00:00 2001 From: Michal Franczel Date: Thu, 26 Mar 2026 10:03:28 +0100 Subject: [PATCH 3/3] fix: Reformat --- tests/unit/test_cache_downloader.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_cache_downloader.py b/tests/unit/test_cache_downloader.py index fc791f8..2e09a58 100644 --- a/tests/unit/test_cache_downloader.py +++ b/tests/unit/test_cache_downloader.py @@ -181,9 +181,10 @@ def test_skips_download_when_done_file_exists(self, tmp_path): version_path.mkdir(parents=True) (version_path / f"{py_ver}-done").write_text("ok") - with patch.object(_mod, "BASE_PATH", str(tmp_path)), patch.object( - _mod.subprocess, "Popen" - ) as mock_popen: + with ( + patch.object(_mod, "BASE_PATH", str(tmp_path)), + patch.object(_mod.subprocess, "Popen") as mock_popen, + ): download_dependency(release, py_ver, "fake-bucket") mock_popen.assert_not_called() @@ -204,9 +205,12 @@ def test_downloads_when_done_file_missing(self, tmp_path): mock_tar.communicate.return_value = (b"", b"") mock_tar.returncode = 0 - with patch.object(_mod, "BASE_PATH", str(tmp_path)), patch.object( - _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar] - ) as mock_popen: + with ( + patch.object(_mod, "BASE_PATH", str(tmp_path)), + patch.object( + _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar] + ) as mock_popen, + ): download_dependency(release, py_ver, "fake-bucket") assert mock_popen.call_count == 2