diff --git a/applications/modelling_e2e/handler.py b/applications/modelling_e2e/handler.py index 0f2c4fe6..6e3dece5 100644 --- a/applications/modelling_e2e/handler.py +++ b/applications/modelling_e2e/handler.py @@ -453,22 +453,29 @@ def handler(body: dict[str, Any], context: Any) -> Optional[dict[str, Any]]: f"{[s['certificate_number'] for s in skipped_certs]}" ) - if failures: - failed_ids = [f["property_id"] for f in failures] - # Persisted verbatim into the subtask's outputs.error (via - # SubTask.fail), so include each property's error type + message — - # not just the IDs — to make failed runs diagnosable without - # cross-referencing CloudWatch. - message = ( - f"failed property_ids: {failed_ids}; " - f"details: {json.dumps(failures)}" - ) - if skipped_certs: - message += ( - f"; skipped_unmappable_cohort_certs: {json.dumps(skipped_certs)}" + # A property that errored AND a cohort cert the mapper could not consume + # are both surfaced as failures, so the subtask is marked failed and + # shows up for debugging. The whole batch has already run by this point — + # every property that could be modelled was written to DB above — so + # failing here flags the run without discarding the work done so far. + if failures or skipped_certs: + parts: list[str] = [] + if failures: + failed_ids = [f["property_id"] for f in failures] + # Persisted verbatim into the subtask's outputs.error (via + # SubTask.fail): include each property's error type + message, + # not just the IDs, so failed runs are diagnosable without + # cross-referencing CloudWatch. + parts.append( + f"failed property_ids: {failed_ids}; " + f"details: {json.dumps(failures)}" ) - raise RuntimeError(message) + if skipped_certs: + parts.append( + f"skipped_unmappable_cohort_certs: {json.dumps(skipped_certs)}" + ) + raise RuntimeError("; ".join(parts)) - return {"skipped_unmappable_cohort_certs": skipped_certs} if skipped_certs else None + return None finally: read_session.close() diff --git a/tests/applications/modelling_e2e/test_handler.py b/tests/applications/modelling_e2e/test_handler.py index 6929d13f..540d6040 100644 --- a/tests/applications/modelling_e2e/test_handler.py +++ b/tests/applications/modelling_e2e/test_handler.py @@ -214,16 +214,19 @@ def test_lodged_epc_path_saves_epc_plan_and_marks_modelled( _baseline_orchestrator.return_value.run.assert_called_once_with([PROPERTY_ID]) -def test_skipped_cohort_certs_are_surfaced_in_the_outputs() -> None: +def test_skipped_cohort_certs_fail_the_subtask_but_the_plan_is_still_saved() -> None: """Cohort certs the mapper can't consume are skipped (so prediction is not - aborted) and surfaced — with cert numbers — in the subtask outputs, so the - mapper gaps can be reported and closed.""" + aborted), then surfaced as a failure — the subtask is marked failed (the + cert numbers land in outputs.error via the raised RuntimeError) so the + mapper gaps get debugged. The batch still ran to completion first, so the + property's plan was committed before the handler raised.""" from repositories.comparable_properties.epc_comparable_properties_repository import ( SkippedCohortCert, ) mock_engine = _engine_mock([PROPERTY_ID], [UPRN], [POSTCODE]) mock_plan = _plan_mock() + mock_uow = MagicMock() skipped = [ SkippedCohortCert( certificate_number="8257-7539-1649-0633-4992", @@ -287,24 +290,21 @@ def test_skipped_cohort_certs_are_surfaced_in_the_outputs() -> None: MockUoW = stack.enter_context( patch("applications.modelling_e2e.handler.PostgresUnitOfWork") ) - MockUoW.return_value.__enter__.return_value = MagicMock() + MockUoW.return_value.__enter__.return_value = mock_uow MockUoW.return_value.__exit__.return_value = False - # Act - result = _call_handler(_BODY) + # Act — the skipped cert fails the subtask, but only after the batch ran. + with pytest.raises(RuntimeError) as excinfo: + _call_handler(_BODY) - # Assert — the handler's return (→ subtask outputs.result) carries the cert - # numbers + errors of every skipped cohort cert. - assert result == { - "skipped_unmappable_cohort_certs": [ - { - "certificate_number": "8257-7539-1649-0633-4992", - "error": ( - "ValueError: RdSapSchema17_1: missing required field 'window'" - ), - } - ] - } + # Assert — the cert number + error reach outputs.error (the raised message), + # and the property's plan was still committed before the handler raised. + message = str(excinfo.value) + assert "skipped_unmappable_cohort_certs" in message + assert "8257-7539-1649-0633-4992" in message + assert "RdSapSchema17_1: missing required field 'window'" in message + mock_uow.plan.save.assert_called_once() + mock_uow.commit.assert_called_once() # ---------------------------------------------------------------------------