fix: Address PR review comments

Author: Alexj9837

src/blueapi/cli/cli.py

@@ -358,7 +358,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
 

src/blueapi/client/client.py

@@ -498,7 +498,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:
@@ -530,7 +530,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"
             )
 
@@ -659,7 +659,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,
@@ -684,7 +684,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

src/blueapi/client/rest.py

@@ -42,7 +42,7 @@ class UnauthorisedAccessError(BlueskyRequestError):
     pass
 
 
-class BlueskyRemoteControlError(BlueskyRequestError):
+class BlueskyRemoteControlError(Exception):
     pass
 
 
@@ -109,21 +109,21 @@ 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:
     code = response.status_code
     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()
@@ -135,9 +135,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)
 
 
 class BlueapiRestClient:

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"<Response \[401\]>"):
+        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 "<Response [409]>" 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 "<Response [400]>" in str(exception)
+    assert exception.value.args[0]
     with pytest.raises(BlueskyRemoteControlError) as exception:
         client.pause()
-    assert "<Response [400]>" in str(exception)
+    assert exception.value.args[0]
 
 
 def test_get_task_by_status(rest_client: BlueapiRestClient):

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",
         ),
         (

tests/unit_tests/client/test_client.py

@@ -225,7 +225,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")
@@ -249,9 +249,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")