From aa2eb1a823c08751b0deba072f39d1b4285d52f6 Mon Sep 17 00:00:00 2001 From: "Tom C (DLS)" <101418278+coretl@users.noreply.github.com> Date: Mon, 5 Aug 2024 10:21:59 +0100 Subject: [PATCH] Improve the error messages by including command sent (#94) --- src/pandablocks/_exchange.py | 17 ++++++++++++++--- src/pandablocks/commands.py | 18 ++++++----------- src/pandablocks/connections.py | 11 +---------- tests/test_asyncio.py | 5 ++++- tests/test_blocking.py | 5 ++++- tests/test_pandablocks.py | 35 ++++++++++++++++++++++------------ 6 files changed, 52 insertions(+), 39 deletions(-) diff --git a/src/pandablocks/_exchange.py b/src/pandablocks/_exchange.py index f41f24470..9eebc95bc 100644 --- a/src/pandablocks/_exchange.py +++ b/src/pandablocks/_exchange.py @@ -16,16 +16,27 @@ def __init__(self, to_send: Union[str, list[str]]): self.received: list[str] = [] self.is_multiline = False + def _error_message(self) -> str: + sent = "\n".join(self.to_send) + received = "\n".join(self.received) + return f"{sent!r} -> {received!r}" + + def check_ok(self): + assert self.received == ["OK"], self._error_message() + @property def line(self) -> str: """Check received is not multiline and return the line""" - assert not self.is_multiline - return self.received[0] + assert not self.is_multiline and self.received[0].startswith( + "OK =" + ), self._error_message() + # Remove the OK= header + return self.received[0][4:] @property def multiline(self) -> list[str]: """Return the multiline received lines, processed to remove markup""" - assert self.is_multiline + assert self.is_multiline, self._error_message() # Remove the ! and . markup return [line[1:] for line in self.received[:-1]] diff --git a/src/pandablocks/commands.py b/src/pandablocks/commands.py index 0d1561f77..b94a0f6a0 100644 --- a/src/pandablocks/commands.py +++ b/src/pandablocks/commands.py @@ -207,10 +207,7 @@ def execute(self) -> ExchangeGenerator[Union[str, list[str]]]: if ex.is_multiline: return ex.multiline else: - # We got OK =value - line = ex.line - assert line.startswith("OK =") - return line[4:] + return ex.line @dataclass @@ -232,10 +229,7 @@ class GetLine(Command[str]): def execute(self) -> ExchangeGenerator[str]: ex = Exchange(f"{self.field}?") yield ex - # Expect "OK =value" - line = ex.line - assert line.startswith("OK =") - return line[4:] + return ex.line @dataclass @@ -285,7 +279,7 @@ def execute(self) -> ExchangeGenerator[None]: else: ex = Exchange(f"{self.field}={self.value}") yield ex - assert ex.line == "OK" + ex.check_ok() @dataclass @@ -308,7 +302,7 @@ def execute(self) -> ExchangeGenerator[None]: # Multiline table with blank line to terminate ex = Exchange([f"{self.field}<<"] + self.value + [""]) yield ex - assert ex.line == "OK" + ex.check_ok() class Arm(Command[None]): @@ -317,7 +311,7 @@ class Arm(Command[None]): def execute(self) -> ExchangeGenerator[None]: ex = Exchange("*PCAP.ARM=") yield ex - assert ex.line == "OK" + ex.check_ok() class Disarm(Command[None]): @@ -326,7 +320,7 @@ class Disarm(Command[None]): def execute(self) -> ExchangeGenerator[None]: ex = Exchange("*PCAP.DISARM=") yield ex - assert ex.line == "OK" + ex.check_ok() @dataclass diff --git a/src/pandablocks/connections.py b/src/pandablocks/connections.py index 75eeb17af..fd77001e3 100644 --- a/src/pandablocks/connections.py +++ b/src/pandablocks/connections.py @@ -120,16 +120,7 @@ class _ExchangeContext: generator: Optional[ExchangeGenerator[Any]] = None def exception(self, e: Exception) -> CommandException: - """Return a `CommandException` with the sent and received strings - in the text""" - msg = f"{self.command} ->" - if self.exchange.is_multiline: - for line in self.exchange.multiline: - msg += "\n " + line - else: - msg += " " + self.exchange.line - if e.args: - msg += f"\n{type(e).__name__}:{e}" + msg = f"{self.command} raised error:\n{type(e).__name__}: {e}" return CommandException(msg).with_traceback(e.__traceback__) diff --git a/tests/test_asyncio.py b/tests/test_asyncio.py index 99a66b92d..0f44eca10 100644 --- a/tests/test_asyncio.py +++ b/tests/test_asyncio.py @@ -27,7 +27,10 @@ async def test_asyncio_bad_put_raises(dummy_server_async): async with AsyncioClient("localhost") as client: with pytest.raises(CommandException) as cm: await asyncio.wait_for(client.send(Put("PCAP.thing", 1)), timeout=1) - assert str(cm.value) == "Put(field='PCAP.thing', value=1) -> ERR no such field" + assert ( + str(cm.value) == "Put(field='PCAP.thing', value=1) raised error:\n" + "AssertionError: 'PCAP.thing=1' -> 'ERR no such field'" + ) assert dummy_server_async.received == ["PCAP.thing=1"] diff --git a/tests/test_blocking.py b/tests/test_blocking.py index 0cb91dfa2..dced49f6b 100644 --- a/tests/test_blocking.py +++ b/tests/test_blocking.py @@ -17,7 +17,10 @@ def test_blocking_bad_put_raises(dummy_server_in_thread): with BlockingClient("localhost") as client: with pytest.raises(CommandException) as cm: client.send(Put("PCAP.thing", 1), timeout=1) - assert str(cm.value) == "Put(field='PCAP.thing', value=1) -> ERR no such field" + assert ( + str(cm.value) == "Put(field='PCAP.thing', value=1) raised error:\n" + "AssertionError: 'PCAP.thing=1' -> 'ERR no such field'" + ) assert dummy_server_in_thread.received == ["PCAP.thing=1"] diff --git a/tests/test_pandablocks.py b/tests/test_pandablocks.py index c06565dd0..a909b8a8c 100644 --- a/tests/test_pandablocks.py +++ b/tests/test_pandablocks.py @@ -83,7 +83,10 @@ def test_get_line_error_when_multiline(): assert get_responses(conn, b"!ACTIVE 5 bit_out\n.\n") == [ ( cmd, - ACommandException("GetLine(field='PCAP.ACTIVE') ->\n ACTIVE 5 bit_out"), + ACommandException( + "GetLine(field='PCAP.ACTIVE') raised error:\n" + "AssertionError: 'PCAP.ACTIVE?' -> '!ACTIVE 5 bit_out\\n.'" + ), ) ] @@ -95,7 +98,10 @@ def test_get_line_error_no_ok(): assert get_responses(conn, b"NOT OK\n") == [ ( cmd, - ACommandException("GetLine(field='PCAP.ACTIVE') -> NOT OK"), + ACommandException( + "GetLine(field='PCAP.ACTIVE') raised error:\n" + "AssertionError: 'PCAP.ACTIVE?' -> 'NOT OK'" + ), ) ] @@ -117,7 +123,10 @@ def test_get_multiline_error_when_single_line(): assert get_responses(conn, b"1\n") == [ ( cmd, - ACommandException("GetMultiline(field='PCAP.*') -> 1"), + ACommandException( + "GetMultiline(field='PCAP.*') raised error:\n" + "AssertionError: 'PCAP.*?' -> '1'" + ), ) ] @@ -143,7 +152,8 @@ def test_put_fails_with_single_line_exception(): ( cmd, ACommandException( - "Put(field='PCAP.blah', value='something') -> ERR No such field" + "Put(field='PCAP.blah', value='something') raised error:\n" + "AssertionError: 'PCAP.blah=something' -> 'ERR No such field'" ), ) ] @@ -157,11 +167,9 @@ def test_put_fails_with_multiline_exception(): ( cmd, ACommandException( - """\ -Put(field='PCAP.blah', value='something') -> - This is bad - Very bad - Don't do this""" + "Put(field='PCAP.blah', value='something') raised error:\n" + "AssertionError: 'PCAP.blah=something' -> " + '"!This is bad\\n!Very bad\\n!Don\'t do this\\n."' ), ) ] @@ -267,7 +275,8 @@ def test_get_block_info_error(): ( cmd, ACommandException( - "GetBlockInfo(skip_description=False) -> ERR Cannot read blocks" + "GetBlockInfo(skip_description=False) raised error:\n" + "AssertionError: '*BLOCKS?' -> 'ERR Cannot read blocks'" ), ) ] @@ -291,7 +300,8 @@ def test_get_block_info_desc_err(): ( cmd, ACommandException( - "GetBlockInfo(skip_description=False) -> ERR could not get description" + "GetBlockInfo(skip_description=False) raised error:\n" + "AssertionError: '*DESC.PCAP?' -> 'ERR could not get description'" ), ) ] @@ -416,7 +426,8 @@ def test_get_fields_non_existant_block(): ( cmd, ACommandException( - "GetFieldInfo(block='FOO', extended_metadata=True) -> ERR No such block" + "GetFieldInfo(block='FOO', extended_metadata=True) raised error:\n" + "AssertionError: 'FOO.*?' -> 'ERR No such block'" ), ) ]