✅ Upgrade remote sample fixtures (#640)

Addresses part of issue #603 and improves fixtures in tests:

- Some tests were using `_fetch_remote_sample` even though tests have a special `remote_sample` wrapper.
- `remote_sample` previously created a new folder for each run (data-0, data-1, data-2, and so on). Now it uses just one folder for all remote fixtures during a test session.
- To prevent duplicated downloads, I added download locks to the fixtures.
- Combined `_fetch_remote_sample` and `download_data` since they had the same logic.

---------

Co-authored-by: Shan E Ahmed Raza <[email protected]>

requirements/requirements.txt

@@ -3,6 +3,7 @@
 albumentations>=1.3.0
 Click>=8.1.3
 defusedxml>=0.7.1
+filelock>=3.9.0
 flask>=2.2.2
 glymur>=0.12.1, ~=0.12.6 # 0.12.6 is incompatible due to a private attribute
 imagecodecs>=2022.9.26

tests/conftest.py

@@ -60,12 +60,18 @@ def root_path(request) -> Path:
 
 
 @pytest.fixture(scope="session")
-def remote_sample(tmp_path_factory: TempPathFactory) -> Callable:
+def tmp_samples_path(tmp_path_factory: TempPathFactory):
+    """Return a temporary path."""
+    return tmp_path_factory.mktemp("data")
+
+
+@pytest.fixture(scope="session")
+def remote_sample(tmp_samples_path) -> Callable:
     """Factory fixture for fetching sample files."""
 
     def __remote_sample(key: str) -> pathlib.Path:
         """Wrapper around tiatoolbox.data._fetch_remote_sample for tests."""
-        return _fetch_remote_sample(key, tmp_path_factory.mktemp("data"))
+        return _fetch_remote_sample(key, tmp_samples_path)
 
     return __remote_sample
 

tests/models/test_dataset.py

@@ -96,7 +96,7 @@ def test_kather_dataset(tmp_path):
         "/kather100k-train-nonorm-subset-90.zip"
     )
     save_zip_path = os.path.join(save_dir_path, "Kather.zip")
-    download_data(url, save_zip_path)
+    download_data(url, save_path=save_zip_path)
     unzip_data(save_zip_path, save_dir_path)
     extracted_dir = os.path.join(save_dir_path, "NCT-CRC-HE-100K-NONORM/")
     dataset = KatherPatchDataset(save_dir_path=extracted_dir)

tests/test_tiffreader.py

@@ -1,13 +1,12 @@
 import pytest
 from defusedxml import ElementTree
 
-from tiatoolbox.data import _fetch_remote_sample
 from tiatoolbox.wsicore import wsireader
 
 
-def test_ome_missing_instrument_ref(monkeypatch):
+def test_ome_missing_instrument_ref(monkeypatch, remote_sample):
     """Test that an OME-TIFF can be read without instrument reference."""
-    sample = _fetch_remote_sample("ome-brightfield-pyramid-1-small")
+    sample = remote_sample("ome-brightfield-pyramid-1-small")
     wsi = wsireader.TIFFWSIReader(sample)
     page = wsi.tiff.pages[0]
     description = page.description
@@ -26,9 +25,9 @@ def test_ome_missing_instrument_ref(monkeypatch):
     assert wsi.info.objective_power is None
 
 
-def test_ome_missing_physicalsize(monkeypatch):
+def test_ome_missing_physicalsize(monkeypatch, remote_sample):
     """Test that an OME-TIFF can be read without physical size."""
-    sample = _fetch_remote_sample("ome-brightfield-pyramid-1-small")
+    sample = remote_sample("ome-brightfield-pyramid-1-small")
     wsi = wsireader.TIFFWSIReader(sample)
     page = wsi.tiff.pages[0]
     description = page.description
@@ -47,9 +46,9 @@ def test_ome_missing_physicalsize(monkeypatch):
     assert wsi.info.mpp is None
 
 
-def test_ome_missing_physicalsizey(monkeypatch, caplog):
+def test_ome_missing_physicalsizey(monkeypatch, caplog, remote_sample):
     """Test that an OME-TIFF can be read without physical size."""
-    sample = _fetch_remote_sample("ome-brightfield-pyramid-1-small")
+    sample = remote_sample("ome-brightfield-pyramid-1-small")
     wsi = wsireader.TIFFWSIReader(sample)
     page = wsi.tiff.pages[0]
     description = page.description
@@ -68,9 +67,9 @@ def test_ome_missing_physicalsizey(monkeypatch, caplog):
     assert "Only one MPP value found. Using it for both X  and Y" in caplog.text
 
 
-def test_tiffreader_non_tiled_metadata(monkeypatch):
+def test_tiffreader_non_tiled_metadata(monkeypatch, remote_sample):
     """Test that fetching metadata for non-tiled TIFF works."""
-    sample = _fetch_remote_sample("ome-brightfield-pyramid-1-small")
+    sample = remote_sample("ome-brightfield-pyramid-1-small")
     wsi = wsireader.TIFFWSIReader(sample)
     monkeypatch.setattr(wsi.tiff, "is_ome", False)
     monkeypatch.setattr(

tests/test_utils.py

@@ -13,6 +13,7 @@
 import pandas as pd
 import pytest
 from PIL import Image
+from requests import HTTPError
 from shapely.geometry import Polygon
 
 from tests.test_annotation_stores import cell_polygon
@@ -934,12 +935,16 @@ def test_download_unzip_data():
     if os.path.exists(save_dir_path):
         shutil.rmtree(save_dir_path, ignore_errors=True)
     os.makedirs(save_dir_path)
-    save_zip_path = os.path.join(save_dir_path, "test_directory.zip")
-    misc.download_data(url, save_zip_path)
-    misc.download_data(url, save_zip_path, overwrite=True)  # do overwrite
-    misc.unzip_data(save_zip_path, save_dir_path, del_zip=False)  # not remove
-    assert os.path.exists(save_zip_path)
-    misc.unzip_data(save_zip_path, save_dir_path)
+
+    save_zip_path1 = misc.download_data(url, save_dir=save_dir_path)
+    save_zip_path2 = misc.download_data(
+        url, save_dir=save_dir_path, overwrite=True
+    )  # do overwrite
+    assert save_zip_path1 == save_zip_path2
+
+    misc.unzip_data(save_zip_path1, save_dir_path, del_zip=False)  # not remove
+    assert os.path.exists(save_zip_path1)
+    misc.unzip_data(save_zip_path1, save_dir_path)
 
     extracted_path = os.path.join(save_dir_path, "test_directory")
     # to avoid hidden files in case of MAC-OS or Windows (?)
@@ -992,11 +997,19 @@ def test_download_data():
     # URL not valid
     # shouldn't use save_path if test runs correctly
     save_path = os.path.join(save_dir_path, "temp")
-    with pytest.raises(ConnectionError):
+    with pytest.raises(HTTPError):
         misc.download_data(
             "https://tiatoolbox.dcs.warwick.ac.uk/invalid-url", save_path
         )
 
+    # Both save_dir and save_path are specified
+    with pytest.raises(ValueError, match="save_path and save_dir"):
+        misc.download_data(url, save_dir=save_dir_path, save_path=save_path)
+
+    # None of save_dir and save_path are specified
+    with pytest.raises(ValueError, match="save_path or save_dir"):
+        misc.download_data(url)
+
 
 def test_parse_cv2_interpolaton():
     """Test parsing interpolation modes for cv2."""