From 19073cbc043b9694d1692b207920f688107fe1f1 Mon Sep 17 00:00:00 2001 From: Keith Ralphs Date: Mon, 27 Jan 2025 11:12:50 +0000 Subject: [PATCH 1/5] Handle blank on 'None' exception messages --- src/blueapi/service/runner.py | 22 +++++++++--- tests/unit_tests/service/test_runner.py | 45 ++++++++++++++++++------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/blueapi/service/runner.py b/src/blueapi/service/runner.py index 734d24bf8..695c7c80e 100644 --- a/src/blueapi/service/runner.py +++ b/src/blueapi/service/runner.py @@ -30,6 +30,8 @@ P = ParamSpec("P") T = TypeVar("T") +BLANK_REPORT = "The source message was blank" + def _init_worker(): # Replace sigint to allow subprocess to be terminated @@ -80,12 +82,12 @@ def start(self): initialized=True, ) except Exception as e: + LOGGER.exception(e) self._state = EnvironmentResponse( environment_id=environment_id, initialized=False, - error_message=str(e), + error_message=_safe_exception_message(e), ) - LOGGER.exception(e) @start_as_current_span(TRACER) def stop(self): @@ -98,15 +100,17 @@ def stop(self): self._state = EnvironmentResponse( environment_id=environment_id, initialized=False, - error_message=self._state.error_message, + error_message=_safe_message( + type(self).__name__, self._state.error_message + ), ) except Exception as e: + LOGGER.exception(e) self._state = EnvironmentResponse( environment_id=environment_id, initialized=False, - error_message=str(e), + error_message=_safe_exception_message(e), ) - LOGGER.exception(e) @start_as_current_span(TRACER, "function", "args", "kwargs") def run( @@ -188,3 +192,11 @@ def _validate_function(func: Any, function_name: str) -> Callable: elif not callable(func): raise RpcError(f"{function_name}: Object in subprocess is not a function") return func + + +def _safe_exception_message(e: Exception) -> str: + return _safe_message(type(e).__name__, str(e)) + + +def _safe_message(owner: str, message: str | None) -> str: + return f"{owner}: {BLANK_REPORT if (not message or message.isspace()) else message}" diff --git a/tests/unit_tests/service/test_runner.py b/tests/unit_tests/service/test_runner.py index 0cdfe477a..9eef9894d 100644 --- a/tests/unit_tests/service/test_runner.py +++ b/tests/unit_tests/service/test_runner.py @@ -17,6 +17,7 @@ InvalidRunnerStateError, RpcError, WorkerDispatcher, + _safe_exception_message, import_and_run_function, ) @@ -54,7 +55,9 @@ def test_initialize(runner: WorkerDispatcher, mock_subprocess: Mock): assert runner.run(interface.get_worker_state) == 123 runner.stop() - assert runner.state.error_message is None + assert ( + runner.state.error_message == "WorkerDispatcher: The source message was blank" + ) assert not runner.state.initialized @@ -71,19 +74,33 @@ def test_raises_if_used_before_started(runner: WorkerDispatcher): runner.run(interface.get_plans) -def test_error_on_runner_setup(runner: WorkerDispatcher, mock_subprocess: Mock): - error_message = "Intentional start_worker exception" - environment_id = uuid.uuid4() - expected_state = EnvironmentResponse( - environment_id=environment_id, - initialized=False, - error_message=error_message, +@pytest.mark.parametrize( + "message", + [ + None, + "", + " ", + "Intentional start_worker exception", + ], +) +def test_using_safe_exception_message_copes_with_all_message_types_on_runner_setup( + runner: WorkerDispatcher, mock_subprocess: Mock, message: str | None +): + try: + raise Exception() if message is None else Exception(message) + except Exception as e: + expected_state = EnvironmentResponse( + environment_id=uuid.uuid4(), + initialized=False, + error_message=_safe_exception_message(e), + ) + mock_subprocess.apply.side_effect = ( + Exception() if message is None else Exception(message) ) - mock_subprocess.apply.side_effect = Exception(error_message) - # Calling reload here instead of start also indirectly - # tests that stop() doesn't raise if there is no error message - # and the runner is not yet initialised + # Calling reload here instead of start also indirectly tests + # that stop() doesn't raise if there is no error message and the + # runner is not yet initialised. runner.reload() state = runner.state expected_state.environment_id = state.environment_id @@ -116,7 +133,9 @@ def test_can_reload_after_an_error(pool_mock: MagicMock): runner.start() current_env = runner.state.environment_id assert runner.state == EnvironmentResponse( - environment_id=current_env, initialized=False, error_message="invalid code" + environment_id=current_env, + initialized=False, + error_message="SyntaxError: invalid code", ) runner.reload() From ff945b4564f08dab45abbedc6e1ac4eda1e2ae12 Mon Sep 17 00:00:00 2001 From: Keith Ralphs Date: Mon, 27 Jan 2025 11:23:10 +0000 Subject: [PATCH 2/5] Use constant in test --- tests/unit_tests/service/test_runner.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/service/test_runner.py b/tests/unit_tests/service/test_runner.py index 9eef9894d..b5b8ff626 100644 --- a/tests/unit_tests/service/test_runner.py +++ b/tests/unit_tests/service/test_runner.py @@ -14,6 +14,7 @@ from blueapi.service import interface from blueapi.service.model import EnvironmentResponse from blueapi.service.runner import ( + BLANK_REPORT, InvalidRunnerStateError, RpcError, WorkerDispatcher, @@ -55,9 +56,7 @@ def test_initialize(runner: WorkerDispatcher, mock_subprocess: Mock): assert runner.run(interface.get_worker_state) == 123 runner.stop() - assert ( - runner.state.error_message == "WorkerDispatcher: The source message was blank" - ) + assert runner.state.error_message == f"{type(runner).__name__}: {BLANK_REPORT}" assert not runner.state.initialized From 6e23de94d4ba6da65b4c4494ba6b4d5a90abc43c Mon Sep 17 00:00:00 2001 From: Keith Ralphs Date: Mon, 27 Jan 2025 16:10:35 +0000 Subject: [PATCH 3/5] Cope with blank error message in _wait_for_reload --- src/blueapi/client/client.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index 1dbcb8039..1b52236ea 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -22,6 +22,7 @@ TasksListResponse, WorkerTask, ) +from blueapi.service.runner import BLANK_REPORT from blueapi.worker import Task, TrackableTask, WorkerEvent, WorkerState from blueapi.worker.event import ProgressEvent, TaskStatus @@ -412,7 +413,10 @@ def _wait_for_reload( while too_late is None or time.time() < too_late: # Poll until the environment is restarted or the timeout is reached status = self._rest.get_environment() - if status.error_message is not None: + if ( + status.error_message is not None + and BLANK_REPORT not in status.error_message + ): raise BlueskyRemoteControlError( f"Error reloading environment: {status.error_message}" ) From 031e3cfd78ea97e00da5d44af34ac6c70c905361 Mon Sep 17 00:00:00 2001 From: Keith Ralphs Date: Tue, 28 Jan 2025 11:48:58 +0000 Subject: [PATCH 4/5] Remove error interception in stop --- src/blueapi/client/client.py | 6 +----- src/blueapi/service/runner.py | 4 +--- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index 1b52236ea..1dbcb8039 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -22,7 +22,6 @@ TasksListResponse, WorkerTask, ) -from blueapi.service.runner import BLANK_REPORT from blueapi.worker import Task, TrackableTask, WorkerEvent, WorkerState from blueapi.worker.event import ProgressEvent, TaskStatus @@ -413,10 +412,7 @@ def _wait_for_reload( while too_late is None or time.time() < too_late: # Poll until the environment is restarted or the timeout is reached status = self._rest.get_environment() - if ( - status.error_message is not None - and BLANK_REPORT not in status.error_message - ): + if status.error_message is not None: raise BlueskyRemoteControlError( f"Error reloading environment: {status.error_message}" ) diff --git a/src/blueapi/service/runner.py b/src/blueapi/service/runner.py index 695c7c80e..2b5a5f37f 100644 --- a/src/blueapi/service/runner.py +++ b/src/blueapi/service/runner.py @@ -100,9 +100,7 @@ def stop(self): self._state = EnvironmentResponse( environment_id=environment_id, initialized=False, - error_message=_safe_message( - type(self).__name__, self._state.error_message - ), + error_message=self._state.error_message, ) except Exception as e: LOGGER.exception(e) From 262b6427137481aa70dbadf20862679dea5ae309 Mon Sep 17 00:00:00 2001 From: Keith Ralphs Date: Tue, 28 Jan 2025 12:56:40 +0000 Subject: [PATCH 5/5] revert unit test change --- tests/unit_tests/service/test_runner.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/unit_tests/service/test_runner.py b/tests/unit_tests/service/test_runner.py index b5b8ff626..f96fa3826 100644 --- a/tests/unit_tests/service/test_runner.py +++ b/tests/unit_tests/service/test_runner.py @@ -14,7 +14,6 @@ from blueapi.service import interface from blueapi.service.model import EnvironmentResponse from blueapi.service.runner import ( - BLANK_REPORT, InvalidRunnerStateError, RpcError, WorkerDispatcher, @@ -56,7 +55,7 @@ def test_initialize(runner: WorkerDispatcher, mock_subprocess: Mock): assert runner.run(interface.get_worker_state) == 123 runner.stop() - assert runner.state.error_message == f"{type(runner).__name__}: {BLANK_REPORT}" + assert runner.state.error_message is None assert not runner.state.initialized