minor cleanup

Author: alexluck-sift

python/lib/sift_client/_tests/resources/test_test_results_log_file.py

@@ -98,8 +98,6 @@ async def test_create_stamps_log_file(self, api):
         }
         result = await api.create(report_data, log_file=LOG)
         assert result._log_file == LOG
-        # Confirm the resource forwards log_file to the low-level wrapper, i.e.
-        # the value reaches the file-write branch (not just the returned entity).
         assert api._low_level_client.create_test_report.call_args.kwargs["log_file"] == LOG
 
     @pytest.mark.asyncio
@@ -337,11 +335,8 @@ async def test_metadata_update_round_trips_through_log_file(self, real_api, tmp_
             "trial_number": 42.5,
             "is_dry_run": True,
         }
-        await real_api.update(
-            test_report=report,
-            update={"metadata": metadata},
-            log_file=report._log_file,  # mirrors `log_file or self._log_file`
-        )
+        # No log_file kwarg — the resource layer must read it off the entity.
+        await real_api.update(test_report=report, update={"metadata": metadata})
 
         # Find the UpdateTestReport line and decode it the same way replay does.
         update_entries = [

python/lib/sift_client/_tests/sift_types/test_results.py

@@ -176,60 +176,12 @@ def test_update_test_report(self, mock_test_report, mock_client):
             )
             mock_update.assert_called_once_with(updated_report)
 
-    @pytest.mark.parametrize(
-        "entity_fixture,resource_method,update_payload",
-        [
-            ("mock_test_report", "update", {"status": TestStatus.FAILED}),
-            ("mock_test_step", "update_step", {"description": "x"}),
-            ("mock_test_measurement", "update_measurement", {"passed": False}),
-        ],
-    )
-    @pytest.mark.parametrize(
-        "cached,kwarg,expected",
-        [
-            (None, None, None),  # online: no cache, no kwarg -> None
-            ("/tmp/cached.jsonl", None, "/tmp/cached.jsonl"),  # cache fallback (the fix)
-            (None, "/tmp/kwarg.jsonl", "/tmp/kwarg.jsonl"),  # explicit only
-            ("/tmp/cached.jsonl", "/tmp/kwarg.jsonl", "/tmp/kwarg.jsonl"),  # kwarg wins
-        ],
-    )
-    def test_update_resolves_log_file(
-        self,
-        request,
-        mock_client,
-        entity_fixture,
-        resource_method,
-        update_payload,
-        cached,
-        kwarg,
-        expected,
-    ):
-        """Entity .update() forwards `log_file or self._log_file` to the resource.
-
-        Covers all four (cached, kwarg) combinations across all three entity types.
-        """
-        entity = request.getfixturevalue(entity_fixture)
-        if cached is not None:
-            entity.__dict__["_log_file"] = cached
-        mock_method = getattr(mock_client.test_results, resource_method)
-        mock_method.return_value = MagicMock()
-        entity._update = MagicMock()
-
-        if kwarg is not None:
-            entity.update(update_payload, log_file=kwarg)
-        else:
-            entity.update(update_payload)
-
-        assert mock_method.call_args.kwargs["log_file"] == expected
-
     def test_update_preserves_cached_log_file(self, mock_test_report, mock_client):
         """After .update() returns, the cached _log_file survives — BaseType._update()
         only copies model_fields, and private attrs are excluded.
         """
         mock_test_report.__dict__["_log_file"] = "/tmp/cached.jsonl"
 
-        # The returned entity from the resource layer carries its own _log_file
-        # via _stamp_log_file. Real _update() should not clobber self._log_file.
         updated = TestReport(
             id_=mock_test_report.id_,
             status=TestStatus.FAILED,

python/lib/sift_client/resources/test_results.py

@@ -47,10 +47,14 @@ def __init__(self, sift_client: SiftClient):
         self._low_level_client = TestResultsLowLevelClient(grpc_client=self.client.grpc_client)
         self._upload_client = UploadLowLevelClient(rest_client=self.client.rest_client)
 
-    @staticmethod
-    def _associate_log_file(instance, log_file: str | Path | None):
-        # Mirror BaseType._apply_client_to_instance: bypass frozen by writing to
-        # __dict__ directly.
+    def _finalize(self, instance, log_file: str | Path | None):
+        """Attach the client and stamp the log file on a returned entity.
+
+        Bypasses the frozen-model guard the same way `_apply_client_to_instance`
+        does. Read paths skip this and call `_apply_client_to_instance` directly
+        so fetched entities don't carry a log file they didn't originate from.
+        """
+        self._apply_client_to_instance(instance)
         instance.__dict__["_log_file"] = log_file
         return instance
 
@@ -93,7 +97,7 @@ async def create(
             test_report=test_report,
             log_file=log_file,
         )
-        return self._associate_log_file(self._apply_client_to_instance(created_report), log_file)
+        return self._finalize(created_report, log_file)
 
     async def get(self, *, test_report_id: str) -> TestReport:
         """Get a TestReport.
@@ -269,9 +273,7 @@ async def update(
         updated_test_report = await self._low_level_client.update_test_report(
             update, log_file=log_file, existing=existing
         )
-        return self._associate_log_file(
-            self._apply_client_to_instance(updated_test_report), log_file
-        )
+        return self._finalize(updated_test_report, log_file)
 
     async def archive(self, *, test_report: str | TestReport) -> TestReport:
         """Archive a test report.
@@ -319,7 +321,7 @@ async def create_step(
         test_step_result = await self._low_level_client.create_test_step(
             test_step, log_file=log_file
         )
-        return self._associate_log_file(self._apply_client_to_instance(test_step_result), log_file)
+        return self._finalize(test_step_result, log_file)
 
     async def list_steps(
         self,
@@ -450,7 +452,7 @@ async def update_step(
         updated_test_step = await self._low_level_client.update_test_step(
             update, log_file=log_file, existing=existing
         )
-        return self._associate_log_file(self._apply_client_to_instance(updated_test_step), log_file)
+        return self._finalize(updated_test_step, log_file)
 
     async def delete_step(self, *, test_step: str | TestStep) -> None:
         """Delete a test step.
@@ -484,9 +486,7 @@ async def create_measurement(
         test_measurement_result = await self._low_level_client.create_test_measurement(
             test_measurement, log_file=log_file
         )
-        measurement = self._associate_log_file(
-            self._apply_client_to_instance(test_measurement_result), log_file
-        )
+        measurement = self._finalize(test_measurement_result, log_file)
         if update_step and log_file is None:
             step = await self.get_step(test_step=test_measurement_result.test_step_id)
             if step.status == TestStatus.PASSED and not measurement.passed:
@@ -623,9 +623,7 @@ async def update_measurement(
         updated_test_measurement = await self._low_level_client.update_test_measurement(
             update, log_file=log_file, existing=test_measurement
         )
-        updated_test_measurement = self._associate_log_file(
-            self._apply_client_to_instance(updated_test_measurement), log_file
-        )
+        updated_test_measurement = self._finalize(updated_test_measurement, log_file)
         if update_step and log_file is None and update.passed is not None and not update.passed:
             step = await self.get_step(test_step=updated_test_measurement.test_step_id)
             if step.status == TestStatus.PASSED:

python/lib/sift_client/sift_types/test_report.py

@@ -204,8 +204,11 @@ def update(
         update: TestStepUpdate | dict,
         log_file: str | Path | None = None,
     ) -> TestStep:
-        """Update the TestStep."""
-        log_file = log_file if log_file is not None else self._log_file
+        """Update the TestStep.
+
+        If `log_file` is omitted, the resource layer falls back to the value
+        cached on `self` from the create call (offline/logging mode).
+        """
         updated_test_step = self.client.test_results.update_step(
             test_step=self, update=update, log_file=log_file
         )
@@ -431,7 +434,6 @@ def update(
         Returns:
             The updated TestMeasurement.
         """
-        log_file = log_file if log_file is not None else self._log_file
         updated_test_measurement = self.client.test_results.update_measurement(
             test_measurement=self, update=update, update_step=update_step, log_file=log_file
         )
@@ -613,8 +615,11 @@ def update(
         update: TestReportUpdate | dict,
         log_file: str | Path | None = None,
     ) -> TestReport:
-        """Update the TestReport."""
-        log_file = log_file if log_file is not None else self._log_file
+        """Update the TestReport.
+
+        If `log_file` is omitted, the resource layer falls back to the value
+        cached on `self` from the create call (offline/logging mode).
+        """
         updated_test_report = self.client.test_results.update(
             test_report=self, update=update, log_file=log_file
         )