From 30f7c585dfec331c3a833b8fae295110bd18402c Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Wed, 11 Mar 2026 09:30:57 +0000 Subject: [PATCH 01/20] chore: Improve default error handling and enhance exception messages --- src/blueapi/client/rest.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 6caa0add2..cd23f9b46 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -38,21 +38,29 @@ LOGGER = logging.getLogger(__name__) -class UnauthorisedAccessError(Exception): +class NonJsonResponseError(Exception): pass -class BlueskyRemoteControlError(Exception): +class BlueskyRequestError(Exception): + def __init__(self, code: int | None = None, message: str = "") -> None: + super().__init__(code, message) + + +class UnauthorisedAccessError(BlueskyRequestError): pass -class NonJsonResponseError(Exception): +class BlueskyRemoteControlError(BlueskyRequestError): pass -class BlueskyRequestError(Exception): - def __init__(self, code: int, message: str) -> None: - super().__init__(message, code) +class NotFoundError(BlueskyRequestError): + pass + + +class UnknownPlanError(BlueskyRequestError): + pass class NoContentError(Exception): @@ -105,16 +113,14 @@ def from_validation_error(cls, ve: ValidationError): ) -class UnknownPlanError(Exception): - pass - - def _exception(response: requests.Response) -> Exception | None: code = response.status_code if code < 400: return None + elif code in (401, 403): + return UnauthorisedAccessError(code, str(response)) elif code == 404: - return KeyError(str(_response_json(response))) + return NotFoundError(code, str(response)) else: return BlueskyRemoteControlError(code, str(response)) @@ -124,9 +130,9 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: if code < 400: return None elif code == 401 or code == 403: - return UnauthorisedAccessError() + return UnauthorisedAccessError(code, str(response)) elif code == 404: - return UnknownPlanError() + return UnknownPlanError(code, str(response)) elif code == 422: try: content = _response_json(response) @@ -138,9 +144,9 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: except Exception: # If the error can't be parsed into something sensible, return the # raw text in a generic exception so at least it gets reported - return BlueskyRequestError(code, response.text) + return BlueskyRequestError(code, str(response)) else: - return BlueskyRequestError(code, response.text) + return BlueskyRequestError(code, str(response)) def _response_json(response: requests.Response) -> Any: From 65764a8d083726b86897b7e4fdd71ba09f5f674d Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Wed, 11 Mar 2026 09:33:16 +0000 Subject: [PATCH 02/20] fix: Use typed exceptions in cli and client --- src/blueapi/cli/cli.py | 17 +++++++++-------- src/blueapi/client/client.py | 12 ++++++------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 8ec705ae0..e6f8769c1 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -254,13 +254,12 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T: raise ClickException( "Failed to establish connection to blueapi server." ) from ce + except UnauthorisedAccessError as e: + raise ClickException( + "Access denied. Please check your login status and try again." + ) from e except BlueskyRemoteControlError as e: - if str(e) == "": - raise ClickException( - "Access denied. Please check your login status and try again." - ) from e - else: - raise e + raise e return wrapper @@ -398,8 +397,10 @@ def on_event(event: AnyEvent) -> None: raise ClickException("Unauthorised request") from ua except InvalidParametersError as ip: raise ClickException(ip.message()) from ip - except (BlueskyRemoteControlError, BlueskyStreamingError) as e: - raise ClickException(f"server error with this message: {e}") from e + except BlueskyStreamingError as se: + raise ClickException(f"streaming error: {se}") from se + except BlueskyRemoteControlError as e: + raise ClickException(f"remote control error: {e.args[1]}") from e except ValueError as ve: raise ClickException(f"task could not run: {ve}") from ve diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index 32d589089..d188a3c1b 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -42,7 +42,7 @@ from blueapi.worker.task_worker import TrackableTask from .event_bus import AnyEvent, EventBusClient, OnAnyEvent -from .rest import BlueapiRestClient, BlueskyRemoteControlError +from .rest import BlueapiRestClient, BlueskyRemoteControlError, NotFoundError TRACER = get_tracer("client") @@ -99,7 +99,7 @@ def __getitem__(self, name: str) -> "DeviceRef": self._cache[name] = device setattr(self, model.name, device) return device - except KeyError: + except NotFoundError: pass raise AttributeError(f"No device named '{name}' available") @@ -512,7 +512,7 @@ def inner_on_event(event: AnyEvent, ctx: MessageContext) -> None: if event.task_status is None: complete.set_exception( BlueskyRemoteControlError( - "Server completed without task status" + message="Server completed without task status" ) ) else: @@ -544,7 +544,7 @@ def create_and_start_task(self, task: TaskRequest) -> TaskResponse: return response else: raise BlueskyRemoteControlError( - f"Tried to create and start task {response.task_id} " + message=f"Tried to create and start task {response.task_id} " f"but {worker_response.task_id} was started instead" ) @@ -673,7 +673,7 @@ def reload_environment( status = self._rest.delete_environment() except Exception as e: raise BlueskyRemoteControlError( - "Failed to tear down the environment" + message="Failed to tear down the environment" ) from e return self._wait_for_reload( status, @@ -698,7 +698,7 @@ def _wait_for_reload( status = self._rest.get_environment() if status.error_message is not None: raise BlueskyRemoteControlError( - f"Error reloading environment: {status.error_message}" + message=f"Error reloading environment: {status.error_message}" ) elif ( status.initialized and status.environment_id != previous_environment_id From 58d910c1979da8d35de8bb803a5e43faac2086fa Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Wed, 11 Mar 2026 09:34:34 +0000 Subject: [PATCH 03/20] test: Update tests for new exception hierarchy --- tests/system_tests/test_blueapi_system.py | 16 ++++++++------- tests/unit_tests/cli/test_cli.py | 19 ++++++++++-------- tests/unit_tests/client/test_client.py | 21 ++++++++++++++++---- tests/unit_tests/client/test_rest.py | 24 ++++++++++++++++------- 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 4763cebfa..4a5e503f0 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -15,7 +15,9 @@ BlueapiRestClient, BlueskyRemoteControlError, BlueskyRequestError, + NotFoundError, ServiceUnavailableError, + UnauthorisedAccessError, ) from blueapi.config import ( ApplicationConfig, @@ -217,7 +219,7 @@ def test_cannot_access_endpoints( "get_oidc_config" ) # get_oidc_config can be accessed without auth for get_method in blueapi_rest_client_get_methods: - with pytest.raises(BlueskyRemoteControlError, match=r""): + with pytest.raises(UnauthorisedAccessError, match=r""): getattr(client_without_auth._rest, get_method)() @@ -243,7 +245,7 @@ def test_get_plans_by_name(client: BlueapiClient, expected_plans: PlanResponse): def test_get_non_existent_plan(rest_client: BlueapiRestClient): - with pytest.raises(KeyError, match="{'detail': 'Item not found'}"): + with pytest.raises(NotFoundError): rest_client.get_plan("Not exists") @@ -268,12 +270,12 @@ def test_get_device_by_name( def test_get_non_existent_device(rest_client: BlueapiRestClient): - with pytest.raises(KeyError, match="{'detail': 'Item not found'}"): + with pytest.raises(NotFoundError): rest_client.get_device("Not exists") def test_client_non_existent_device(client: BlueapiClient): - with pytest.raises(AttributeError, match="No device named 'missing' available"): + with pytest.raises(AttributeError): _ = client.devices.missing @@ -295,7 +297,7 @@ def test_instrument_session_propagated(rest_client: BlueapiRestClient): def test_create_task_validation_error(rest_client: BlueapiRestClient): - with pytest.raises(BlueskyRequestError, match="Internal Server Error"): + with pytest.raises(BlueskyRequestError): rest_client.create_task( TaskRequest( name="Not-exists", @@ -336,12 +338,12 @@ def test_get_task_by_id(rest_client: BlueapiRestClient): def test_get_non_existent_task(rest_client: BlueapiRestClient): - with pytest.raises(KeyError, match="{'detail': 'Item not found'}"): + with pytest.raises(NotFoundError): rest_client.get_task("Not-exists") def test_delete_non_existent_task(rest_client: BlueapiRestClient): - with pytest.raises(KeyError, match="{'detail': 'Item not found'}"): + with pytest.raises(NotFoundError): rest_client.clear_task("Not-exists") diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 49e8b3e61..9e0837dcc 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -118,9 +118,8 @@ def test_connection_error_caught_by_wrapper_func( def test_authentication_error_caught_by_wrapper_func( mock_requests: Mock, runner: CliRunner ): - mock_requests.side_effect = BlueskyRemoteControlError("") + mock_requests.side_effect = UnauthorisedAccessError(message="") result = runner.invoke(main, ["controller", "plans"]) - assert ( result.output == "Error: Access denied. Please check your login status and try again.\n" @@ -129,12 +128,11 @@ def test_authentication_error_caught_by_wrapper_func( @patch("blueapi.client.rest.requests.Session.request") def test_remote_error_raised_by_wrapper_func(mock_requests: Mock, runner: CliRunner): - mock_requests.side_effect = BlueskyRemoteControlError("Response [450]") - + mock_requests.side_effect = BlueskyRemoteControlError(message="Response [450]") result = runner.invoke(main, ["controller", "plans"]) assert ( isinstance(result.exception, BlueskyRemoteControlError) - and result.exception.args == ("Response [450]",) + and result.exception.args[1] == "Response [450]" and result.exit_code == 1 ) @@ -680,7 +678,7 @@ def test_env_reload_server_side_error(runner: CliRunner): assert isinstance(result.exception, BlueskyRemoteControlError), ( "Expected a BlueskyRemoteError from cli runner" ) - assert result.exception.args[0] == "Failed to tear down the environment" + assert result.exception.args[1] == "Failed to tear down the environment" # Check if the endpoints were hit as expected assert len(responses.calls) == 1 # +1 for the DELETE call @@ -716,13 +714,17 @@ def test_env_reload_server_side_error(runner: CliRunner): "Error: Incorrect parameters supplied\n Missing value for 'foo'\n", ), ( - BlueskyRemoteControlError("Server error"), - "Error: server error with this message: Server error\n", + BlueskyRemoteControlError(message="Server error"), + "Error: remote control error: Server error\n", ), ( ValueError("Error parsing parameters"), "Error: task could not run: Error parsing parameters\n", ), + ( + BlueskyStreamingError("streaming failed"), + "Error: streaming error: streaming failed\n", + ), ], ids=[ "unknown_plan", @@ -730,6 +732,7 @@ def test_env_reload_server_side_error(runner: CliRunner): "invalid_parameters", "remote_control", "value_error", + "streaming_error", ], ) def test_error_handling(exception, error_message, runner: CliRunner): diff --git a/tests/unit_tests/client/test_client.py b/tests/unit_tests/client/test_client.py index a41c22bb1..f9d2eadcd 100644 --- a/tests/unit_tests/client/test_client.py +++ b/tests/unit_tests/client/test_client.py @@ -20,7 +20,11 @@ PlanFailedError, ) from blueapi.client.event_bus import AnyEvent, EventBusClient -from blueapi.client.rest import BlueapiRestClient, BlueskyRemoteControlError +from blueapi.client.rest import ( + BlueapiRestClient, + BlueskyRemoteControlError, + NotFoundError, +) from blueapi.config import MissingStompConfigurationError from blueapi.core import DataEvent from blueapi.service.model import ( @@ -99,7 +103,14 @@ def mock_rest() -> BlueapiRestClient: mock.get_plans.return_value = PLANS mock.get_plan.side_effect = lambda n: {p.name: p for p in PLANS.plans}[n] mock.get_devices.return_value = DEVICES - mock.get_device.side_effect = lambda n: {d.name: d for d in DEVICES.devices}[n] + device_map = {d.name: d for d in DEVICES.devices} + + def get_device(n: str): + if n not in device_map: + raise NotFoundError(404, "") + return device_map[n] + + mock.get_device.side_effect = get_device mock.get_state.return_value = WorkerState.IDLE mock.get_task.return_value = TASK mock.get_all_tasks.return_value = TASKS @@ -229,7 +240,7 @@ def test_create_and_start_task_fails_if_task_creation_fails( client: BlueapiClient, mock_rest: Mock, ): - mock_rest.create_task.side_effect = BlueskyRemoteControlError("No can do") + mock_rest.create_task.side_effect = BlueskyRemoteControlError(message="No can do") with pytest.raises(BlueskyRemoteControlError): client.create_and_start_task( TaskRequest(name="baz", instrument_session="cm12345-1") @@ -253,7 +264,9 @@ def test_create_and_start_task_fails_if_task_start_fails( mock_rest: Mock, ): mock_rest.create_task.return_value = TaskResponse(task_id="baz") - mock_rest.update_worker_task.side_effect = BlueskyRemoteControlError("No can do") + mock_rest.update_worker_task.side_effect = BlueskyRemoteControlError( + message="No can do" + ) with pytest.raises(BlueskyRemoteControlError): client.create_and_start_task( TaskRequest(name="baz", instrument_session="cm12345-1") diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 804fb5ee5..e4837d4f7 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -13,6 +13,7 @@ BlueskyRemoteControlError, BlueskyRequestError, InvalidParametersError, + NotFoundError, ParameterError, UnauthorisedAccessError, UnknownPlanError, @@ -41,8 +42,9 @@ def rest_with_auth(oidc_config: OIDCConfig, tmp_path) -> BlueapiRestClient: @pytest.mark.parametrize( "code,expected_exception", [ - (404, KeyError), - (401, BlueskyRemoteControlError), + (404, NotFoundError), + (401, UnauthorisedAccessError), + (403, UnauthorisedAccessError), (450, BlueskyRemoteControlError), (500, BlueskyRemoteControlError), ], @@ -65,9 +67,9 @@ def test_rest_error_code( "code,content,expected_exception", [ (200, None, None), - (401, None, UnauthorisedAccessError()), - (403, None, UnauthorisedAccessError()), - (404, None, UnknownPlanError()), + (401, None, UnauthorisedAccessError(401, "")), + (403, None, UnauthorisedAccessError(403, "")), + (404, None, UnknownPlanError(404, "")), ( 422, """{ @@ -89,6 +91,11 @@ def test_rest_error_code( ] ), ), + ( + 422, + '{"detail": "not a list"}', + BlueskyRequestError(422, ""), + ), (450, "non-standard", BlueskyRequestError(450, "non-standard")), (500, "internal_error", BlueskyRequestError(500, "internal_error")), ], @@ -104,8 +111,11 @@ def test_create_task_exceptions( response.json.side_effect = lambda: json.loads(content) if content else None err = _create_task_exceptions(response) assert isinstance(err, type(expected_exception)) - if expected_exception is not None: - assert err.args == expected_exception.args + if isinstance(expected_exception, InvalidParametersError): + assert isinstance(err, InvalidParametersError) + assert err.errors == expected_exception.errors + elif expected_exception is not None: + assert err.args[0] == code def test_auth_request_functionality( From 44dc8ed158466ed439bd6c7b4d6a325048f47361 Mon Sep 17 00:00:00 2001 From: Alexj9837 Date: Tue, 17 Mar 2026 11:51:44 +0000 Subject: [PATCH 04/20] fix: Address PR review comments --- src/blueapi/cli/cli.py | 2 +- src/blueapi/client/client.py | 8 ++++---- src/blueapi/client/rest.py | 16 ++++++++-------- tests/system_tests/test_blueapi_system.py | 20 ++++++++++---------- tests/unit_tests/cli/test_cli.py | 8 ++++---- tests/unit_tests/client/test_client.py | 6 ++---- 6 files changed, 29 insertions(+), 31 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index e6f8769c1..55addb87d 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -400,7 +400,7 @@ def on_event(event: AnyEvent) -> None: except BlueskyStreamingError as se: raise ClickException(f"streaming error: {se}") from se except BlueskyRemoteControlError as e: - raise ClickException(f"remote control error: {e.args[1]}") from e + raise ClickException(f"remote control error: {e.args[0]}") from e except ValueError as ve: raise ClickException(f"task could not run: {ve}") from ve diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index d188a3c1b..a3fc69f44 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -512,7 +512,7 @@ def inner_on_event(event: AnyEvent, ctx: MessageContext) -> None: if event.task_status is None: complete.set_exception( BlueskyRemoteControlError( - message="Server completed without task status" + "Server completed without task status" ) ) else: @@ -544,7 +544,7 @@ def create_and_start_task(self, task: TaskRequest) -> TaskResponse: return response else: raise BlueskyRemoteControlError( - message=f"Tried to create and start task {response.task_id} " + f"Tried to create and start task {response.task_id} " f"but {worker_response.task_id} was started instead" ) @@ -673,7 +673,7 @@ def reload_environment( status = self._rest.delete_environment() except Exception as e: raise BlueskyRemoteControlError( - message="Failed to tear down the environment" + "Failed to tear down the environment" ) from e return self._wait_for_reload( status, @@ -698,7 +698,7 @@ def _wait_for_reload( status = self._rest.get_environment() if status.error_message is not None: raise BlueskyRemoteControlError( - message=f"Error reloading environment: {status.error_message}" + f"Error reloading environment: {status.error_message}" ) elif ( status.initialized and status.environment_id != previous_environment_id diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index cd23f9b46..5184ff41c 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -51,7 +51,7 @@ class UnauthorisedAccessError(BlueskyRequestError): pass -class BlueskyRemoteControlError(BlueskyRequestError): +class BlueskyRemoteControlError(Exception): pass @@ -118,11 +118,11 @@ def _exception(response: requests.Response) -> Exception | None: if code < 400: return None elif code in (401, 403): - return UnauthorisedAccessError(code, str(response)) + return UnauthorisedAccessError(code, response.text) elif code == 404: - return NotFoundError(code, str(response)) + return NotFoundError(code, response.text) else: - return BlueskyRemoteControlError(code, str(response)) + return BlueskyRemoteControlError(response.text) def _create_task_exceptions(response: requests.Response) -> Exception | None: @@ -130,9 +130,9 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: if code < 400: return None elif code == 401 or code == 403: - return UnauthorisedAccessError(code, str(response)) + return UnauthorisedAccessError(code, response.text) elif code == 404: - return UnknownPlanError(code, str(response)) + return UnknownPlanError(code, response.text) elif code == 422: try: content = _response_json(response) @@ -144,9 +144,9 @@ def _create_task_exceptions(response: requests.Response) -> Exception | None: except Exception: # If the error can't be parsed into something sensible, return the # raw text in a generic exception so at least it gets reported - return BlueskyRequestError(code, str(response)) + return BlueskyRequestError(code, response.text) else: - return BlueskyRequestError(code, str(response)) + return BlueskyRequestError(code, response.text) def _response_json(response: requests.Response) -> Any: diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 4a5e503f0..52572dad5 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -219,7 +219,7 @@ def test_cannot_access_endpoints( "get_oidc_config" ) # get_oidc_config can be accessed without auth for get_method in blueapi_rest_client_get_methods: - with pytest.raises(UnauthorisedAccessError, match=r""): + with pytest.raises(UnauthorisedAccessError, match=r"Not authenticated"): getattr(client_without_auth._rest, get_method)() @@ -245,7 +245,7 @@ def test_get_plans_by_name(client: BlueapiClient, expected_plans: PlanResponse): def test_get_non_existent_plan(rest_client: BlueapiRestClient): - with pytest.raises(NotFoundError): + with pytest.raises(NotFoundError, match=r"Item not found"): rest_client.get_plan("Not exists") @@ -270,12 +270,12 @@ def test_get_device_by_name( def test_get_non_existent_device(rest_client: BlueapiRestClient): - with pytest.raises(NotFoundError): + with pytest.raises(NotFoundError, match=r"Item not found"): rest_client.get_device("Not exists") def test_client_non_existent_device(client: BlueapiClient): - with pytest.raises(AttributeError): + with pytest.raises(AttributeError, match="No device named 'missing' available"): _ = client.devices.missing @@ -297,7 +297,7 @@ def test_instrument_session_propagated(rest_client: BlueapiRestClient): def test_create_task_validation_error(rest_client: BlueapiRestClient): - with pytest.raises(BlueskyRequestError): + with pytest.raises(BlueskyRequestError, match="Internal Server Error"): rest_client.create_task( TaskRequest( name="Not-exists", @@ -338,12 +338,12 @@ def test_get_task_by_id(rest_client: BlueapiRestClient): def test_get_non_existent_task(rest_client: BlueapiRestClient): - with pytest.raises(NotFoundError): + with pytest.raises(NotFoundError, match=r"Item not found"): rest_client.get_task("Not-exists") def test_delete_non_existent_task(rest_client: BlueapiRestClient): - with pytest.raises(NotFoundError): + with pytest.raises(NotFoundError, match=r"Item not found"): rest_client.clear_task("Not-exists") @@ -365,7 +365,7 @@ def test_put_worker_task_fails_if_not_idle(rest_client: BlueapiRestClient): with pytest.raises(BlueskyRemoteControlError) as exception: rest_client.update_worker_task(WorkerTask(task_id=small_task.task_id)) - assert "" in str(exception) + assert "Worker already active" in exception.value.args[0] rest_client.cancel_current_task(WorkerState.ABORTING) rest_client.clear_task(small_task.task_id) rest_client.clear_task(long_task.task_id) @@ -378,10 +378,10 @@ def test_get_worker_state(client: BlueapiClient): def test_set_state_transition_error(client: BlueapiClient): with pytest.raises(BlueskyRemoteControlError) as exception: client.resume() - assert "" in str(exception) + assert exception.value.args[0] with pytest.raises(BlueskyRemoteControlError) as exception: client.pause() - assert "" in str(exception) + assert exception.value.args[0] def test_get_task_by_status(rest_client: BlueapiRestClient): diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 9e0837dcc..22ee86357 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -128,11 +128,11 @@ def test_authentication_error_caught_by_wrapper_func( @patch("blueapi.client.rest.requests.Session.request") def test_remote_error_raised_by_wrapper_func(mock_requests: Mock, runner: CliRunner): - mock_requests.side_effect = BlueskyRemoteControlError(message="Response [450]") + mock_requests.side_effect = BlueskyRemoteControlError("Response [450]") result = runner.invoke(main, ["controller", "plans"]) assert ( isinstance(result.exception, BlueskyRemoteControlError) - and result.exception.args[1] == "Response [450]" + and result.exception.args == ("Response [450]",) and result.exit_code == 1 ) @@ -678,7 +678,7 @@ def test_env_reload_server_side_error(runner: CliRunner): assert isinstance(result.exception, BlueskyRemoteControlError), ( "Expected a BlueskyRemoteError from cli runner" ) - assert result.exception.args[1] == "Failed to tear down the environment" + assert result.exception.args[0] == "Failed to tear down the environment" # Check if the endpoints were hit as expected assert len(responses.calls) == 1 # +1 for the DELETE call @@ -714,7 +714,7 @@ def test_env_reload_server_side_error(runner: CliRunner): "Error: Incorrect parameters supplied\n Missing value for 'foo'\n", ), ( - BlueskyRemoteControlError(message="Server error"), + BlueskyRemoteControlError("Server error"), "Error: remote control error: Server error\n", ), ( diff --git a/tests/unit_tests/client/test_client.py b/tests/unit_tests/client/test_client.py index f9d2eadcd..b5c59c9bc 100644 --- a/tests/unit_tests/client/test_client.py +++ b/tests/unit_tests/client/test_client.py @@ -240,7 +240,7 @@ def test_create_and_start_task_fails_if_task_creation_fails( client: BlueapiClient, mock_rest: Mock, ): - mock_rest.create_task.side_effect = BlueskyRemoteControlError(message="No can do") + mock_rest.create_task.side_effect = BlueskyRemoteControlError("No can do") with pytest.raises(BlueskyRemoteControlError): client.create_and_start_task( TaskRequest(name="baz", instrument_session="cm12345-1") @@ -264,9 +264,7 @@ def test_create_and_start_task_fails_if_task_start_fails( mock_rest: Mock, ): mock_rest.create_task.return_value = TaskResponse(task_id="baz") - mock_rest.update_worker_task.side_effect = BlueskyRemoteControlError( - message="No can do" - ) + mock_rest.update_worker_task.side_effect = BlueskyRemoteControlError("No can do") with pytest.raises(BlueskyRemoteControlError): client.create_and_start_task( TaskRequest(name="baz", instrument_session="cm12345-1") From 0a9cbff71b50b5e6871fdf0a0ccbcc5c7fa5d917 Mon Sep 17 00:00:00 2001 From: Alexj9837 Date: Tue, 17 Mar 2026 13:27:12 +0000 Subject: [PATCH 05/20] added content for test --- tests/unit_tests/client/test_rest.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index e4837d4f7..c6f6de480 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -67,9 +67,17 @@ def test_rest_error_code( "code,content,expected_exception", [ (200, None, None), - (401, None, UnauthorisedAccessError(401, "")), - (403, None, UnauthorisedAccessError(403, "")), - (404, None, UnknownPlanError(404, "")), + ( + 401, + "unauthorised access", + UnauthorisedAccessError(401, "unauthorised access"), + ), + (403, "Forbidden", UnauthorisedAccessError(403, "Forbidden")), + ( + 404, + "Plan '{name}' was not recognised", + UnknownPlanError(404, "Plan '{name}' was not recognised"), + ), ( 422, """{ @@ -116,6 +124,8 @@ def test_create_task_exceptions( assert err.errors == expected_exception.errors elif expected_exception is not None: assert err.args[0] == code + if content is not None: + assert err.args[1] == content def test_auth_request_functionality( From b3a5fb3ad1a71ebe62d0bec2b4204b3a37a8eeee Mon Sep 17 00:00:00 2001 From: Alex J Date: Fri, 10 Apr 2026 16:25:14 +0100 Subject: [PATCH 06/20] Update src/blueapi/cli/cli.py Poor grammar. Co-authored-by: Peter Holloway --- src/blueapi/cli/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 55addb87d..296cbc9cc 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -398,9 +398,9 @@ def on_event(event: AnyEvent) -> None: except InvalidParametersError as ip: raise ClickException(ip.message()) from ip except BlueskyStreamingError as se: - raise ClickException(f"streaming error: {se}") from se + raise ClickException(f"Streaming error: {se}") from se except BlueskyRemoteControlError as e: - raise ClickException(f"remote control error: {e.args[0]}") from e + raise ClickException(f"Remote control error: {e.args[0]}") from e except ValueError as ve: raise ClickException(f"task could not run: {ve}") from ve From 3d1b21069d30f318506254cf7cedced2d6c2c3c7 Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Fri, 10 Apr 2026 15:55:53 +0000 Subject: [PATCH 07/20] updated expected error message for poor grammar --- tests/unit_tests/cli/test_cli.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 22ee86357..73f40abb4 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -699,7 +699,10 @@ def test_env_reload_server_side_error(runner: CliRunner): "exception, error_message", [ (UnknownPlanError(), "Error: Plan 'sleep' was not recognised\n"), - (UnauthorisedAccessError(), "Error: Unauthorised request\n"), + ( + UnauthorisedAccessError(), + "Error: Access denied. Please check your login status and try again.\n", + ), ( InvalidParametersError( errors=[ @@ -715,7 +718,7 @@ def test_env_reload_server_side_error(runner: CliRunner): ), ( BlueskyRemoteControlError("Server error"), - "Error: remote control error: Server error\n", + "Error: Remote control error: Server error\n", ), ( ValueError("Error parsing parameters"), @@ -723,7 +726,7 @@ def test_env_reload_server_side_error(runner: CliRunner): ), ( BlueskyStreamingError("streaming failed"), - "Error: streaming error: streaming failed\n", + "Error: Streaming error: streaming failed\n", ), ], ids=[ From 71fa2e7b1345e7f6c78e42b8b2c33e012497664f Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Mon, 13 Apr 2026 13:02:57 +0000 Subject: [PATCH 08/20] Removing "I think this will prevent your new check_connection changes having an effect when running plans - you'll still get the Unauthorised request message" --- src/blueapi/cli/cli.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 296cbc9cc..1202f4111 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -393,8 +393,6 @@ def on_event(event: AnyEvent) -> None: raise ClickException(*mse.args) from mse except UnknownPlanError as up: raise ClickException(f"Plan '{name}' was not recognised") from up - except UnauthorisedAccessError as ua: - raise ClickException("Unauthorised request") from ua except InvalidParametersError as ip: raise ClickException(ip.message()) from ip except BlueskyStreamingError as se: From ddfdf00b5ae278fb1d687e4c223ca4ba55168086 Mon Sep 17 00:00:00 2001 From: alexj9837 Date: Tue, 14 Apr 2026 11:53:15 +0000 Subject: [PATCH 09/20] Fix: 400 responsed previously raised a bare status with no body, updated to return some more details --- src/blueapi/service/main.py | 13 +++++++++---- tests/system_tests/test_blueapi_system.py | 4 ++-- tests/unit_tests/service/test_rest_api.py | 10 +++++++--- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/blueapi/service/main.py b/src/blueapi/service/main.py index a53c46885..df6aab221 100644 --- a/src/blueapi/service/main.py +++ b/src/blueapi/service/main.py @@ -534,11 +534,16 @@ def set_state( state_change_request.new_state is WorkerState.ABORTING, state_change_request.reason, ) - except TransitionError: - response.status_code = status.HTTP_400_BAD_REQUEST + except TransitionError as e: + raise HTTPException( + status.HTTP_400_BAD_REQUEST, + detail=(f"Cannot transition from {current_state} to {new_state}"), + ) from e else: - response.status_code = status.HTTP_400_BAD_REQUEST - + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=(f"Cannot transition from {current_state} to {new_state}"), + ) return runner.run(interface.get_worker_state) diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 52572dad5..1ad54761e 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -378,10 +378,10 @@ def test_get_worker_state(client: BlueapiClient): def test_set_state_transition_error(client: BlueapiClient): with pytest.raises(BlueskyRemoteControlError) as exception: client.resume() - assert exception.value.args[0] + assert "Cannot transition from IDLE to RUNNING" in exception.value.args[0] with pytest.raises(BlueskyRemoteControlError) as exception: client.pause() - assert exception.value.args[0] + assert "Cannot transition from IDLE to PAUSED" in exception.value.args[0] def test_get_task_by_status(rest_client: BlueapiRestClient): diff --git a/tests/unit_tests/service/test_rest_api.py b/tests/unit_tests/service/test_rest_api.py index c1d3b6a95..93077dbb8 100644 --- a/tests/unit_tests/service/test_rest_api.py +++ b/tests/unit_tests/service/test_rest_api.py @@ -635,7 +635,7 @@ def test_set_state_transition_error(mock_runner: Mock, client: TestClient): current_state = WorkerState.RUNNING final_state = WorkerState.STOPPING - mock_runner.run.side_effect = [current_state, TransitionError(), final_state] + mock_runner.run.side_effect = [current_state, TransitionError()] response = client.put( "/worker/state", @@ -643,7 +643,9 @@ def test_set_state_transition_error(mock_runner: Mock, client: TestClient): ) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == final_state + assert response.json() == { + "detail": f"Cannot transition from {current_state} to {final_state}" + } def test_set_state_invalid_transition(mock_runner: Mock, client: TestClient): @@ -659,7 +661,9 @@ def test_set_state_invalid_transition(mock_runner: Mock, client: TestClient): ) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == final_state + assert response.json() == { + "detail": f"Cannot transition from {current_state} to {requested_state}" + } def test_get_environment_idle(mock_runner: Mock, client: TestClient) -> None: From ecfe2b74d5493670dced78deb76e6a8e30a791a8 Mon Sep 17 00:00:00 2001 From: Alex J Date: Thu, 16 Apr 2026 15:00:02 +0100 Subject: [PATCH 10/20] chore: improve default error handling 1379 1409 (#1490) rebased from main to fix merge conflicts --------- Co-authored-by: Shreelakshmi Iyengar Co-authored-by: Zoheb Shaikh Co-authored-by: Daniel Fernandes <65790536+dan-fernandes@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Zoheb Shaikh <26975142+ZohebShaikh@users.noreply.github.com> Co-authored-by: Abigail Emery Co-authored-by: oliwenmandiamond <136330507+oliwenmandiamond@users.noreply.github.com> Co-authored-by: Peter Holloway Co-authored-by: NeilSmithDLS <118457264+NeilSmithDLS@users.noreply.github.com> --- src/blueapi/client/rest.py | 32 ++++++++++++++++++----- tests/system_tests/test_blueapi_system.py | 6 ++--- tests/unit_tests/cli/test_cli.py | 2 +- tests/unit_tests/client/test_rest.py | 14 +++------- 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 5184ff41c..3d91cf5a7 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -38,28 +38,41 @@ LOGGER = logging.getLogger(__name__) -class NonJsonResponseError(Exception): - pass +class BlueskyRequestError(Exception): + """Error response from the blueapi server.(422,450,500)""" + def __init__(self, code: int | None = None, message: str = "") -> None: + super().__init__(code, message) + + +class UnauthorisedAccessError(Exception): + """Request was rejected due to missing or invalid credentials (401/403).""" -class BlueskyRequestError(Exception): def __init__(self, code: int | None = None, message: str = "") -> None: super().__init__(code, message) -class UnauthorisedAccessError(BlueskyRequestError): +class BlueskyRemoteControlError(Exception): + """Failure communicating with the blueapi server (e.g. connection refused).""" + pass -class BlueskyRemoteControlError(Exception): +class NonJsonResponseError(Exception): + """Server returned a response that could not be parsed as JSON.""" + pass class NotFoundError(BlueskyRequestError): + """Requested something that couldn't be found (404).""" + pass class UnknownPlanError(BlueskyRequestError): + """ "Plan '{name}' was not recognised" """ + pass @@ -122,7 +135,14 @@ def _exception(response: requests.Response) -> Exception | None: elif code == 404: return NotFoundError(code, response.text) else: - return BlueskyRemoteControlError(response.text) + try: + body = _response_json(response) + message = (body.get("detail") if isinstance(body, dict) else None) or ( + response.text + ) + except NonJsonResponseError: + message = response.text + return BlueskyRemoteControlError(code, message) def _create_task_exceptions(response: requests.Response) -> Exception | None: diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 1ad54761e..0831a7723 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -365,7 +365,7 @@ def test_put_worker_task_fails_if_not_idle(rest_client: BlueapiRestClient): with pytest.raises(BlueskyRemoteControlError) as exception: rest_client.update_worker_task(WorkerTask(task_id=small_task.task_id)) - assert "Worker already active" in exception.value.args[0] + assert exception.value.args[0] == 409 rest_client.cancel_current_task(WorkerState.ABORTING) rest_client.clear_task(small_task.task_id) rest_client.clear_task(long_task.task_id) @@ -378,10 +378,10 @@ def test_get_worker_state(client: BlueapiClient): def test_set_state_transition_error(client: BlueapiClient): with pytest.raises(BlueskyRemoteControlError) as exception: client.resume() - assert "Cannot transition from IDLE to RUNNING" in exception.value.args[0] + assert "Cannot transition from IDLE to RUNNING" in exception.value.args[1] with pytest.raises(BlueskyRemoteControlError) as exception: client.pause() - assert "Cannot transition from IDLE to PAUSED" in exception.value.args[0] + assert "Cannot transition from IDLE to PAUSED" in exception.value.args[1] def test_get_task_by_status(rest_client: BlueapiRestClient): diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 73f40abb4..5c2b41868 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -1169,7 +1169,7 @@ def test_invalid_json( responses.GET, "http://localhost:8000/config/oidc", body="blah blah", - status=404, + status=200, ) result = runner.invoke(main, ["-c", config_with_auth, "login"]) diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index c6f6de480..101c67305 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -67,17 +67,9 @@ def test_rest_error_code( "code,content,expected_exception", [ (200, None, None), - ( - 401, - "unauthorised access", - UnauthorisedAccessError(401, "unauthorised access"), - ), - (403, "Forbidden", UnauthorisedAccessError(403, "Forbidden")), - ( - 404, - "Plan '{name}' was not recognised", - UnknownPlanError(404, "Plan '{name}' was not recognised"), - ), + (401, "", UnauthorisedAccessError(401, "")), + (403, "", UnauthorisedAccessError(403, "")), + (404, "", UnknownPlanError(404, "")), ( 422, """{ From 60ec9759eeea0d00ac5fc86e193e09a13d8d3e73 Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 22 Apr 2026 14:08:34 +0000 Subject: [PATCH 11/20] add another test for non_json_body --- tests/unit_tests/client/test_rest.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 101c67305..350a2d8e6 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -18,6 +18,7 @@ UnauthorisedAccessError, UnknownPlanError, _create_task_exceptions, + _exception, ) from blueapi.config import OIDCConfig from blueapi.service.authentication import SessionCacheManager, SessionManager @@ -120,6 +121,21 @@ def test_create_task_exceptions( assert err.args[1] == content +def test_exception_non_json_body_falls_back_to_text(): + import json + + response = Mock(spec=requests.Response) + response.status_code = 500 + response.text = "Internal Server Error" + response.json.side_effect = json.JSONDecodeError("Expecting value", "", 0) + response.request = Mock() + response.request.url = "http://localhost:8000/test" + response.content = b"Internal Server Error" + err = _exception(response) + assert isinstance(err, BlueskyRemoteControlError) + assert err.args[1] == "Internal Server Error" + + def test_auth_request_functionality( rest_with_auth: BlueapiRestClient, mock_authn_server: responses.RequestsMock, From 01901476848c353a6cfb211e7c605ef5a8186867 Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 22 Apr 2026 14:15:43 +0000 Subject: [PATCH 12/20] chasing from raising a KeyError to a NotFoundError --- tests/system_tests/test_blueapi_system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 0831a7723..d7ca42497 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -623,7 +623,7 @@ def on_event(event: AnyEvent) -> None: # Regression test for #1480 def test_task_submission_after_invalid_task(client_with_stomp: BlueapiClient): - with pytest.raises(KeyError): + with pytest.raises(NotFoundError): # This task hasn't been submitted so should return an error... client_with_stomp._rest.update_worker_task(WorkerTask(task_id="missing")) From 869faff793a4a9a21ba4124cfc70d28ee1ce6fb8 Mon Sep 17 00:00:00 2001 From: Alex J <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 22 Apr 2026 17:17:57 +0100 Subject: [PATCH 13/20] Update src/blueapi/client/rest.py Co-authored-by: Peter Holloway --- src/blueapi/client/rest.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 3d91cf5a7..8f982cc55 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -137,9 +137,7 @@ def _exception(response: requests.Response) -> Exception | None: else: try: body = _response_json(response) - message = (body.get("detail") if isinstance(body, dict) else None) or ( - response.text - ) + message = body.get("detail", response.text) if isinstance(body, dict) else response.text except NonJsonResponseError: message = response.text return BlueskyRemoteControlError(code, message) From 13cdba0fc71e086e4ede21acd2a0de70737c8189 Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 22 Apr 2026 16:20:03 +0000 Subject: [PATCH 14/20] responding to PR feedback --- src/blueapi/client/rest.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 8f982cc55..de484e75a 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -39,39 +39,38 @@ class BlueskyRequestError(Exception): - """Error response from the blueapi server.(422,450,500)""" + """An error response from the blueapi server.""" def __init__(self, code: int | None = None, message: str = "") -> None: super().__init__(code, message) -class UnauthorisedAccessError(Exception): +class UnauthorisedAccessError(BlueskyRequestError): """Request was rejected due to missing or invalid credentials (401/403).""" - def __init__(self, code: int | None = None, message: str = "") -> None: - super().__init__(code, message) + pass -class BlueskyRemoteControlError(Exception): - """Failure communicating with the blueapi server (e.g. connection refused).""" +class NotFoundError(BlueskyRequestError): + """Requested something that couldn't be found (404).""" pass -class NonJsonResponseError(Exception): - """Server returned a response that could not be parsed as JSON.""" +class UnknownPlanError(BlueskyRequestError): + """Plan '{name}' was not recognised""" pass -class NotFoundError(BlueskyRequestError): - """Requested something that couldn't be found (404).""" +class BlueskyRemoteControlError(Exception): + """Unexpected or failed response from the blueapi server.""" pass -class UnknownPlanError(BlueskyRequestError): - """ "Plan '{name}' was not recognised" """ +class NonJsonResponseError(Exception): + """Server returned a response that could not be parsed as JSON.""" pass From ea376ce186ba8658f1c29f935414274594a0b02a Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 22 Apr 2026 16:22:24 +0000 Subject: [PATCH 15/20] formatting --- src/blueapi/client/rest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index de484e75a..4df435c40 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -136,7 +136,11 @@ def _exception(response: requests.Response) -> Exception | None: else: try: body = _response_json(response) - message = body.get("detail", response.text) if isinstance(body, dict) else response.text + message = ( + body.get("detail", response.text) + if isinstance(body, dict) + else response.text + ) except NonJsonResponseError: message = response.text return BlueskyRemoteControlError(code, message) From 554181c5f71d0c0d82ffc6d09075a238f35c1e2b Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Thu, 23 Apr 2026 12:24:24 +0000 Subject: [PATCH 16/20] raise correct error message for reload_enviroment --- src/blueapi/client/client.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index a3fc69f44..24be2b0f3 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -42,7 +42,13 @@ from blueapi.worker.task_worker import TrackableTask from .event_bus import AnyEvent, EventBusClient, OnAnyEvent -from .rest import BlueapiRestClient, BlueskyRemoteControlError, NotFoundError +from .rest import ( + BlueapiRestClient, + BlueskyRemoteControlError, + BlueskyRequestError, + NotFoundError, + ServiceUnavailableError, +) TRACER = get_tracer("client") @@ -668,9 +674,10 @@ def reload_environment( EnvironmentResponse: Details of the new worker environment. """ - try: status = self._rest.delete_environment() + except (BlueskyRequestError, ServiceUnavailableError): + raise except Exception as e: raise BlueskyRemoteControlError( "Failed to tear down the environment" From 01dff9ec87f72eb21d87e6f49e128015a5b0ad77 Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Thu, 23 Apr 2026 14:13:16 +0000 Subject: [PATCH 17/20] import json at the top --- tests/unit_tests/client/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 350a2d8e6..806f14c20 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -1,3 +1,4 @@ +import json import uuid from pathlib import Path from unittest.mock import MagicMock, Mock, patch @@ -107,7 +108,6 @@ def test_create_task_exceptions( response = Mock(spec=requests.Response) response.status_code = code response.text = content - import json response.json.side_effect = lambda: json.loads(content) if content else None err = _create_task_exceptions(response) From f29f33a8ad897ae514abf6d773355d05c46b7f1d Mon Sep 17 00:00:00 2001 From: alexj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Thu, 23 Apr 2026 16:20:53 +0000 Subject: [PATCH 18/20] feedback from PR comments --- src/blueapi/cli/cli.py | 2 -- src/blueapi/client/client.py | 11 +++++++---- src/blueapi/client/rest.py | 5 ++++- src/blueapi/service/main.py | 6 +++++- tests/system_tests/test_blueapi_system.py | 3 ++- tests/unit_tests/service/test_rest_api.py | 2 +- 6 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 1202f4111..8c746fe92 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -258,8 +258,6 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T: raise ClickException( "Access denied. Please check your login status and try again." ) from e - except BlueskyRemoteControlError as e: - raise e return wrapper diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index 24be2b0f3..aea367447 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -105,9 +105,8 @@ def __getitem__(self, name: str) -> "DeviceRef": self._cache[name] = device setattr(self, model.name, device) return device - except NotFoundError: - pass - raise AttributeError(f"No device named '{name}' available") + except NotFoundError as e: + raise AttributeError(f"No device named '{name}' available") from e def __getattr__(self, name: str) -> "DeviceRef": if name.startswith("_"): @@ -676,7 +675,11 @@ def reload_environment( """ try: status = self._rest.delete_environment() - except (BlueskyRequestError, ServiceUnavailableError): + except ( + BlueskyRequestError, + BlueskyRemoteControlError, + ServiceUnavailableError, + ): raise except Exception as e: raise BlueskyRemoteControlError( diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 4df435c40..cacbc1656 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -200,7 +200,10 @@ def get_plans(self) -> PlanResponse: return self._request_and_deserialize("/plans", PlanResponse) def get_plan(self, name: str) -> PlanModel: - return self._request_and_deserialize(f"/plans/{name}", PlanModel) + try: + return self._request_and_deserialize(f"/plans/{name}", PlanModel) + except NotFoundError as e: + raise UnknownPlanError(404, f"Plan '{name}' not found") from e def get_devices(self) -> DeviceResponse: return self._request_and_deserialize("/devices", DeviceResponse) diff --git a/src/blueapi/service/main.py b/src/blueapi/service/main.py index df6aab221..8ea3c95e1 100644 --- a/src/blueapi/service/main.py +++ b/src/blueapi/service/main.py @@ -535,9 +535,13 @@ def set_state( state_change_request.reason, ) except TransitionError as e: + suffix = f" - {e}" if str(e) else "" raise HTTPException( status.HTTP_400_BAD_REQUEST, - detail=(f"Cannot transition from {current_state} to {new_state}"), + detail=( + f"Error while transitioning from {current_state} " + f"to {new_state}{suffix}" + ), ) from e else: raise HTTPException( diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index d7ca42497..bb1c41dfe 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -18,6 +18,7 @@ NotFoundError, ServiceUnavailableError, UnauthorisedAccessError, + UnknownPlanError, ) from blueapi.config import ( ApplicationConfig, @@ -245,7 +246,7 @@ def test_get_plans_by_name(client: BlueapiClient, expected_plans: PlanResponse): def test_get_non_existent_plan(rest_client: BlueapiRestClient): - with pytest.raises(NotFoundError, match=r"Item not found"): + with pytest.raises(UnknownPlanError, match=r"Plan 'Not exists' not found"): rest_client.get_plan("Not exists") diff --git a/tests/unit_tests/service/test_rest_api.py b/tests/unit_tests/service/test_rest_api.py index 93077dbb8..1ddf2c6ca 100644 --- a/tests/unit_tests/service/test_rest_api.py +++ b/tests/unit_tests/service/test_rest_api.py @@ -644,7 +644,7 @@ def test_set_state_transition_error(mock_runner: Mock, client: TestClient): assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.json() == { - "detail": f"Cannot transition from {current_state} to {final_state}" + "detail": f"Error while transitioning from {current_state} to {final_state}" } From 69357f3c3cb832aaa9e7ac5991b9a9891721d553 Mon Sep 17 00:00:00 2001 From: alxj9837 <52531949+Alexj9837@users.noreply.github.com> Date: Wed, 27 May 2026 15:04:29 +0000 Subject: [PATCH 19/20] failing test --- src/blueapi/client/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/blueapi/client/client.py b/src/blueapi/client/client.py index aea367447..68637594f 100644 --- a/src/blueapi/client/client.py +++ b/src/blueapi/client/client.py @@ -677,7 +677,6 @@ def reload_environment( status = self._rest.delete_environment() except ( BlueskyRequestError, - BlueskyRemoteControlError, ServiceUnavailableError, ): raise From 7bc4e11d04ab98385c1e90274583402ca5ce30db Mon Sep 17 00:00:00 2001 From: Zoheb Shaikh Date: Thu, 28 May 2026 09:20:12 +0100 Subject: [PATCH 20/20] Potential fix for pull request finding 'Module is imported more than once' Also mentioned here https://github.com/DiamondLightSource/blueapi/pull/1491/changes#r3131175025 Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- tests/unit_tests/client/test_rest.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 028ec2061..56283fc85 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -134,8 +134,6 @@ def test_create_task_exceptions( def test_exception_non_json_body_falls_back_to_text(): - import json - response = Mock(spec=requests.Response) response.status_code = 500 response.text = "Internal Server Error"