Add ROS2 action client tests and fix KeyError for missing status field

Co-authored-by: gonzalocasas <[email protected]>

Author: Copilot

src/roslibpy/comm/comm.py

@@ -163,9 +163,18 @@ def _handle_action_result(self, message):
         resultback, _ , errback = action_handlers
         del self._pending_action_requests[request_id]
 
-        LOGGER.debug("Received Action result with status: %s", message["status"])
-
-        results = {"status": ActionGoalStatus(message["status"]).name, "values": message["values"]}
+        # Handle different message formats for status field
+        # ROS2 rosbridge may send status at top level or inside values
+        status = message.get("status")
+        if status is None and "values" in message:
+            status = message["values"].get("status")
+        if status is None:
+            # Default to UNKNOWN if status is not found
+            status = ActionGoalStatus.UNKNOWN.value
+
+        LOGGER.debug("Received Action result with status: %s", status)
+
+        results = {"status": ActionGoalStatus(status).name, "values": message.get("values", {})}
 
         if "result" in message and message["result"] is False:
             if errback:

tests/ros2/test_action_client.py

@@ -0,0 +1,146 @@
+"""Tests for ROS2 ActionClient message handling."""
+from unittest.mock import Mock
+
+from roslibpy.comm.comm import RosBridgeProtocol
+from roslibpy.core import ActionResult, ActionGoalStatus
+
+
+def test_action_result_with_status_at_top_level():
+    """Test handling of action results with status at the top level of the message."""
+    protocol = RosBridgeProtocol()
+    protocol.factory = Mock()
+    protocol.send_message = Mock()
+
+    result_callback = Mock()
+    feedback_callback = Mock()
+    error_callback = Mock()
+
+    request_id = "send_action_goal:/test_action:1"
+    protocol._pending_action_requests[request_id] = (result_callback, feedback_callback, error_callback)
+
+    # ROS2 rosbridge message format with status at top level
+    message = {
+        "op": "action_result",
+        "action": "/test_action",
+        "id": request_id,
+        "status": 4,  # SUCCEEDED
+        "values": {
+            "result": {"success": True, "message": "Action completed"}
+        },
+        "result": True
+    }
+
+    protocol._handle_action_result(message)
+
+    assert result_callback.called
+    result = result_callback.call_args[0][0]
+    assert isinstance(result, ActionResult)
+    assert result["status"] == ActionGoalStatus.SUCCEEDED.name
+    assert result["values"] == message["values"]
+
+
+def test_action_result_with_status_in_values():
+    """Test handling of action results with status inside the values field.
+
+    This reproduces the KeyError issue reported in GitHub issue.
+    """
+    protocol = RosBridgeProtocol()
+    protocol.factory = Mock()
+    protocol.send_message = Mock()
+
+    result_callback = Mock()
+    feedback_callback = Mock()
+    error_callback = Mock()
+
+    request_id = "send_action_goal:/test_action:2"
+    protocol._pending_action_requests[request_id] = (result_callback, feedback_callback, error_callback)
+
+    # Alternative message format with status inside values
+    message = {
+        "op": "action_result",
+        "action": "/test_action",
+        "id": request_id,
+        "values": {
+            "status": 4,  # SUCCEEDED - status is here instead
+            "result": {"success": True, "message": "Action completed"}
+        },
+        "result": True
+    }
+
+    # This should not raise KeyError
+    protocol._handle_action_result(message)
+
+    assert result_callback.called
+    result = result_callback.call_args[0][0]
+    assert isinstance(result, ActionResult)
+    assert result["status"] == ActionGoalStatus.SUCCEEDED.name
+
+
+def test_action_result_failure_with_status_at_top_level():
+    """Test handling of failed action results."""
+    protocol = RosBridgeProtocol()
+    protocol.factory = Mock()
+    protocol.send_message = Mock()
+
+    result_callback = Mock()
+    feedback_callback = Mock()
+    error_callback = Mock()
+
+    request_id = "send_action_goal:/test_action:3"
+    protocol._pending_action_requests[request_id] = (result_callback, feedback_callback, error_callback)
+
+    message = {
+        "op": "action_result",
+        "action": "/test_action",
+        "id": request_id,
+        "status": 6,  # ABORTED
+        "values": {
+            "result": {"success": False, "message": "Action failed"}
+        },
+        "result": False
+    }
+
+    protocol._handle_action_result(message)
+
+    assert error_callback.called
+    result = error_callback.call_args[0][0]
+    assert result["status"] == ActionGoalStatus.ABORTED.name
+    assert result["values"] == message["values"]
+
+
+def test_action_result_without_status_field():
+    """Test handling of action results when status field is missing entirely.
+
+    This is a defensive test for cases where neither top-level nor values contain status.
+    In such cases, we should use a default status.
+    """
+    protocol = RosBridgeProtocol()
+    protocol.factory = Mock()
+    protocol.send_message = Mock()
+
+    result_callback = Mock()
+    feedback_callback = Mock()
+    error_callback = Mock()
+
+    request_id = "send_action_goal:/test_action:4"
+    protocol._pending_action_requests[request_id] = (result_callback, feedback_callback, error_callback)
+
+    # Message without status anywhere
+    message = {
+        "op": "action_result",
+        "action": "/test_action",
+        "id": request_id,
+        "values": {
+            "result": {"success": True}
+        },
+        "result": True
+    }
+
+    # Should not raise KeyError, should use default status
+    protocol._handle_action_result(message)
+
+    assert result_callback.called
+    result = result_callback.call_args[0][0]
+    assert isinstance(result, ActionResult)
+    # Should have some status value (likely UNKNOWN)
+    assert "status" in result