Improve BatteryManager error reporting

A `KeyError` was raised to indicate errors while getting inverters for
batteries, but this have a couple of issues.

First, the `KeyError` was converted to a string to get an error message,
but `KeyError` is a very special error, and the argument is interpreted
as a *key*, not as an error message, so when converting the error to a
`str`, the result is the `repr()` of the *key*, so the strings are
quoted and escaped, which is not what we want to report errors
downstream.

Second, one of the reported errors isn't really a `KeyError`, it is just
a mismatch between what is requested and the current microgrid topology,
so using a `KeyError` was completely wrong.

Since these errors were only used to report errors from a direct call,
we change `_get_components_data()` to return `list[InvBatPair] | str`,
where if there is an error, a `str` with the error description is
returned directly.

As an extra, we sort the IDs when used in error messages, so they are
easier to read for users, and more deterministic for tests.

Tests are also simplified to remove the use of regex, which were
probably only used because string were quoted.

Signed-off-by: Leandro Lucarella <[email protected]>

Author: llucax

src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py

@@ -224,17 +224,18 @@ async def _get_distribution(self, request: Request) -> DistributionResult | Resu
         Returns:
             Distribution of the batteries.
         """
-        try:
-            pairs_data: list[InvBatPair] = self._get_components_data(
-                request.component_ids
-            )
-        except KeyError as err:
-            return Error(request=request, msg=str(err))
+        match self._get_components_data(request.component_ids):
+            case str() as err:
+                return Error(request=request, msg=err)
+            case list() as pairs_data:
+                pass
+            case unexpected:
+                typing.assert_never(unexpected)
 
         if not pairs_data:
             error_msg = (
-                "No data for at least one of the given "
-                f"batteries {str(request.component_ids)}"
+                "No data for at least one of the given batteries: "
+                + self._str_ids(request.component_ids)
             )
             return Error(request=request, msg=str(error_msg))
 
@@ -510,18 +511,17 @@ def nan_metric_in_list(data: list[DataType], metrics: list[str]) -> bool:
 
     def _get_components_data(
         self, batteries: collections.abc.Set[int]
-    ) -> list[InvBatPair]:
+    ) -> list[InvBatPair] | str:
         """Get data for the given batteries and adjacent inverters.
 
         Args:
             batteries: Batteries that needs data.
 
-        Raises:
-            KeyError: If any battery in the given list doesn't exists in microgrid.
-
         Returns:
-            Pairs of battery and adjacent inverter data.
+            Pairs of battery and adjacent inverter data or an error message if there was
+                an error while getting the data.
         """
+        inverter_ids: collections.abc.Set[int]
         pairs_data: list[InvBatPair] = []
 
         working_batteries = self._component_pool_status_tracker.get_working_components(
@@ -530,9 +530,9 @@ def _get_components_data(
 
         for battery_id in working_batteries:
             if battery_id not in self._battery_caches:
-                raise KeyError(
+                return (
                     f"No battery {battery_id}, "
-                    f"available batteries: {list(self._battery_caches.keys())}"
+                    f"available batteries: {self._str_ids(self._battery_caches.keys())}"
                 )
 
         connected_inverters = _get_all_from_map(self._bat_invs_map, batteries)
@@ -545,9 +545,10 @@ def _get_components_data(
 
         if batteries_from_inverters != batteries:
             extra_batteries = batteries_from_inverters - batteries
-            raise KeyError(
-                f"Inverters {_get_all_from_map(self._bat_invs_map, extra_batteries)} "
-                f"are connected to batteries that were not requested: {extra_batteries}"
+            inverter_ids = _get_all_from_map(self._bat_invs_map, extra_batteries)
+            return (
+                f"Inverter(s) ({self._str_ids(inverter_ids)}) are connected to "
+                f"battery(ies) ({self._str_ids(extra_batteries)}) that were not requested"
             )
 
         # set of set of batteries one for each working_battery
@@ -556,7 +557,7 @@ def _get_components_data(
         )
 
         for battery_ids in battery_sets:
-            inverter_ids: frozenset[int] = self._bat_invs_map[next(iter(battery_ids))]
+            inverter_ids = self._bat_invs_map[next(iter(battery_ids))]
 
             data = self._get_battery_inverter_data(battery_ids, inverter_ids)
             if data is None:
@@ -570,6 +571,9 @@ def _get_components_data(
             pairs_data.append(data)
         return pairs_data
 
+    def _str_ids(self, ids: collections.abc.Set[int]) -> str:
+        return ", ".join(str(cid) for cid in sorted(ids))
+
     def _get_power_distribution(
         self, request: Request, inv_bat_pairs: list[InvBatPair]
     ) -> DistributionResult:

tests/microgrid/power_distributing/test_power_distributing.py

@@ -7,7 +7,6 @@
 
 import asyncio
 import math
-import re
 from collections import abc
 from typing import TypeVar
 from unittest.mock import MagicMock
@@ -474,7 +473,7 @@ async def test_two_batteries_one_broken_one_inverters(
                 assert result.request == request
                 assert (
                     result.msg
-                    == "No data for at least one of the given batteries {9, 19}"
+                    == "No data for at least one of the given batteries: 9, 19"
                 )
 
     async def test_battery_two_inverters(self, mocker: MockerFixture) -> None:
@@ -821,13 +820,10 @@ async def test_connected_but_not_requested_batteries(
                 result: Result = done.pop().result()
                 assert isinstance(result, Error)
                 assert result.request == request
-
-                err_msg = re.search(
-                    r"'Inverters \{48\} are connected to batteries that were not "
-                    r"requested: \{19\}'",
-                    result.msg,
+                assert (
+                    result.msg
+                    == "Inverter(s) (48) are connected to battery(ies) (19) that were not requested"
                 )
-                assert err_msg is not None
 
     async def test_battery_soc_nan(self, mocker: MockerFixture) -> None:
         """Test if battery with SoC==NaN is not used."""
@@ -1054,8 +1050,7 @@ async def test_power_distributor_invalid_battery_id(
             result: Result = done.pop().result()
             assert isinstance(result, Error)
             assert result.request == request
-            err_msg = re.search(r"No battery 100, available batteries:", result.msg)
-            assert err_msg is not None
+            assert result.msg == "No battery 100, available batteries: 9, 19, 29"
 
     async def test_power_distributor_one_user_adjust_power_consume(
         self, mocker: MockerFixture