From f41fa80e89fb5841fe9d41b5b2d23b568ae59431 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 09:36:34 +0100 Subject: [PATCH 1/5] feat: add Home Assistant Event entity for command errors Closes #272. When a command to the vehicle fails, a JSON event is now published to the command/error topic so Home Assistant users can build automations around command failures. Changes: - Add _publish_event() to HA discovery base for MQTT Event entities - Register a "Command error" diagnostic event entity in discovery - Publish error events from all failure paths in VehicleCommandHandler - Wrap error event publishing in try-except to prevent masking original errors - Cover the "no handler found" path with error events too Co-Authored-By: Claude Opus 4.6 (1M context) --- src/handlers/vehicle_command.py | 30 ++++++++++++++++++-- src/integrations/home_assistant/base.py | 28 ++++++++++++++++++ src/integrations/home_assistant/discovery.py | 9 ++++++ src/mqtt_topics.py | 3 ++ 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/handlers/vehicle_command.py b/src/handlers/vehicle_command.py index 10917566..75a3455f 100644 --- a/src/handlers/vehicle_command.py +++ b/src/handlers/vehicle_command.py @@ -2,7 +2,7 @@ from dataclasses import dataclass import logging -from typing import TYPE_CHECKING, Final +from typing import TYPE_CHECKING, Any, Final from saic_ismart_client_ng.exceptions import SaicApiException, SaicLogoutException @@ -57,6 +57,22 @@ def __init__( def publisher(self) -> Publisher: return self.vehicle_state.publisher + def __publish_command_error(self, command: str, detail: str) -> None: + try: + error_topic = self.vehicle_state.get_topic(mqtt_topics.COMMAND_ERROR) + event_payload: dict[str, Any] = { + "event_type": "command_error", + "command": command, + "detail": detail, + } + self.publisher.publish_json(error_topic, event_payload) + except Exception: + LOG.warning( + "Failed to publish command error event for command %s", + command, + exc_info=True, + ) + async def handle_mqtt_command(self, *, topic: str, payload: str) -> None: analyzed_topic = self.__get_command_topics(topic) handler = self.__command_handlers.get(analyzed_topic.command_no_vin) @@ -64,6 +80,7 @@ async def handle_mqtt_command(self, *, topic: str, payload: str) -> None: msg = f"No handler found for command topic {analyzed_topic.command_no_vin}" self.publisher.publish_str(analyzed_topic.response_no_global, msg) LOG.error(msg) + self.__publish_command_error(analyzed_topic.command_no_vin, msg) else: await self.__execute_mqtt_command_handler( handler=handler, payload=payload, analyzed_topic=analyzed_topic @@ -92,6 +109,7 @@ async def __execute_mqtt_command_handler( except MqttGatewayException as e: self.publisher.publish_str(result_topic, f"Failed: {e.message}") LOG.exception(e.message, exc_info=e) + self.__publish_command_error(topic, e.message) except SaicLogoutException: LOG.warning( "API Client was logged out, attempting immediate relogin and retry" @@ -99,10 +117,12 @@ async def __execute_mqtt_command_handler( try: await self.relogin_handler.force_login() except Exception as login_err: + detail = f"relogin failed ({login_err})" self.publisher.publish_str( - result_topic, f"Failed: relogin failed ({login_err})" + result_topic, f"Failed: {detail}" ) LOG.error("Immediate relogin failed", exc_info=login_err) + self.__publish_command_error(topic, detail) return try: execution_result = await handler.handle(payload) @@ -115,20 +135,24 @@ async def __execute_mqtt_command_handler( if execution_result.clear_command: self.publisher.clear_topic(topic_no_global) except Exception as retry_err: + detail = str(retry_err) self.publisher.publish_str( - result_topic, f"Failed: {retry_err}" + result_topic, f"Failed: {detail}" ) LOG.error( "Command retry after relogin failed", exc_info=retry_err ) + self.__publish_command_error(topic, detail) except SaicApiException as se: self.publisher.publish_str(result_topic, f"Failed: {se.message}") LOG.exception(se.message, exc_info=se) + self.__publish_command_error(topic, se.message) except Exception as se: self.publisher.publish_str(result_topic, "Failed unexpectedly") LOG.exception( "handle_mqtt_command failed with an unexpected exception", exc_info=se ) + self.__publish_command_error(topic, str(se)) def __get_command_topics(self, topic: str) -> _MqttCommandTopic: global_topic_removed = topic.removeprefix(self.global_mqtt_topic).removeprefix( diff --git a/src/integrations/home_assistant/base.py b/src/integrations/home_assistant/base.py index 0c395104..848eef63 100644 --- a/src/integrations/home_assistant/base.py +++ b/src/integrations/home_assistant/base.py @@ -230,6 +230,34 @@ def _publish_lock( "lock", name, payload, custom_availability ) + def _publish_event( + self, + topic: str, + name: str, + event_types: list[str], + *, + enabled: bool = True, + entity_category: str | None = None, + device_class: str | None = None, + icon: str | None = None, + custom_availability: HaCustomAvailabilityConfig | None = None, + ) -> str: + payload: dict[str, Any] = { + "state_topic": self._get_state_topic(topic), + "event_types": event_types, + "enabled_by_default": enabled, + } + if entity_category is not None: + payload["entity_category"] = entity_category + if device_class is not None: + payload["device_class"] = device_class + if icon is not None: + payload["icon"] = icon + + return self._publish_ha_discovery_message( + "event", name, payload, custom_availability + ) + def _publish_sensor( self, topic: str, diff --git a/src/integrations/home_assistant/discovery.py b/src/integrations/home_assistant/discovery.py index 0e076850..ba1e9cd7 100644 --- a/src/integrations/home_assistant/discovery.py +++ b/src/integrations/home_assistant/discovery.py @@ -327,6 +327,15 @@ def __publish_ha_discovery_messages_real(self) -> None: ) self.__publish_lights_sensors() + # Command error event + self._publish_event( + mqtt_topics.COMMAND_ERROR, + "Command error", + ["command_error"], + entity_category="diagnostic", + icon="mdi:alert-circle", + ) + LOG.debug("Completed publishing Home Assistant discovery messages") def __publish_drivetrain_charging_sensors(self) -> None: diff --git a/src/mqtt_topics.py b/src/mqtt_topics.py index 18c2b1b9..691a9c2c 100644 --- a/src/mqtt_topics.py +++ b/src/mqtt_topics.py @@ -179,4 +179,7 @@ TYRES_REAR_LEFT_PRESSURE = TYRES + "/rearLeftPressure" TYRES_REAR_RIGHT_PRESSURE = TYRES + "/rearRightPressure" +COMMAND = "command" +COMMAND_ERROR = COMMAND + "/error" + VEHICLES = "vehicles" From d59b2f6e936e46f5e48655c0ee5f347ddd71fd21 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 09:39:17 +0100 Subject: [PATCH 2/5] refactor: consolidate command failure reporting into single helper Replace repeated publish_str + LOG + publish_command_error calls across all except blocks with a single __report_command_failure method. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/handlers/vehicle_command.py | 62 +++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/src/handlers/vehicle_command.py b/src/handlers/vehicle_command.py index 75a3455f..61a18e35 100644 --- a/src/handlers/vehicle_command.py +++ b/src/handlers/vehicle_command.py @@ -57,7 +57,16 @@ def __init__( def publisher(self) -> Publisher: return self.vehicle_state.publisher - def __publish_command_error(self, command: str, detail: str) -> None: + def __report_command_failure( + self, + *, + command: str, + result_topic: str, + detail: str, + exc: Exception, + ) -> None: + self.publisher.publish_str(result_topic, f"Failed: {detail}") + LOG.exception("Command %s failed: %s", command, detail, exc_info=exc) try: error_topic = self.vehicle_state.get_topic(mqtt_topics.COMMAND_ERROR) event_payload: dict[str, Any] = { @@ -78,9 +87,12 @@ async def handle_mqtt_command(self, *, topic: str, payload: str) -> None: handler = self.__command_handlers.get(analyzed_topic.command_no_vin) if not handler: msg = f"No handler found for command topic {analyzed_topic.command_no_vin}" - self.publisher.publish_str(analyzed_topic.response_no_global, msg) - LOG.error(msg) - self.__publish_command_error(analyzed_topic.command_no_vin, msg) + self.__report_command_failure( + command=analyzed_topic.command_no_vin, + result_topic=analyzed_topic.response_no_global, + detail=msg, + exc=ValueError(msg), + ) else: await self.__execute_mqtt_command_handler( handler=handler, payload=payload, analyzed_topic=analyzed_topic @@ -107,9 +119,9 @@ async def __execute_mqtt_command_handler( if execution_result.clear_command: self.publisher.clear_topic(topic_no_global) except MqttGatewayException as e: - self.publisher.publish_str(result_topic, f"Failed: {e.message}") - LOG.exception(e.message, exc_info=e) - self.__publish_command_error(topic, e.message) + self.__report_command_failure( + command=topic, result_topic=result_topic, detail=e.message, exc=e + ) except SaicLogoutException: LOG.warning( "API Client was logged out, attempting immediate relogin and retry" @@ -117,12 +129,12 @@ async def __execute_mqtt_command_handler( try: await self.relogin_handler.force_login() except Exception as login_err: - detail = f"relogin failed ({login_err})" - self.publisher.publish_str( - result_topic, f"Failed: {detail}" + self.__report_command_failure( + command=topic, + result_topic=result_topic, + detail=f"relogin failed ({login_err})", + exc=login_err, ) - LOG.error("Immediate relogin failed", exc_info=login_err) - self.__publish_command_error(topic, detail) return try: execution_result = await handler.handle(payload) @@ -135,24 +147,20 @@ async def __execute_mqtt_command_handler( if execution_result.clear_command: self.publisher.clear_topic(topic_no_global) except Exception as retry_err: - detail = str(retry_err) - self.publisher.publish_str( - result_topic, f"Failed: {detail}" - ) - LOG.error( - "Command retry after relogin failed", exc_info=retry_err + self.__report_command_failure( + command=topic, + result_topic=result_topic, + detail=str(retry_err), + exc=retry_err, ) - self.__publish_command_error(topic, detail) except SaicApiException as se: - self.publisher.publish_str(result_topic, f"Failed: {se.message}") - LOG.exception(se.message, exc_info=se) - self.__publish_command_error(topic, se.message) - except Exception as se: - self.publisher.publish_str(result_topic, "Failed unexpectedly") - LOG.exception( - "handle_mqtt_command failed with an unexpected exception", exc_info=se + self.__report_command_failure( + command=topic, result_topic=result_topic, detail=se.message, exc=se + ) + except Exception as e: + self.__report_command_failure( + command=topic, result_topic=result_topic, detail=str(e), exc=e ) - self.__publish_command_error(topic, str(se)) def __get_command_topics(self, topic: str) -> _MqttCommandTopic: global_topic_removed = topic.removeprefix(self.global_mqtt_topic).removeprefix( From 5b51bfcf67e1e6324f23dd0bcf8ebe45223364c9 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 09:42:57 +0100 Subject: [PATCH 3/5] fix: harden __report_command_failure against publish errors - Log the original error before attempting any MQTT publish - Wrap both publish_str and publish_json in try-except - Make exc optional so "no handler found" uses LOG.error (no traceback) - Use safe "unexpected error" detail for the catch-all exception path Co-Authored-By: Claude Opus 4.6 (1M context) --- src/handlers/vehicle_command.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/handlers/vehicle_command.py b/src/handlers/vehicle_command.py index 61a18e35..b0730eb4 100644 --- a/src/handlers/vehicle_command.py +++ b/src/handlers/vehicle_command.py @@ -63,10 +63,20 @@ def __report_command_failure( command: str, result_topic: str, detail: str, - exc: Exception, + exc: Exception | None = None, ) -> None: - self.publisher.publish_str(result_topic, f"Failed: {detail}") - LOG.exception("Command %s failed: %s", command, detail, exc_info=exc) + if exc is not None: + LOG.exception("Command %s failed: %s", command, detail, exc_info=exc) + else: + LOG.error("Command %s failed: %s", command, detail) + try: + self.publisher.publish_str(result_topic, f"Failed: {detail}") + except Exception: + LOG.warning( + "Failed to publish failure result for command %s", + command, + exc_info=True, + ) try: error_topic = self.vehicle_state.get_topic(mqtt_topics.COMMAND_ERROR) event_payload: dict[str, Any] = { @@ -91,7 +101,6 @@ async def handle_mqtt_command(self, *, topic: str, payload: str) -> None: command=analyzed_topic.command_no_vin, result_topic=analyzed_topic.response_no_global, detail=msg, - exc=ValueError(msg), ) else: await self.__execute_mqtt_command_handler( @@ -159,7 +168,10 @@ async def __execute_mqtt_command_handler( ) except Exception as e: self.__report_command_failure( - command=topic, result_topic=result_topic, detail=str(e), exc=e + command=topic, + result_topic=result_topic, + detail="unexpected error", + exc=e, ) def __get_command_topics(self, topic: str) -> _MqttCommandTopic: From 8747f062a73df060c290e9b41d41b6948c432386 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 09:46:20 +0100 Subject: [PATCH 4/5] test: add tests for command error event publishing Cover all error paths in VehicleCommandHandler: - Success path (no error event) - No handler found (error event without traceback) - MqttGatewayException, SaicApiException, unexpected Exception - SaicLogoutException: relogin success/failure, retry failure - Resilience: publish_str/publish_json failures don't propagate - Event payload structure and topic verification Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/handlers/test_vehicle_command.py | 293 +++++++++++++++++++++++++ 1 file changed, 293 insertions(+) create mode 100644 tests/handlers/test_vehicle_command.py diff --git a/tests/handlers/test_vehicle_command.py b/tests/handlers/test_vehicle_command.py new file mode 100644 index 00000000..29da9b20 --- /dev/null +++ b/tests/handlers/test_vehicle_command.py @@ -0,0 +1,293 @@ +from __future__ import annotations + +import unittest +from unittest.mock import AsyncMock, MagicMock, patch + +from saic_ismart_client_ng.exceptions import SaicApiException, SaicLogoutException + +from exceptions import MqttGatewayException +from handlers.command.base import RESULT_REFRESH_AND_CLEAR +from handlers.vehicle_command import VehicleCommandHandler +import mqtt_topics + +MQTT_TOPIC = "saic" +VIN = "vin_test_000000000" +VEHICLE_PREFIX = f"vehicles/{VIN}" + + +def _make_handler( + *, + vehicle_state: MagicMock | None = None, + saic_api: AsyncMock | None = None, + relogin_handler: AsyncMock | None = None, +) -> VehicleCommandHandler: + if vehicle_state is None: + vehicle_state = MagicMock() + vehicle_state.publisher = MagicMock() + vehicle_state.get_topic.side_effect = lambda t: f"{VEHICLE_PREFIX}/{t}" + if saic_api is None: + saic_api = AsyncMock() + if relogin_handler is None: + relogin_handler = AsyncMock() + return VehicleCommandHandler( + vehicle_state=vehicle_state, + saic_api=saic_api, + relogin_handler=relogin_handler, + mqtt_topic=MQTT_TOPIC, + vehicle_prefix=VEHICLE_PREFIX, + ) + + +def _command_topic(command_topic_suffix: str) -> str: + return f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{command_topic_suffix}" + + +def _result_topic(command_topic_suffix: str) -> str: + base = command_topic_suffix.removesuffix(mqtt_topics.SET_SUFFIX).removesuffix("/") + return f"{VEHICLE_PREFIX}/{base}/{mqtt_topics.RESULT_SUFFIX}" + + +class TestVehicleCommandSuccess(unittest.IsolatedAsyncioTestCase): + async def test_successful_command_publishes_success(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_any_call(result_topic, "Success") + handler.publisher.publish_json.assert_not_called() + + +class TestVehicleCommandNoHandler(unittest.IsolatedAsyncioTestCase): + async def test_no_handler_publishes_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic("nonexistent/topic/set") + result_topic = _result_topic("nonexistent/topic/set") + + await handler.handle_mqtt_command(topic=topic, payload="test") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, + "Failed: No handler found for command topic nonexistent/topic/set", + ) + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["event_type"] == "command_error" + assert event_payload["command"] == "nonexistent/topic/set" + + async def test_no_handler_does_not_log_traceback(self) -> None: + handler = _make_handler() + topic = _command_topic("nonexistent/topic/set") + + with patch("handlers.vehicle_command.LOG") as mock_log: + await handler.handle_mqtt_command(topic=topic, payload="test") + mock_log.error.assert_called_once() + mock_log.exception.assert_not_called() + + +class TestVehicleCommandMqttGatewayException(unittest.IsolatedAsyncioTestCase): + async def test_mqtt_gateway_exception_publishes_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=MqttGatewayException("test error") + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, "Failed: test error" + ) + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["event_type"] == "command_error" + assert event_payload["detail"] == "test error" + + +class TestVehicleCommandSaicApiException(unittest.IsolatedAsyncioTestCase): + async def test_saic_api_exception_publishes_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=SaicApiException("api error", return_code=8) + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, "Failed: return code: 8, message: api error" + ) + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["detail"] == "return code: 8, message: api error" + + +class TestVehicleCommandUnexpectedException(unittest.IsolatedAsyncioTestCase): + async def test_unexpected_exception_uses_safe_detail(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=RuntimeError("secret internal detail") + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, "Failed: unexpected error" + ) + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["detail"] == "unexpected error" + assert "secret" not in event_payload["detail"] + + +class TestVehicleCommandSaicLogoutException(unittest.IsolatedAsyncioTestCase): + async def test_logout_relogin_success_retries_command(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=[SaicLogoutException("logged out"), RESULT_REFRESH_AND_CLEAR] + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.relogin_handler.force_login.assert_awaited_once() + assert cmd_handler.handle.await_count == 2 + handler.publisher.publish_str.assert_any_call(result_topic, "Success") + handler.publisher.publish_json.assert_not_called() + + async def test_logout_relogin_failure_publishes_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock(side_effect=SaicLogoutException("logged out")) + handler.relogin_handler.force_login = AsyncMock( + side_effect=Exception("login failed") + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, "Failed: relogin failed (login failed)" + ) + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert "relogin failed" in event_payload["detail"] + + async def test_logout_retry_failure_publishes_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=[SaicLogoutException("logged out"), RuntimeError("retry boom")] + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once_with( + result_topic, "Failed: retry boom" + ) + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["detail"] == "retry boom" + + +class TestReportCommandFailureResilience(unittest.IsolatedAsyncioTestCase): + async def test_publish_str_failure_does_not_prevent_error_event(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=MqttGatewayException("cmd error") + ) + handler.publisher.publish_str.side_effect = ConnectionError("broker down") + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_json.assert_called_once() + event_payload = handler.publisher.publish_json.call_args[0][1] + assert event_payload["detail"] == "cmd error" + + async def test_publish_json_failure_does_not_raise(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=MqttGatewayException("cmd error") + ) + handler.publisher.publish_json.side_effect = ConnectionError("broker down") + + await handler.handle_mqtt_command(topic=topic, payload="true") + + handler.publisher.publish_str.assert_called_once() + + +class TestCommandErrorEventPayload(unittest.IsolatedAsyncioTestCase): + async def test_error_event_topic_uses_vehicle_prefix(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=MqttGatewayException("some error") + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + error_topic = handler.publisher.publish_json.call_args[0][0] + assert error_topic == f"{VEHICLE_PREFIX}/{mqtt_topics.COMMAND_ERROR}" + + async def test_error_event_payload_structure(self) -> None: + handler = _make_handler() + topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + + cmd_handler = handler._VehicleCommandHandler__command_handlers[ + mqtt_topics.DRIVETRAIN_CHARGING_SET + ] + cmd_handler.handle = AsyncMock( + side_effect=SaicApiException("operation too frequent", return_code=8) + ) + + await handler.handle_mqtt_command(topic=topic, payload="true") + + event_payload = handler.publisher.publish_json.call_args[0][1] + assert set(event_payload.keys()) == {"event_type", "command", "detail"} + assert event_payload["event_type"] == "command_error" + assert event_payload["command"] == mqtt_topics.DRIVETRAIN_CHARGING_SET + assert event_payload["detail"] == "return code: 8, message: operation too frequent" From ba67c4a02c8dbbeb188a114f1c571a6f0c6390a4 Mon Sep 17 00:00:00 2001 From: Giovanni Condello Date: Sat, 14 Mar 2026 09:50:50 +0100 Subject: [PATCH 5/5] fix: rewrite tests to pass mypy strict checks Restructure tests to avoid accessing private name-mangled attributes and asserting on typed Publisher methods. Drive exceptions through saic_api mocks instead of patching internal command handlers. Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/handlers/test_vehicle_command.py | 366 +++++++++++-------------- 1 file changed, 157 insertions(+), 209 deletions(-) diff --git a/tests/handlers/test_vehicle_command.py b/tests/handlers/test_vehicle_command.py index 29da9b20..ecc568fb 100644 --- a/tests/handlers/test_vehicle_command.py +++ b/tests/handlers/test_vehicle_command.py @@ -5,289 +5,237 @@ from saic_ismart_client_ng.exceptions import SaicApiException, SaicLogoutException -from exceptions import MqttGatewayException -from handlers.command.base import RESULT_REFRESH_AND_CLEAR from handlers.vehicle_command import VehicleCommandHandler import mqtt_topics MQTT_TOPIC = "saic" VIN = "vin_test_000000000" VEHICLE_PREFIX = f"vehicles/{VIN}" +CHARGING_SET_TOPIC = f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGING_SET}" +CHARGING_RESULT_TOPIC = ( + f"{VEHICLE_PREFIX}/{mqtt_topics.DRIVETRAIN_CHARGING}/{mqtt_topics.RESULT_SUFFIX}" +) +COMMAND_ERROR_TOPIC = f"{VEHICLE_PREFIX}/{mqtt_topics.COMMAND_ERROR}" -def _make_handler( +def _build( *, - vehicle_state: MagicMock | None = None, saic_api: AsyncMock | None = None, relogin_handler: AsyncMock | None = None, -) -> VehicleCommandHandler: - if vehicle_state is None: - vehicle_state = MagicMock() - vehicle_state.publisher = MagicMock() - vehicle_state.get_topic.side_effect = lambda t: f"{VEHICLE_PREFIX}/{t}" - if saic_api is None: - saic_api = AsyncMock() - if relogin_handler is None: - relogin_handler = AsyncMock() - return VehicleCommandHandler( - vehicle_state=vehicle_state, - saic_api=saic_api, - relogin_handler=relogin_handler, - mqtt_topic=MQTT_TOPIC, - vehicle_prefix=VEHICLE_PREFIX, +) -> tuple[VehicleCommandHandler, MagicMock]: + """Build a VehicleCommandHandler with a MagicMock publisher. + + Returns (handler, mock_publisher) so callers can assert on mock_publisher + without going through the typed Publisher interface. + """ + mock_publisher = MagicMock() + vehicle_state = MagicMock() + vehicle_state.publisher = mock_publisher + vehicle_state.vin = VIN + vehicle_state.get_topic.side_effect = lambda t: f"{VEHICLE_PREFIX}/{t}" + return ( + VehicleCommandHandler( + vehicle_state=vehicle_state, + saic_api=saic_api or AsyncMock(), + relogin_handler=relogin_handler or AsyncMock(), + mqtt_topic=MQTT_TOPIC, + vehicle_prefix=VEHICLE_PREFIX, + ), + mock_publisher, ) -def _command_topic(command_topic_suffix: str) -> str: - return f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/{command_topic_suffix}" - - -def _result_topic(command_topic_suffix: str) -> str: - base = command_topic_suffix.removesuffix(mqtt_topics.SET_SUFFIX).removesuffix("/") - return f"{VEHICLE_PREFIX}/{base}/{mqtt_topics.RESULT_SUFFIX}" - - -class TestVehicleCommandSuccess(unittest.IsolatedAsyncioTestCase): +class TestSuccessPath(unittest.IsolatedAsyncioTestCase): async def test_successful_command_publishes_success(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + handler, pub = _build() - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_str.assert_any_call(result_topic, "Success") - handler.publisher.publish_json.assert_not_called() + pub.publish_str.assert_any_call(CHARGING_RESULT_TOPIC, "Success") + pub.publish_json.assert_not_called() -class TestVehicleCommandNoHandler(unittest.IsolatedAsyncioTestCase): - async def test_no_handler_publishes_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic("nonexistent/topic/set") - result_topic = _result_topic("nonexistent/topic/set") +class TestNoHandlerFound(unittest.IsolatedAsyncioTestCase): + async def test_publishes_error_event(self) -> None: + handler, pub = _build() + bad_topic = f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/nonexistent/topic/set" + result_topic = f"{VEHICLE_PREFIX}/nonexistent/topic/{mqtt_topics.RESULT_SUFFIX}" - await handler.handle_mqtt_command(topic=topic, payload="test") + await handler.handle_mqtt_command(topic=bad_topic, payload="test") - handler.publisher.publish_str.assert_called_once_with( + pub.publish_str.assert_any_call( result_topic, "Failed: No handler found for command topic nonexistent/topic/set", ) - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["event_type"] == "command_error" - assert event_payload["command"] == "nonexistent/topic/set" + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert event["event_type"] == "command_error" + assert event["command"] == "nonexistent/topic/set" - async def test_no_handler_does_not_log_traceback(self) -> None: - handler = _make_handler() - topic = _command_topic("nonexistent/topic/set") + async def test_does_not_log_traceback(self) -> None: + handler, _ = _build() + bad_topic = f"{MQTT_TOPIC}/{VEHICLE_PREFIX}/nonexistent/topic/set" with patch("handlers.vehicle_command.LOG") as mock_log: - await handler.handle_mqtt_command(topic=topic, payload="test") + await handler.handle_mqtt_command(topic=bad_topic, payload="test") mock_log.error.assert_called_once() mock_log.exception.assert_not_called() -class TestVehicleCommandMqttGatewayException(unittest.IsolatedAsyncioTestCase): - async def test_mqtt_gateway_exception_publishes_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) +class TestMqttGatewayException(unittest.IsolatedAsyncioTestCase): + async def test_publishes_error_event(self) -> None: + """An invalid payload triggers MqttGatewayException from payload conversion.""" + handler, pub = _build() - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=MqttGatewayException("test error") + await handler.handle_mqtt_command( + topic=CHARGING_SET_TOPIC, payload="not_a_boolean" ) - await handler.handle_mqtt_command(topic=topic, payload="true") - - handler.publisher.publish_str.assert_called_once_with( - result_topic, "Failed: test error" + pub.publish_str.assert_any_call( + CHARGING_RESULT_TOPIC, + "Failed: Unsupported payload not_a_boolean for command " + "DrivetrainChargingCommand", ) - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["event_type"] == "command_error" - assert event_payload["detail"] == "test error" - + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert event["event_type"] == "command_error" + assert "Unsupported payload" in event["detail"] -class TestVehicleCommandSaicApiException(unittest.IsolatedAsyncioTestCase): - async def test_saic_api_exception_publishes_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=SaicApiException("api error", return_code=8) +class TestSaicApiException(unittest.IsolatedAsyncioTestCase): + async def test_publishes_error_event(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicApiException( + "operation too frequent", return_code=8 ) + handler, pub = _build(saic_api=saic_api) - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_str.assert_called_once_with( - result_topic, "Failed: return code: 8, message: api error" + pub.publish_str.assert_any_call( + CHARGING_RESULT_TOPIC, + "Failed: return code: 8, message: operation too frequent", ) - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["detail"] == "return code: 8, message: api error" + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert event["event_type"] == "command_error" + assert "operation too frequent" in event["detail"] -class TestVehicleCommandUnexpectedException(unittest.IsolatedAsyncioTestCase): - async def test_unexpected_exception_uses_safe_detail(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=RuntimeError("secret internal detail") - ) +class TestUnexpectedException(unittest.IsolatedAsyncioTestCase): + async def test_uses_safe_detail(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = RuntimeError("secret internal detail") + handler, pub = _build(saic_api=saic_api) - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_str.assert_called_once_with( - result_topic, "Failed: unexpected error" + pub.publish_str.assert_any_call( + CHARGING_RESULT_TOPIC, "Failed: unexpected error" ) - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["detail"] == "unexpected error" - assert "secret" not in event_payload["detail"] - + event = pub.publish_json.call_args[0][1] + assert event["detail"] == "unexpected error" + assert "secret" not in event["detail"] -class TestVehicleCommandSaicLogoutException(unittest.IsolatedAsyncioTestCase): - async def test_logout_relogin_success_retries_command(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET +class TestSaicLogoutException(unittest.IsolatedAsyncioTestCase): + async def test_relogin_success_retries_command(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = [ + SaicLogoutException("logged out"), + None, ] - cmd_handler.handle = AsyncMock( - side_effect=[SaicLogoutException("logged out"), RESULT_REFRESH_AND_CLEAR] - ) - - await handler.handle_mqtt_command(topic=topic, payload="true") + handler, pub = _build(saic_api=saic_api) - handler.relogin_handler.force_login.assert_awaited_once() - assert cmd_handler.handle.await_count == 2 - handler.publisher.publish_str.assert_any_call(result_topic, "Success") - handler.publisher.publish_json.assert_not_called() + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - async def test_logout_relogin_failure_publishes_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + relogin = handler.relogin_handler + assert isinstance(relogin, AsyncMock) + relogin.force_login.assert_awaited_once() + assert saic_api.control_charging.await_count == 2 + pub.publish_str.assert_any_call(CHARGING_RESULT_TOPIC, "Success") + pub.publish_json.assert_not_called() - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock(side_effect=SaicLogoutException("logged out")) - handler.relogin_handler.force_login = AsyncMock( - side_effect=Exception("login failed") - ) + async def test_relogin_failure_publishes_error_event(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicLogoutException("logged out") + relogin = AsyncMock() + relogin.force_login.side_effect = Exception("login failed") + handler, pub = _build(saic_api=saic_api, relogin_handler=relogin) - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_str.assert_called_once_with( - result_topic, "Failed: relogin failed (login failed)" + pub.publish_str.assert_any_call( + CHARGING_RESULT_TOPIC, "Failed: relogin failed (login failed)" ) - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert "relogin failed" in event_payload["detail"] - - async def test_logout_retry_failure_publishes_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - result_topic = _result_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert "relogin failed" in event["detail"] - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET + async def test_retry_failure_publishes_error_event(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = [ + SaicLogoutException("logged out"), + RuntimeError("retry boom"), ] - cmd_handler.handle = AsyncMock( - side_effect=[SaicLogoutException("logged out"), RuntimeError("retry boom")] - ) + handler, pub = _build(saic_api=saic_api) - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_str.assert_called_once_with( - result_topic, "Failed: retry boom" + pub.publish_str.assert_any_call( + CHARGING_RESULT_TOPIC, "Failed: retry boom" ) - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["detail"] == "retry boom" + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert event["detail"] == "retry boom" -class TestReportCommandFailureResilience(unittest.IsolatedAsyncioTestCase): +class TestReportFailureResilience(unittest.IsolatedAsyncioTestCase): async def test_publish_str_failure_does_not_prevent_error_event(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=MqttGatewayException("cmd error") - ) - handler.publisher.publish_str.side_effect = ConnectionError("broker down") + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicApiException("err", return_code=1) + handler, pub = _build(saic_api=saic_api) + pub.publish_str.side_effect = ConnectionError("broker down") - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - handler.publisher.publish_json.assert_called_once() - event_payload = handler.publisher.publish_json.call_args[0][1] - assert event_payload["detail"] == "cmd error" + pub.publish_json.assert_called_once() + event = pub.publish_json.call_args[0][1] + assert event["event_type"] == "command_error" async def test_publish_json_failure_does_not_raise(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=MqttGatewayException("cmd error") - ) - handler.publisher.publish_json.side_effect = ConnectionError("broker down") - - await handler.handle_mqtt_command(topic=topic, payload="true") + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicApiException("err", return_code=1) + handler, pub = _build(saic_api=saic_api) + pub.publish_json.side_effect = ConnectionError("broker down") - handler.publisher.publish_str.assert_called_once() + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") + pub.publish_str.assert_called_once() -class TestCommandErrorEventPayload(unittest.IsolatedAsyncioTestCase): - async def test_error_event_topic_uses_vehicle_prefix(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=MqttGatewayException("some error") - ) - - await handler.handle_mqtt_command(topic=topic, payload="true") +class TestErrorEventPayload(unittest.IsolatedAsyncioTestCase): + async def test_topic_uses_vehicle_prefix(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicApiException("err", return_code=1) + handler, pub = _build(saic_api=saic_api) - error_topic = handler.publisher.publish_json.call_args[0][0] - assert error_topic == f"{VEHICLE_PREFIX}/{mqtt_topics.COMMAND_ERROR}" + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - async def test_error_event_payload_structure(self) -> None: - handler = _make_handler() - topic = _command_topic(mqtt_topics.DRIVETRAIN_CHARGING_SET) + error_topic = pub.publish_json.call_args[0][0] + assert error_topic == COMMAND_ERROR_TOPIC - cmd_handler = handler._VehicleCommandHandler__command_handlers[ - mqtt_topics.DRIVETRAIN_CHARGING_SET - ] - cmd_handler.handle = AsyncMock( - side_effect=SaicApiException("operation too frequent", return_code=8) + async def test_payload_structure(self) -> None: + saic_api = AsyncMock() + saic_api.control_charging.side_effect = SaicApiException( + "operation too frequent", return_code=8 ) + handler, pub = _build(saic_api=saic_api) - await handler.handle_mqtt_command(topic=topic, payload="true") + await handler.handle_mqtt_command(topic=CHARGING_SET_TOPIC, payload="true") - event_payload = handler.publisher.publish_json.call_args[0][1] - assert set(event_payload.keys()) == {"event_type", "command", "detail"} - assert event_payload["event_type"] == "command_error" - assert event_payload["command"] == mqtt_topics.DRIVETRAIN_CHARGING_SET - assert event_payload["detail"] == "return code: 8, message: operation too frequent" + event = pub.publish_json.call_args[0][1] + assert set(event.keys()) == {"event_type", "command", "detail"} + assert event["event_type"] == "command_error" + assert event["command"] == mqtt_topics.DRIVETRAIN_CHARGING_SET + assert "operation too frequent" in event["detail"]