From 19041fa231634259cd06a422c9a0edab055b3ae6 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 10:53:02 -0700 Subject: [PATCH 1/7] pass log file with update calls in test_results --- .../_tests/sift_types/test_results.py | 75 +++++++++++++++++++ .../lib/sift_client/resources/test_results.py | 25 +++++-- .../lib/sift_client/sift_types/test_report.py | 11 ++- 3 files changed, 103 insertions(+), 8 deletions(-) diff --git a/python/lib/sift_client/_tests/sift_types/test_results.py b/python/lib/sift_client/_tests/sift_types/test_results.py index 8caef1978..3e8bb9479 100644 --- a/python/lib/sift_client/_tests/sift_types/test_results.py +++ b/python/lib/sift_client/_tests/sift_types/test_results.py @@ -176,6 +176,81 @@ 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, + name=mock_test_report.name, + test_system_name=mock_test_report.test_system_name, + test_case=mock_test_report.test_case, + start_time=mock_test_report.start_time, + end_time=mock_test_report.end_time, + metadata={"k": "v"}, + is_archived=False, + ) + updated.__dict__["_log_file"] = "/tmp/cached.jsonl" + mock_client.test_results.update.return_value = updated + + result = mock_test_report.update({"status": TestStatus.FAILED}) + + assert result is mock_test_report + assert mock_test_report._log_file == "/tmp/cached.jsonl" + assert mock_test_report.status == TestStatus.FAILED + assert mock_test_report.metadata == {"k": "v"} + def test_archive_test_report(self, mock_test_report, mock_client): """Test archiving a test report.""" # Create archived report mock diff --git a/python/lib/sift_client/resources/test_results.py b/python/lib/sift_client/resources/test_results.py index 3a78b7b69..ad13c2487 100644 --- a/python/lib/sift_client/resources/test_results.py +++ b/python/lib/sift_client/resources/test_results.py @@ -47,6 +47,13 @@ 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. + instance.__dict__["_log_file"] = log_file + return instance + async def import_(self, test_file: str | Path) -> TestReport: """Import a test report from an already-uploaded file. @@ -86,7 +93,7 @@ async def create( test_report=test_report, log_file=log_file, ) - return self._apply_client_to_instance(created_report) + return self._associate_log_file(self._apply_client_to_instance(created_report), log_file) async def get(self, *, test_report_id: str) -> TestReport: """Get a TestReport. @@ -260,7 +267,9 @@ async def update( updated_test_report = await self._low_level_client.update_test_report( update, log_file=log_file, existing=existing ) - return self._apply_client_to_instance(updated_test_report) + return self._associate_log_file( + self._apply_client_to_instance(updated_test_report), log_file + ) async def archive(self, *, test_report: str | TestReport) -> TestReport: """Archive a test report. @@ -308,7 +317,7 @@ async def create_step( test_step_result = await self._low_level_client.create_test_step( test_step, log_file=log_file ) - return self._apply_client_to_instance(test_step_result) + return self._associate_log_file(self._apply_client_to_instance(test_step_result), log_file) async def list_steps( self, @@ -437,7 +446,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._apply_client_to_instance(updated_test_step) + return self._associate_log_file(self._apply_client_to_instance(updated_test_step), log_file) async def delete_step(self, *, test_step: str | TestStep) -> None: """Delete a test step. @@ -471,7 +480,9 @@ async def create_measurement( test_measurement_result = await self._low_level_client.create_test_measurement( test_measurement, log_file=log_file ) - measurement = self._apply_client_to_instance(test_measurement_result) + measurement = self._associate_log_file( + self._apply_client_to_instance(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: @@ -606,7 +617,9 @@ 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._apply_client_to_instance(updated_test_measurement) + updated_test_measurement = self._associate_log_file( + self._apply_client_to_instance(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: diff --git a/python/lib/sift_client/sift_types/test_report.py b/python/lib/sift_client/sift_types/test_report.py index 37225d222..e935b1ba5 100644 --- a/python/lib/sift_client/sift_types/test_report.py +++ b/python/lib/sift_client/sift_types/test_report.py @@ -151,6 +151,8 @@ class TestStep(BaseType[TestStepProto, "TestStep"], FileAttachmentsMixin): start_time: datetime end_time: datetime error_info: ErrorInfo | None = None + # Set by the resource layer when this instance was produced from a logging-mode call + _log_file: str | Path | None = None @classmethod def _from_proto(cls, proto: TestStepProto, sift_client: SiftClient | None = None) -> TestStep: @@ -203,8 +205,7 @@ def update( log_file: str | Path | None = None, ) -> TestStep: """Update the TestStep.""" - if not self.client: - raise ValueError("Client not set") + log_file = log_file if log_file is not None else self._log_file updated_test_step = self.client.test_results.update_step( test_step=self, update=update, log_file=log_file ) @@ -345,6 +346,8 @@ class TestMeasurement(BaseType[TestMeasurementProto, "TestMeasurement"]): string_expected_value: str | None = None passed: bool timestamp: datetime + # Set by the resource layer when this instance was produced from a logging-mode call + _log_file: str | Path | None = None @classmethod def _from_proto( @@ -428,6 +431,7 @@ 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 ) @@ -544,6 +548,8 @@ class TestReport(BaseType[TestReportProto, "TestReport"], FileAttachmentsMixin): run_id: str | None = None archived_date: datetime | None = None is_archived: bool + # Set by the resource layer when this instance was produced from a logging-mode call + _log_file: str | Path | None = None @classmethod def _from_proto( @@ -608,6 +614,7 @@ def update( log_file: str | Path | None = None, ) -> TestReport: """Update the TestReport.""" + log_file = log_file if log_file is not None else self._log_file updated_test_report = self.client.test_results.update( test_report=self, update=update, log_file=log_file ) From 24c6fe4139854aecdb27b150e5d4f64ad60c750c Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 11:12:32 -0700 Subject: [PATCH 2/7] clean up to use new log_file plumbing --- .../resources/test_test_results_log_file.py | 362 ++++++++++++++++++ .../lib/sift_client/resources/test_results.py | 6 + .../util/test_results/context_manager.py | 3 +- .../util/test_results/pytest_util.py | 4 +- 4 files changed, 370 insertions(+), 5 deletions(-) create mode 100644 python/lib/sift_client/_tests/resources/test_test_results_log_file.py diff --git a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py new file mode 100644 index 000000000..cabeedc43 --- /dev/null +++ b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py @@ -0,0 +1,362 @@ +"""Unit tests for log_file plumbing in TestResultsAPIAsync.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +import pytest_asyncio + +from sift_client.resources.test_results import TestResultsAPIAsync +from sift_client.sift_types.test_report import ( + TestMeasurement, + TestMeasurementType, + TestReport, + TestStatus, + TestStep, + TestStepType, +) + +T0 = datetime(2026, 1, 1, tzinfo=timezone.utc) + + +def _make_report(id_: str = "sim-report") -> TestReport: + return TestReport( + id_=id_, + status=TestStatus.IN_PROGRESS, + name="n", + test_system_name="s", + test_case="c", + start_time=T0, + end_time=T0, + metadata={}, + is_archived=False, + ) + + +def _make_step(id_: str = "sim-step") -> TestStep: + return TestStep( + id_=id_, + test_report_id="sim-report", + name="step", + step_type=TestStepType.ACTION, + step_path="1", + status=TestStatus.IN_PROGRESS, + start_time=T0, + end_time=T0, + ) + + +def _make_measurement(id_: str = "sim-meas") -> TestMeasurement: + return TestMeasurement( + id_=id_, + measurement_type=TestMeasurementType.BOOLEAN, + name="m", + test_step_id="sim-step", + boolean_value=True, + passed=True, + timestamp=T0, + ) + + +@pytest.fixture +def mock_client(): + client = MagicMock() + client.grpc_client = MagicMock() + client.rest_client = MagicMock() + return client + + +@pytest_asyncio.fixture +def api(mock_client): + """Build a TestResultsAPIAsync with mocked low-level + upload clients.""" + with patch( + "sift_client.resources.test_results.TestResultsLowLevelClient", + autospec=True, + ), patch( + "sift_client.resources.test_results.UploadLowLevelClient", + autospec=True, + ): + return TestResultsAPIAsync(mock_client) + + +LOG = "/tmp/log.jsonl" + + +class TestCreateStamping: + @pytest.mark.asyncio + async def test_create_stamps_log_file(self, api): + api._low_level_client.create_test_report = AsyncMock(return_value=_make_report()) + report_data = { + "status": TestStatus.IN_PROGRESS, + "name": "n", + "test_system_name": "s", + "test_case": "c", + "start_time": T0, + "end_time": T0, + } + 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 + async def test_create_step_stamps_log_file(self, api): + api._low_level_client.create_test_step = AsyncMock(return_value=_make_step()) + step_data = { + "test_report_id": "sim-report", + "name": "step", + "step_type": TestStepType.ACTION, + "step_path": "1", + "status": TestStatus.IN_PROGRESS, + "start_time": T0, + "end_time": T0, + } + result = await api.create_step(step_data, log_file=LOG) + assert result._log_file == LOG + + @pytest.mark.asyncio + async def test_create_measurement_stamps_log_file(self, api): + api._low_level_client.create_test_measurement = AsyncMock(return_value=_make_measurement()) + meas_data = { + "test_step_id": "sim-step", + "name": "m", + "measurement_type": TestMeasurementType.BOOLEAN, + "boolean_value": True, + "passed": True, + "timestamp": T0, + } + result = await api.create_measurement(meas_data, log_file=LOG) + assert result._log_file == LOG + + +class TestUpdateStamping: + @pytest.mark.asyncio + async def test_update_stamps_log_file(self, api): + existing = _make_report() + api._low_level_client.update_test_report = AsyncMock(return_value=existing) + result = await api.update( + test_report=existing, update={"status": TestStatus.FAILED}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == LOG + + @pytest.mark.asyncio + async def test_update_step_stamps_log_file(self, api): + existing = _make_step() + api._low_level_client.update_test_step = AsyncMock(return_value=existing) + result = await api.update_step( + test_step=existing, update={"description": "x"}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == LOG + + @pytest.mark.asyncio + async def test_update_measurement_stamps_log_file(self, api): + existing = _make_measurement() + api._low_level_client.update_test_measurement = AsyncMock(return_value=existing) + result = await api.update_measurement( + test_measurement=existing, update={"passed": False}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == LOG + + +CACHED = "/tmp/cached.jsonl" +KWARG = "/tmp/kwarg.jsonl" + + +class TestResourceMethodReadsStampedEntity: + """Resource-level fallback: when no log_file kwarg is passed, read it off + the entity. Symmetric with the entity-level convenience method's behavior. + """ + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), # the fallback + (CACHED, KWARG, KWARG), # kwarg wins + ], + ) + @pytest.mark.asyncio + async def test_update_reads_log_file_from_test_report(self, api, cached, kwarg, expected): + entity = _make_report() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_report = AsyncMock(return_value=entity) + + await api.update(test_report=entity, update={"status": TestStatus.FAILED}, log_file=kwarg) + + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == expected + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), + (CACHED, KWARG, KWARG), + ], + ) + @pytest.mark.asyncio + async def test_update_step_reads_log_file_from_test_step(self, api, cached, kwarg, expected): + entity = _make_step() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_step = AsyncMock(return_value=entity) + + await api.update_step(test_step=entity, update={"description": "x"}, log_file=kwarg) + + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == expected + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), + (CACHED, KWARG, KWARG), + ], + ) + @pytest.mark.asyncio + async def test_update_measurement_reads_log_file_from_test_measurement( + self, api, cached, kwarg, expected + ): + entity = _make_measurement() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_measurement = AsyncMock(return_value=entity) + + await api.update_measurement( + test_measurement=entity, update={"passed": False}, log_file=kwarg + ) + + assert ( + api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == expected + ) + + @pytest.mark.asyncio + async def test_update_with_string_id_has_no_fallback(self, api): + """Passing a bare ID (no entity) means no _log_file to read; the resource + forwards None to the low-level wrapper. + """ + api._low_level_client.update_test_report = AsyncMock(return_value=_make_report()) + await api.update(test_report="some-id", update={"status": TestStatus.FAILED}) + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] is None + + @pytest.mark.asyncio + async def test_update_step_with_string_id_has_no_fallback(self, api): + api._low_level_client.update_test_step = AsyncMock(return_value=_make_step()) + await api.update_step(test_step="some-id", update={"description": "x"}) + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] is None + + +class TestReadPathsDoNotStamp: + """get/list_/find/import_log_file return real entities; they must not carry _log_file.""" + + @pytest.mark.asyncio + async def test_get_does_not_stamp(self, api): + api._low_level_client.get_test_report = AsyncMock(return_value=_make_report("real-id")) + result = await api.get(test_report_id="real-id") + assert result._log_file is None + + @pytest.mark.asyncio + async def test_list_does_not_stamp(self, api): + api._low_level_client.list_all_test_reports = AsyncMock( + return_value=[_make_report("a"), _make_report("b")] + ) + results = await api.list_() + assert all(r._log_file is None for r in results) + + @pytest.mark.asyncio + async def test_import_log_file_does_not_stamp(self, api, tmp_path): + from sift_client._internal.low_level_wrappers.test_results import ReplayResult + + log_path = tmp_path / "log.jsonl" + log_path.touch() + replay_result = ReplayResult( + report=_make_report("real-report"), + steps=[_make_step("real-step")], + measurements=[_make_measurement("real-meas")], + ) + api._low_level_client.import_log_file = AsyncMock(return_value=replay_result) + + result = await api.import_log_file(log_path) + + assert result.report._log_file is None + assert all(s._log_file is None for s in result.steps) + assert all(m._log_file is None for m in result.measurements) + + +class TestEndToEndLogFileRouting: + """Full pipeline: resource -> real low-level client -> actual file write. + + No mocking of the low-level client; the GrpcClient stub is mocked but is + never invoked because the file-write branch in the low-level wrapper + short-circuits before any gRPC call when log_file is set. Proves the + cached-_log_file plumbing reaches the file on disk. + """ + + @pytest.fixture + def real_api(self, mock_client): + """TestResultsAPIAsync wired through a real TestResultsLowLevelClient.""" + return TestResultsAPIAsync(mock_client) + + @pytest.mark.asyncio + async def test_metadata_update_round_trips_through_log_file(self, real_api, tmp_path): + """The actual ENG-11152 regression: update with metadata via cached + _log_file, then read the JSONL line back through the same parser the + replay path uses and verify every key/value round-trips. Proves the + user-visible payload (not just an opaque entry) lands on disk. + """ + from google.protobuf import json_format + from sift.test_reports.v1.test_reports_pb2 import UpdateTestReportRequest + + from sift_client._internal.low_level_wrappers._test_results_log import ( + iter_log_data_lines, + ) + from sift_client.util.metadata import metadata_proto_to_dict + + log_file = tmp_path / "metadata.jsonl" + report_data = { + "status": TestStatus.IN_PROGRESS, + "name": "n", + "test_system_name": "s", + "test_case": "c", + "start_time": T0, + "end_time": T0, + } + report = await real_api.create(report_data, log_file=log_file) + assert report._log_file == log_file + + # Mix of string, number, and boolean to cover all three MetadataValue arms. + metadata = { + "run_id": "run-abc-123", + "operator": "test-user", + "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` + ) + + # Find the UpdateTestReport line and decode it the same way replay does. + update_entries = [ + (rt, rid, js) + for rt, rid, js in iter_log_data_lines(log_file) + if rt == "UpdateTestReport" + ] + assert len(update_entries) == 1 + _, _, json_str = update_entries[0] + + request = UpdateTestReportRequest() + json_format.Parse(json_str, request) + + assert "metadata" in request.update_mask.paths + round_tripped = metadata_proto_to_dict(list(request.test_report.metadata)) + assert round_tripped == metadata + # And confirm we never reached the gRPC stub. + real_api._low_level_client._grpc_client.get_stub.assert_not_called() diff --git a/python/lib/sift_client/resources/test_results.py b/python/lib/sift_client/resources/test_results.py index ad13c2487..7a8344a3f 100644 --- a/python/lib/sift_client/resources/test_results.py +++ b/python/lib/sift_client/resources/test_results.py @@ -259,6 +259,8 @@ async def update( test_report_id = ( test_report._id_or_error if isinstance(test_report, TestReport) else test_report ) + if log_file is None and isinstance(test_report, TestReport): + log_file = test_report._log_file if isinstance(update, dict): update = TestReportUpdate.model_validate(update) @@ -437,6 +439,8 @@ async def update_step( The updated TestStep. """ test_step_id = test_step._id_or_error if isinstance(test_step, TestStep) else test_step + if log_file is None and isinstance(test_step, TestStep): + log_file = test_step._log_file if isinstance(update, dict): update = TestStepUpdate.model_validate(update) @@ -610,6 +614,8 @@ async def update_measurement( Returns: The updated TestMeasurement. """ + if log_file is None: + log_file = test_measurement._log_file if isinstance(update, dict): update = TestMeasurementUpdate.model_validate(update) diff --git a/python/lib/sift_client/util/test_results/context_manager.py b/python/lib/sift_client/util/test_results/context_manager.py index 4c179c060..e304e6247 100644 --- a/python/lib/sift_client/util/test_results/context_manager.py +++ b/python/lib/sift_client/util/test_results/context_manager.py @@ -195,7 +195,7 @@ def __exit__(self, exc_type, exc_value, traceback): update["status"] = TestStatus.FAILED else: update["status"] = TestStatus.PASSED - self.report.update(update, log_file=self.log_file) + self.report.update(update) if self._import_proc is not None: try: @@ -392,7 +392,6 @@ def update_step_from_result( "end_time": datetime.now(timezone.utc), "error_info": error_info, }, - log_file=self.report_context.log_file, ) return result diff --git a/python/lib/sift_client/util/test_results/pytest_util.py b/python/lib/sift_client/util/test_results/pytest_util.py index ae22fa122..ac7ca957a 100644 --- a/python/lib/sift_client/util/test_results/pytest_util.py +++ b/python/lib/sift_client/util/test_results/pytest_util.py @@ -68,9 +68,7 @@ def pytest_runtest_makereport(item: pytest.Item, call: pytest.CallInfo[Any]): # Skipped steps won't invoke the method/fixtures at all, so we need to manually record a step. if REPORT_CONTEXT: with REPORT_CONTEXT.new_step(name=item.name) as new_step: - new_step.current_step.update( - {"status": TestStatus.SKIPPED}, log_file=REPORT_CONTEXT.log_file - ) + new_step.current_step.update({"status": TestStatus.SKIPPED}) setattr(item, "rep_" + report.when, call) From 5c2981d7ed1fb9fc094349eccdd31983a1df03eb Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 11:31:38 -0700 Subject: [PATCH 3/7] minor cleanup --- .../resources/test_test_results_log_file.py | 9 +--- .../_tests/sift_types/test_results.py | 48 ------------------- .../lib/sift_client/resources/test_results.py | 30 ++++++------ .../lib/sift_client/sift_types/test_report.py | 15 ++++-- 4 files changed, 26 insertions(+), 76 deletions(-) diff --git a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py index cabeedc43..922258406 100644 --- a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py +++ b/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 = [ diff --git a/python/lib/sift_client/_tests/sift_types/test_results.py b/python/lib/sift_client/_tests/sift_types/test_results.py index 3e8bb9479..f073fcb4e 100644 --- a/python/lib/sift_client/_tests/sift_types/test_results.py +++ b/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, diff --git a/python/lib/sift_client/resources/test_results.py b/python/lib/sift_client/resources/test_results.py index 7a8344a3f..22e984b5e 100644 --- a/python/lib/sift_client/resources/test_results.py +++ b/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: diff --git a/python/lib/sift_client/sift_types/test_report.py b/python/lib/sift_client/sift_types/test_report.py index e935b1ba5..882c7aab4 100644 --- a/python/lib/sift_client/sift_types/test_report.py +++ b/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 ) From bd561da30e57a7bb917e4d49dcfb8ae5d5c0f4f7 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 11:55:26 -0700 Subject: [PATCH 4/7] comment sanitize --- .../sift_client/_tests/resources/test_test_results_log_file.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py index 922258406..afede540a 100644 --- a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py +++ b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py @@ -303,7 +303,7 @@ def real_api(self, mock_client): @pytest.mark.asyncio async def test_metadata_update_round_trips_through_log_file(self, real_api, tmp_path): - """The actual ENG-11152 regression: update with metadata via cached + """Update with metadata via cached _log_file, then read the JSONL line back through the same parser the replay path uses and verify every key/value round-trips. Proves the user-visible payload (not just an opaque entry) lands on disk. From 7348ad867bba4ed30411f103c9ebcccaa067e790 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 12:11:10 -0700 Subject: [PATCH 5/7] doc update --- python/docs/examples/pytest_plugin.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/python/docs/examples/pytest_plugin.md b/python/docs/examples/pytest_plugin.md index c465b4cb7..cf56dd75e 100644 --- a/python/docs/examples/pytest_plugin.md +++ b/python/docs/examples/pytest_plugin.md @@ -523,10 +523,7 @@ The `unit` argument is a free-form string label (e.g. `"V"`, `"C"`, `"psi"`). def test_runtime_skip(step): with step.substep(name="optional_calibration") as cal: if not precondition_met(): - cal.current_step.update( - {"status": TestStatus.SKIPPED}, - log_file=step.report_context.log_file, - ) + cal.current_step.update({"status": TestStatus.SKIPPED}) ``` A manually-resolved status is honored by the step's exit handler. No From dfa27b356a68b36a478dd57bef9a7aef45f083d4 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 12:12:18 -0700 Subject: [PATCH 6/7] remove comment that isn't part of autogen --- python/pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/pyproject.toml b/python/pyproject.toml index 458b6a2a1..4e0e7165e 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -171,7 +171,7 @@ file-imports = [ ] hdf5 = [ 'h5py~=3.11', - 'polars~=1.8', # only used by sift_py; remove once sift_py is fully deprecated + 'polars~=1.8', ] openssl = [ 'cffi~=1.14', From 5e05bd6a6bc392c008a911ec38121baf4fcdf630 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 13 May 2026 16:28:40 -0700 Subject: [PATCH 7/7] comment and fix a few dev script items --- .../_tests/resources/test_test_results.py | 342 ++++++++++++++++- .../resources/test_test_results_log_file.py | 357 ------------------ python/scripts/dev | 5 +- 3 files changed, 345 insertions(+), 359 deletions(-) delete mode 100644 python/lib/sift_client/_tests/resources/test_test_results_log_file.py diff --git a/python/lib/sift_client/_tests/resources/test_test_results.py b/python/lib/sift_client/_tests/resources/test_test_results.py index 60941bfb9..3ade1d1c3 100644 --- a/python/lib/sift_client/_tests/resources/test_test_results.py +++ b/python/lib/sift_client/_tests/resources/test_test_results.py @@ -4,10 +4,11 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from typing import ClassVar -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock, patch import grpc import pytest +import pytest_asyncio from grpc import aio as aiogrpc from sift_client._internal.low_level_wrappers.test_results import TestResultsLowLevelClient @@ -770,3 +771,342 @@ def updater() -> None: assert sidecar.exists() reloaded = LogTracking.load(log_file) assert reloaded.last_uploaded_line >= 1 + + +T0 = datetime(2026, 1, 1, tzinfo=timezone.utc) + + +def _make_report(id_: str = "sim-report") -> TestReport: + return TestReport( + id_=id_, + status=TestStatus.IN_PROGRESS, + name="n", + test_system_name="s", + test_case="c", + start_time=T0, + end_time=T0, + metadata={}, + is_archived=False, + ) + + +def _make_step(id_: str = "sim-step") -> TestStep: + return TestStep( + id_=id_, + test_report_id="sim-report", + name="step", + step_type=TestStepType.ACTION, + step_path="1", + status=TestStatus.IN_PROGRESS, + start_time=T0, + end_time=T0, + ) + + +def _make_measurement(id_: str = "sim-meas") -> TestMeasurement: + return TestMeasurement( + id_=id_, + measurement_type=TestMeasurementType.BOOLEAN, + name="m", + test_step_id="sim-step", + boolean_value=True, + passed=True, + timestamp=T0, + ) + + +@pytest.fixture +def mock_client(): + client = MagicMock() + client.grpc_client = MagicMock() + client.rest_client = MagicMock() + return client + + +@pytest_asyncio.fixture +def api(mock_client): + """Build a TestResultsAPIAsync with mocked low-level + upload clients.""" + with patch( + "sift_client.resources.test_results.TestResultsLowLevelClient", + autospec=True, + ), patch( + "sift_client.resources.test_results.UploadLowLevelClient", + autospec=True, + ): + return TestResultsAPIAsync(mock_client) + + +LOG = "/tmp/log.jsonl" + + +class TestCreateStamping: + @pytest.mark.asyncio + async def test_create_stamps_log_file(self, api): + api._low_level_client.create_test_report = AsyncMock(return_value=_make_report()) + report_data = { + "status": TestStatus.IN_PROGRESS, + "name": "n", + "test_system_name": "s", + "test_case": "c", + "start_time": T0, + "end_time": T0, + } + result = await api.create(report_data, log_file=LOG) + assert result._log_file == LOG + assert api._low_level_client.create_test_report.call_args.kwargs["log_file"] == LOG + + @pytest.mark.asyncio + async def test_create_step_stamps_log_file(self, api): + api._low_level_client.create_test_step = AsyncMock(return_value=_make_step()) + step_data = { + "test_report_id": "sim-report", + "name": "step", + "step_type": TestStepType.ACTION, + "step_path": "1", + "status": TestStatus.IN_PROGRESS, + "start_time": T0, + "end_time": T0, + } + result = await api.create_step(step_data, log_file=LOG) + assert result._log_file == LOG + + @pytest.mark.asyncio + async def test_create_measurement_stamps_log_file(self, api): + api._low_level_client.create_test_measurement = AsyncMock(return_value=_make_measurement()) + meas_data = { + "test_step_id": "sim-step", + "name": "m", + "measurement_type": TestMeasurementType.BOOLEAN, + "boolean_value": True, + "passed": True, + "timestamp": T0, + } + result = await api.create_measurement(meas_data, log_file=LOG) + assert result._log_file == LOG + + +class TestUpdateStamping: + @pytest.mark.asyncio + async def test_update_stamps_log_file(self, api): + existing = _make_report() + api._low_level_client.update_test_report = AsyncMock(return_value=existing) + result = await api.update( + test_report=existing, update={"status": TestStatus.FAILED}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == LOG + + @pytest.mark.asyncio + async def test_update_step_stamps_log_file(self, api): + existing = _make_step() + api._low_level_client.update_test_step = AsyncMock(return_value=existing) + result = await api.update_step( + test_step=existing, update={"description": "x"}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == LOG + + @pytest.mark.asyncio + async def test_update_measurement_stamps_log_file(self, api): + existing = _make_measurement() + api._low_level_client.update_test_measurement = AsyncMock(return_value=existing) + result = await api.update_measurement( + test_measurement=existing, update={"passed": False}, log_file=LOG + ) + assert result._log_file == LOG + assert api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == LOG + + +CACHED = "/tmp/cached.jsonl" +KWARG = "/tmp/kwarg.jsonl" + + +class TestResourceMethodReadsStampedEntity: + """Resource-level fallback: when no log_file kwarg is passed, read it off + the entity. Symmetric with the entity-level convenience method's behavior. + """ + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), # the fallback + (CACHED, KWARG, KWARG), # kwarg wins + ], + ) + @pytest.mark.asyncio + async def test_update_reads_log_file_from_test_report(self, api, cached, kwarg, expected): + entity = _make_report() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_report = AsyncMock(return_value=entity) + + await api.update(test_report=entity, update={"status": TestStatus.FAILED}, log_file=kwarg) + + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == expected + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), + (CACHED, KWARG, KWARG), + ], + ) + @pytest.mark.asyncio + async def test_update_step_reads_log_file_from_test_step(self, api, cached, kwarg, expected): + entity = _make_step() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_step = AsyncMock(return_value=entity) + + await api.update_step(test_step=entity, update={"description": "x"}, log_file=kwarg) + + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == expected + + @pytest.mark.parametrize( + ("cached", "kwarg", "expected"), + [ + (None, None, None), + (CACHED, None, CACHED), + (CACHED, KWARG, KWARG), + ], + ) + @pytest.mark.asyncio + async def test_update_measurement_reads_log_file_from_test_measurement( + self, api, cached, kwarg, expected + ): + entity = _make_measurement() + if cached is not None: + entity.__dict__["_log_file"] = cached + api._low_level_client.update_test_measurement = AsyncMock(return_value=entity) + + await api.update_measurement( + test_measurement=entity, update={"passed": False}, log_file=kwarg + ) + + assert ( + api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == expected + ) + + @pytest.mark.asyncio + async def test_update_with_string_id_has_no_fallback(self, api): + """Passing a bare ID (no entity) means no _log_file to read; the resource + forwards None to the low-level wrapper. + """ + api._low_level_client.update_test_report = AsyncMock(return_value=_make_report()) + await api.update(test_report="some-id", update={"status": TestStatus.FAILED}) + assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] is None + + @pytest.mark.asyncio + async def test_update_step_with_string_id_has_no_fallback(self, api): + api._low_level_client.update_test_step = AsyncMock(return_value=_make_step()) + await api.update_step(test_step="some-id", update={"description": "x"}) + assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] is None + + +class TestReadPathsDoNotStamp: + """get/list_/find/import_log_file return real entities; they must not carry _log_file.""" + + @pytest.mark.asyncio + async def test_get_does_not_stamp(self, api): + api._low_level_client.get_test_report = AsyncMock(return_value=_make_report("real-id")) + result = await api.get(test_report_id="real-id") + assert result._log_file is None + + @pytest.mark.asyncio + async def test_list_does_not_stamp(self, api): + api._low_level_client.list_all_test_reports = AsyncMock( + return_value=[_make_report("a"), _make_report("b")] + ) + results = await api.list_() + assert all(r._log_file is None for r in results) + + @pytest.mark.asyncio + async def test_import_log_file_does_not_stamp(self, api, tmp_path): + from sift_client._internal.low_level_wrappers.test_results import ReplayResult + + log_path = tmp_path / "log.jsonl" + log_path.touch() + replay_result = ReplayResult( + report=_make_report("real-report"), + steps=[_make_step("real-step")], + measurements=[_make_measurement("real-meas")], + ) + api._low_level_client.import_log_file = AsyncMock(return_value=replay_result) + + result = await api.import_log_file(log_path) + + assert result.report._log_file is None + assert all(s._log_file is None for s in result.steps) + assert all(m._log_file is None for m in result.measurements) + + +class TestEndToEndLogFileRouting: + """Full pipeline: resource -> real low-level client -> actual file write. + + No mocking of the low-level client; the GrpcClient stub is mocked but is + never invoked because the file-write branch in the low-level wrapper + short-circuits before any gRPC call when log_file is set. Proves the + cached-_log_file plumbing reaches the file on disk. + """ + + @pytest.fixture + def real_api(self, mock_client): + """TestResultsAPIAsync wired through a real TestResultsLowLevelClient.""" + return TestResultsAPIAsync(mock_client) + + @pytest.mark.asyncio + async def test_metadata_update_round_trips_through_log_file(self, real_api, tmp_path): + """Update with metadata via cached + _log_file, then read the JSONL line back through the same parser the + replay path uses and verify every key/value round-trips. Proves the + user-visible payload (not just an opaque entry) lands on disk. + """ + from google.protobuf import json_format + from sift.test_reports.v1.test_reports_pb2 import UpdateTestReportRequest + + from sift_client._internal.low_level_wrappers._test_results_log import ( + iter_log_data_lines, + ) + from sift_client.util.metadata import metadata_proto_to_dict + + log_file = tmp_path / "metadata.jsonl" + report_data = { + "status": TestStatus.IN_PROGRESS, + "name": "n", + "test_system_name": "s", + "test_case": "c", + "start_time": T0, + "end_time": T0, + } + report = await real_api.create(report_data, log_file=log_file) + assert report._log_file == log_file + + # Mix of string, number, and boolean to cover all three MetadataValue arms. + metadata = { + "run_id": "run-abc-123", + "operator": "test-user", + "trial_number": 42.5, + "is_dry_run": True, + } + # 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 = [ + (rt, rid, js) + for rt, rid, js in iter_log_data_lines(log_file) + if rt == "UpdateTestReport" + ] + assert len(update_entries) == 1 + _, _, json_str = update_entries[0] + + request = UpdateTestReportRequest() + json_format.Parse(json_str, request) + + assert "metadata" in request.update_mask.paths + round_tripped = metadata_proto_to_dict(list(request.test_report.metadata)) + assert round_tripped == metadata + # And confirm we never reached the gRPC stub. + real_api._low_level_client._grpc_client.get_stub.assert_not_called() diff --git a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py b/python/lib/sift_client/_tests/resources/test_test_results_log_file.py deleted file mode 100644 index afede540a..000000000 --- a/python/lib/sift_client/_tests/resources/test_test_results_log_file.py +++ /dev/null @@ -1,357 +0,0 @@ -"""Unit tests for log_file plumbing in TestResultsAPIAsync.""" - -from __future__ import annotations - -from datetime import datetime, timezone -from unittest.mock import AsyncMock, MagicMock, patch - -import pytest -import pytest_asyncio - -from sift_client.resources.test_results import TestResultsAPIAsync -from sift_client.sift_types.test_report import ( - TestMeasurement, - TestMeasurementType, - TestReport, - TestStatus, - TestStep, - TestStepType, -) - -T0 = datetime(2026, 1, 1, tzinfo=timezone.utc) - - -def _make_report(id_: str = "sim-report") -> TestReport: - return TestReport( - id_=id_, - status=TestStatus.IN_PROGRESS, - name="n", - test_system_name="s", - test_case="c", - start_time=T0, - end_time=T0, - metadata={}, - is_archived=False, - ) - - -def _make_step(id_: str = "sim-step") -> TestStep: - return TestStep( - id_=id_, - test_report_id="sim-report", - name="step", - step_type=TestStepType.ACTION, - step_path="1", - status=TestStatus.IN_PROGRESS, - start_time=T0, - end_time=T0, - ) - - -def _make_measurement(id_: str = "sim-meas") -> TestMeasurement: - return TestMeasurement( - id_=id_, - measurement_type=TestMeasurementType.BOOLEAN, - name="m", - test_step_id="sim-step", - boolean_value=True, - passed=True, - timestamp=T0, - ) - - -@pytest.fixture -def mock_client(): - client = MagicMock() - client.grpc_client = MagicMock() - client.rest_client = MagicMock() - return client - - -@pytest_asyncio.fixture -def api(mock_client): - """Build a TestResultsAPIAsync with mocked low-level + upload clients.""" - with patch( - "sift_client.resources.test_results.TestResultsLowLevelClient", - autospec=True, - ), patch( - "sift_client.resources.test_results.UploadLowLevelClient", - autospec=True, - ): - return TestResultsAPIAsync(mock_client) - - -LOG = "/tmp/log.jsonl" - - -class TestCreateStamping: - @pytest.mark.asyncio - async def test_create_stamps_log_file(self, api): - api._low_level_client.create_test_report = AsyncMock(return_value=_make_report()) - report_data = { - "status": TestStatus.IN_PROGRESS, - "name": "n", - "test_system_name": "s", - "test_case": "c", - "start_time": T0, - "end_time": T0, - } - result = await api.create(report_data, log_file=LOG) - assert result._log_file == LOG - assert api._low_level_client.create_test_report.call_args.kwargs["log_file"] == LOG - - @pytest.mark.asyncio - async def test_create_step_stamps_log_file(self, api): - api._low_level_client.create_test_step = AsyncMock(return_value=_make_step()) - step_data = { - "test_report_id": "sim-report", - "name": "step", - "step_type": TestStepType.ACTION, - "step_path": "1", - "status": TestStatus.IN_PROGRESS, - "start_time": T0, - "end_time": T0, - } - result = await api.create_step(step_data, log_file=LOG) - assert result._log_file == LOG - - @pytest.mark.asyncio - async def test_create_measurement_stamps_log_file(self, api): - api._low_level_client.create_test_measurement = AsyncMock(return_value=_make_measurement()) - meas_data = { - "test_step_id": "sim-step", - "name": "m", - "measurement_type": TestMeasurementType.BOOLEAN, - "boolean_value": True, - "passed": True, - "timestamp": T0, - } - result = await api.create_measurement(meas_data, log_file=LOG) - assert result._log_file == LOG - - -class TestUpdateStamping: - @pytest.mark.asyncio - async def test_update_stamps_log_file(self, api): - existing = _make_report() - api._low_level_client.update_test_report = AsyncMock(return_value=existing) - result = await api.update( - test_report=existing, update={"status": TestStatus.FAILED}, log_file=LOG - ) - assert result._log_file == LOG - assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == LOG - - @pytest.mark.asyncio - async def test_update_step_stamps_log_file(self, api): - existing = _make_step() - api._low_level_client.update_test_step = AsyncMock(return_value=existing) - result = await api.update_step( - test_step=existing, update={"description": "x"}, log_file=LOG - ) - assert result._log_file == LOG - assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == LOG - - @pytest.mark.asyncio - async def test_update_measurement_stamps_log_file(self, api): - existing = _make_measurement() - api._low_level_client.update_test_measurement = AsyncMock(return_value=existing) - result = await api.update_measurement( - test_measurement=existing, update={"passed": False}, log_file=LOG - ) - assert result._log_file == LOG - assert api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == LOG - - -CACHED = "/tmp/cached.jsonl" -KWARG = "/tmp/kwarg.jsonl" - - -class TestResourceMethodReadsStampedEntity: - """Resource-level fallback: when no log_file kwarg is passed, read it off - the entity. Symmetric with the entity-level convenience method's behavior. - """ - - @pytest.mark.parametrize( - ("cached", "kwarg", "expected"), - [ - (None, None, None), - (CACHED, None, CACHED), # the fallback - (CACHED, KWARG, KWARG), # kwarg wins - ], - ) - @pytest.mark.asyncio - async def test_update_reads_log_file_from_test_report(self, api, cached, kwarg, expected): - entity = _make_report() - if cached is not None: - entity.__dict__["_log_file"] = cached - api._low_level_client.update_test_report = AsyncMock(return_value=entity) - - await api.update(test_report=entity, update={"status": TestStatus.FAILED}, log_file=kwarg) - - assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] == expected - - @pytest.mark.parametrize( - ("cached", "kwarg", "expected"), - [ - (None, None, None), - (CACHED, None, CACHED), - (CACHED, KWARG, KWARG), - ], - ) - @pytest.mark.asyncio - async def test_update_step_reads_log_file_from_test_step(self, api, cached, kwarg, expected): - entity = _make_step() - if cached is not None: - entity.__dict__["_log_file"] = cached - api._low_level_client.update_test_step = AsyncMock(return_value=entity) - - await api.update_step(test_step=entity, update={"description": "x"}, log_file=kwarg) - - assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] == expected - - @pytest.mark.parametrize( - ("cached", "kwarg", "expected"), - [ - (None, None, None), - (CACHED, None, CACHED), - (CACHED, KWARG, KWARG), - ], - ) - @pytest.mark.asyncio - async def test_update_measurement_reads_log_file_from_test_measurement( - self, api, cached, kwarg, expected - ): - entity = _make_measurement() - if cached is not None: - entity.__dict__["_log_file"] = cached - api._low_level_client.update_test_measurement = AsyncMock(return_value=entity) - - await api.update_measurement( - test_measurement=entity, update={"passed": False}, log_file=kwarg - ) - - assert ( - api._low_level_client.update_test_measurement.call_args.kwargs["log_file"] == expected - ) - - @pytest.mark.asyncio - async def test_update_with_string_id_has_no_fallback(self, api): - """Passing a bare ID (no entity) means no _log_file to read; the resource - forwards None to the low-level wrapper. - """ - api._low_level_client.update_test_report = AsyncMock(return_value=_make_report()) - await api.update(test_report="some-id", update={"status": TestStatus.FAILED}) - assert api._low_level_client.update_test_report.call_args.kwargs["log_file"] is None - - @pytest.mark.asyncio - async def test_update_step_with_string_id_has_no_fallback(self, api): - api._low_level_client.update_test_step = AsyncMock(return_value=_make_step()) - await api.update_step(test_step="some-id", update={"description": "x"}) - assert api._low_level_client.update_test_step.call_args.kwargs["log_file"] is None - - -class TestReadPathsDoNotStamp: - """get/list_/find/import_log_file return real entities; they must not carry _log_file.""" - - @pytest.mark.asyncio - async def test_get_does_not_stamp(self, api): - api._low_level_client.get_test_report = AsyncMock(return_value=_make_report("real-id")) - result = await api.get(test_report_id="real-id") - assert result._log_file is None - - @pytest.mark.asyncio - async def test_list_does_not_stamp(self, api): - api._low_level_client.list_all_test_reports = AsyncMock( - return_value=[_make_report("a"), _make_report("b")] - ) - results = await api.list_() - assert all(r._log_file is None for r in results) - - @pytest.mark.asyncio - async def test_import_log_file_does_not_stamp(self, api, tmp_path): - from sift_client._internal.low_level_wrappers.test_results import ReplayResult - - log_path = tmp_path / "log.jsonl" - log_path.touch() - replay_result = ReplayResult( - report=_make_report("real-report"), - steps=[_make_step("real-step")], - measurements=[_make_measurement("real-meas")], - ) - api._low_level_client.import_log_file = AsyncMock(return_value=replay_result) - - result = await api.import_log_file(log_path) - - assert result.report._log_file is None - assert all(s._log_file is None for s in result.steps) - assert all(m._log_file is None for m in result.measurements) - - -class TestEndToEndLogFileRouting: - """Full pipeline: resource -> real low-level client -> actual file write. - - No mocking of the low-level client; the GrpcClient stub is mocked but is - never invoked because the file-write branch in the low-level wrapper - short-circuits before any gRPC call when log_file is set. Proves the - cached-_log_file plumbing reaches the file on disk. - """ - - @pytest.fixture - def real_api(self, mock_client): - """TestResultsAPIAsync wired through a real TestResultsLowLevelClient.""" - return TestResultsAPIAsync(mock_client) - - @pytest.mark.asyncio - async def test_metadata_update_round_trips_through_log_file(self, real_api, tmp_path): - """Update with metadata via cached - _log_file, then read the JSONL line back through the same parser the - replay path uses and verify every key/value round-trips. Proves the - user-visible payload (not just an opaque entry) lands on disk. - """ - from google.protobuf import json_format - from sift.test_reports.v1.test_reports_pb2 import UpdateTestReportRequest - - from sift_client._internal.low_level_wrappers._test_results_log import ( - iter_log_data_lines, - ) - from sift_client.util.metadata import metadata_proto_to_dict - - log_file = tmp_path / "metadata.jsonl" - report_data = { - "status": TestStatus.IN_PROGRESS, - "name": "n", - "test_system_name": "s", - "test_case": "c", - "start_time": T0, - "end_time": T0, - } - report = await real_api.create(report_data, log_file=log_file) - assert report._log_file == log_file - - # Mix of string, number, and boolean to cover all three MetadataValue arms. - metadata = { - "run_id": "run-abc-123", - "operator": "test-user", - "trial_number": 42.5, - "is_dry_run": True, - } - # 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 = [ - (rt, rid, js) - for rt, rid, js in iter_log_data_lines(log_file) - if rt == "UpdateTestReport" - ] - assert len(update_entries) == 1 - _, _, json_str = update_entries[0] - - request = UpdateTestReportRequest() - json_format.Parse(json_str, request) - - assert "metadata" in request.update_mask.paths - round_tripped = metadata_proto_to_dict(list(request.test_report.metadata)) - assert round_tripped == metadata - # And confirm we never reached the gRPC stub. - real_api._low_level_client._grpc_client.get_stub.assert_not_called() diff --git a/python/scripts/dev b/python/scripts/dev index 39cb5926b..5a1397803 100755 --- a/python/scripts/dev +++ b/python/scripts/dev @@ -35,7 +35,7 @@ EOT pip_install() { source venv/bin/activate - pip install '.[all-dev]' + pip install '.[dev-all]' pip install -e . } @@ -94,14 +94,17 @@ doc_build() { } run_tests() { + source venv/bin/activate pytest -m "not integration" } run_tests_integration() { + source venv/bin/activate pytest -m "integration" } run_tests_all() { + source venv/bin/activate pytest }