changes based on review: try-except should catch more geodiff errors and using real MerginProject in test_push_logging.py

Author: DanChov

mergin/client_push.py

@@ -15,6 +15,13 @@
 import tempfile
 import concurrent.futures
 import os
+from pygeodiff import (
+    GeoDiff,
+    GeoDiffLibError,
+    GeoDiffLibConflictError,
+    GeoDiffLibUnsupportedChangeError,
+    GeoDiffLibVersionError,
+)
 
 from .common import UPLOAD_CHUNK_SIZE, ClientError
 from .merginproject import MerginProject
@@ -326,16 +333,10 @@ def _geodiff_changes_count(mp: MerginProject, diff_rel_path: str):
     Never raises – diagnostics/logging must not fail.
     """
 
+    diff_abs = mp.fpath_meta(diff_rel_path)
     try:
-        diff_abs = mp.fpath_meta(diff_rel_path)
-    except FileNotFoundError:
-        return None
-
-    try:
-        from pygeodiff import GeoDiff, GeoDiffLibError
-
         return GeoDiff().changes_count(diff_abs)
-    except GeoDiffLibError:
+    except (GeoDiffLibError, GeoDiffLibConflictError, GeoDiffLibUnsupportedChangeError, GeoDiffLibVersionError):
         return None
 
 

mergin/test/test_push_logging.py

@@ -2,62 +2,62 @@
 from pathlib import Path
 import logging
 import tempfile
+from unittest.mock import MagicMock
 import pytest
 
 from pygeodiff import GeoDiff
 from mergin.client_push import push_project_finalize, UploadJob
 from mergin.common import ClientError
+from mergin.merginproject import MerginProject
+from mergin.client import MerginClient
 
 
 @pytest.mark.parametrize("status_code", [502, 504])
 def test_push_finalize_logs_on_5xx_real_diff(caplog, status_code, tmp_path):
-    # --- data ---
+    # test data
     data_dir = Path(__file__).resolve().parent / "test_data"
     base = data_dir / "base.gpkg"
     modified = data_dir / "inserted_1_A.gpkg"
-    assert base.exists()
-    assert modified.exists()
+    assert base.exists() and modified.exists()
 
-    diff_path = tmp_path / "base_to_inserted_1_A.diff"
-    GeoDiff().create_changeset(str(base), str(modified), str(diff_path))
-    diff_size = diff_path.stat().st_size
-    file_size = modified.stat().st_size
+    # real MerginProject in temp dir
+    proj_dir = tmp_path / "proj"
+    meta_dir = proj_dir / ".mergin"
+    meta_dir.mkdir(parents=True)
+    mp = MerginProject(str(proj_dir))
 
-    # --- logger ---
+    # route MP logs into pytest caplog
     logger = logging.getLogger("mergin_test")
     logger.setLevel(logging.DEBUG)
     logger.propagate = True
     caplog.set_level(logging.ERROR, logger="mergin_test")
+    mp.log = logger
 
-    # --- fake MP/MC len tam, kde treba ---
-    class MP:
-        def __init__(self, log):
-            self.log = log
-
-        def update_metadata(self, *a, **k):
-            pass
-
-        def apply_push_changes(self, *a, **k):
-            pass
-
-        def fpath_meta(self, rel):
-            return str(diff_path) if rel == diff_path.name else rel
+    # generate a real diff into .mergin/
+    diff_path = meta_dir / "base_to_inserted_1_A.diff"
+    GeoDiff().create_changeset(str(base), str(modified), str(diff_path))
+    diff_rel = diff_path.name
+    diff_size = diff_path.stat().st_size
+    file_size = modified.stat().st_size
 
+    # mock MerginClient: only patch post(); simulate 5xx on finish
     tx = "tx-1"
 
-    class MC:
-        def post(self, url, *args, **kwargs):
-            if url.endswith(f"/v1/project/push/finish/{tx}"):
-                err = ClientError("Gateway error")
-                setattr(err, "http_error", status_code)
-                raise err
-            if url.endswith(f"/v1/project/push/cancel/{tx}"):
-                return SimpleNamespace(msg="cancelled")
-            return SimpleNamespace(msg="ok")
+    def mc_post(url, *args, **kwargs):
+        if url.endswith(f"/v1/project/push/finish/{tx}"):
+            err = ClientError("Gateway error")
+            setattr(err, "http_error", status_code)  # emulate HTTP code on the exception
+            raise err
+        if url.endswith(f"/v1/project/push/cancel/{tx}"):
+            return SimpleNamespace(msg="cancelled")
+        return SimpleNamespace(msg="ok")
+
+    mc = MagicMock(spec=MerginClient)
+    mc.post.side_effect = mc_post
 
     tmp_dir = tempfile.TemporaryDirectory(prefix="python-api-client-test-")
 
-    # --- real UploadJob objekt ---
+    # build a real UploadJob that references the diff/file sizes
     job = UploadJob(
         project_path="u/p",
         changes={
@@ -66,25 +66,24 @@ def post(self, url, *args, **kwargs):
                 {
                     "path": modified.name,
                     "size": file_size,
-                    "diff": {"path": diff_path.name, "size": diff_size},
+                    "diff": {"path": diff_rel, "size": diff_size},
                     "chunks": [1],
                 }
             ],
             "removed": [],
         },
         transaction_id=tx,
-        mp=MP(logger),
-        mc=MC(),
+        mp=mp,
+        mc=mc,
         tmp_dir=tmp_dir,
     )
 
-    # nastav to, čo finalize() očakáva
     job.total_size = 1234
     job.transferred_size = 1234
-    job.upload_queue_items = [1]  # len pre log „Upload details“
+    job.upload_queue_items = [1]
     job.executor = SimpleNamespace(shutdown=lambda wait=True: None)
     job.futures = [SimpleNamespace(done=lambda: True, exception=lambda: None, running=lambda: False)]
-    job.server_resp = {"version": "n/a"}  # aj tak sa 5xx vyhodí skôr
+    job.server_resp = {"version": "n/a"}
 
     with pytest.raises(ClientError):
         push_project_finalize(job)