refactor(services): migrate to rosapi_service()/rosapi_type(), add tool tests (step4)

ros_mcp/tools/services.py

@@ -4,6 +4,7 @@
 from mcp.types import ToolAnnotations
 
 from ros_mcp.utils.response import _check_response, _safe_get_values
+from ros_mcp.utils.rosapi_types import rosapi_service, rosapi_type
 from ros_mcp.utils.websocket import WebSocketManager
 
 
@@ -30,8 +31,8 @@ def get_services() -> dict:
         """
         message = {
             "op": "call_service",
-            "service": "/rosapi/services",
-            "type": "rosapi_msgs/srv/Services",
+            "service": rosapi_service("services"),
+            "type": rosapi_type("Services"),
             "args": {},
             "id": "get_services_request_1",
         }
@@ -74,8 +75,8 @@ def get_service_type(service: str) -> dict:
 
         message = {
             "op": "call_service",
-            "service": "/rosapi/service_type",
-            "type": "rosapi_msgs/srv/ServiceType",
+            "service": rosapi_service("service_type"),
+            "type": rosapi_type("ServiceType"),
             "args": {"service": service},
             "id": f"get_service_type_request_{service.replace('/', '_')}",
         }
@@ -134,8 +135,8 @@ def get_service_details(service: str) -> dict:
             # First get the service type
             type_message = {
                 "op": "call_service",
-                "service": "/rosapi/service_type",
-                "type": "rosapi_msgs/srv/ServiceType",
+                "service": rosapi_service("service_type"),
+                "type": rosapi_type("ServiceType"),
                 "args": {"service": service},
                 "id": f"get_service_type_{service.replace('/', '_')}",
             }
@@ -157,8 +158,8 @@ def get_service_details(service: str) -> dict:
             # Get request details
             request_message = {
                 "op": "call_service",
-                "service": "/rosapi/service_request_details",
-                "type": "rosapi_msgs/srv/ServiceRequestDetails",
+                "service": rosapi_service("service_request_details"),
+                "type": rosapi_type("ServiceRequestDetails"),
                 "args": {"type": result["type"]},
                 "id": f"get_service_details_request_{result['type'].replace('/', '_')}",
             }
@@ -179,8 +180,8 @@ def get_service_details(service: str) -> dict:
             # Get response details
             response_message = {
                 "op": "call_service",
-                "service": "/rosapi/service_response_details",
-                "type": "rosapi_msgs/srv/ServiceResponseDetails",
+                "service": rosapi_service("service_response_details"),
+                "type": rosapi_type("ServiceResponseDetails"),
                 "args": {"type": result["type"]},
                 "id": f"get_service_details_response_{result['type'].replace('/', '_')}",
             }
@@ -201,8 +202,8 @@ def get_service_details(service: str) -> dict:
             # Get service providers
             provider_message = {
                 "op": "call_service",
-                "service": "/rosapi/service_node",
-                "type": "rosapi_msgs/srv/ServiceNode",
+                "service": rosapi_service("service_node"),
+                "type": rosapi_type("ServiceNode"),
                 "args": {"service": service},
                 "id": f"get_service_providers_request_{service.replace('/', '_')}",
             }

ros_mcp/tools/topics.py

@@ -11,6 +11,7 @@
 
 from ros_mcp.tools.images import _encode_image_to_imagecontent, convert_expects_image_hint
 from ros_mcp.utils.response import _check_response, _safe_get_values
+from ros_mcp.utils.rosapi_types import rosapi_service, rosapi_type
 from ros_mcp.utils.websocket import WebSocketManager, parse_input
 
 
@@ -38,8 +39,8 @@ def get_topics() -> dict:
         # rosbridge service call to get topic list
         message = {
             "op": "call_service",
-            "service": "/rosapi/topics",
-            "type": "rosapi/Topics",
+            "service": rosapi_service("topics"),
+            "type": rosapi_type("Topics"),
             "args": {},
             "id": "get_topics_request_1",
         }
@@ -87,8 +88,8 @@ def get_topic_type(topic: str) -> dict:
         # rosbridge service call to get topic type
         message = {
             "op": "call_service",
-            "service": "/rosapi/topic_type",
-            "type": "rosapi/TopicType",
+            "service": rosapi_service("topic_type"),
+            "type": rosapi_type("TopicType"),
             "args": {"topic": topic},
             "id": f"get_topic_type_request_{topic.replace('/', '_')}",
         }
@@ -149,8 +150,8 @@ def get_topic_details(topic: str) -> dict:
             # Get topic type
             type_message = {
                 "op": "call_service",
-                "service": "/rosapi/topic_type",
-                "type": "rosapi/TopicType",
+                "service": rosapi_service("topic_type"),
+                "type": rosapi_type("TopicType"),
                 "args": {"topic": topic},
                 "id": f"get_topic_type_{topic.replace('/', '_')}",
             }
@@ -163,8 +164,8 @@ def get_topic_details(topic: str) -> dict:
             # Get publishers for this topic
             publishers_message = {
                 "op": "call_service",
-                "service": "/rosapi/publishers",
-                "type": "rosapi/Publishers",
+                "service": rosapi_service("publishers"),
+                "type": rosapi_type("Publishers"),
                 "args": {"topic": topic},
                 "id": f"get_publishers_{topic.replace('/', '_')}",
             }
@@ -177,8 +178,8 @@ def get_topic_details(topic: str) -> dict:
             # Get subscribers for this topic
             subscribers_message = {
                 "op": "call_service",
-                "service": "/rosapi/subscribers",
-                "type": "rosapi/Subscribers",
+                "service": rosapi_service("subscribers"),
+                "type": rosapi_type("Subscribers"),
                 "args": {"topic": topic},
                 "id": f"get_subscribers_{topic.replace('/', '_')}",
             }
@@ -226,8 +227,8 @@ def get_message_details(message_type: str) -> dict:
         # rosbridge service call to get message details
         message = {
             "op": "call_service",
-            "service": "/rosapi/message_details",
-            "type": "rosapi/MessageDetails",
+            "service": rosapi_service("message_details"),
+            "type": rosapi_type("MessageDetails"),
             "args": {"type": message_type},
             "id": f"get_message_details_request_{message_type.replace('/', '_')}",
         }

tests/integration/scripts/run-connection-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run connection integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-connection-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" connection

tests/integration/scripts/run-detect-version-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run detect_version integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-detect-version-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" detect_version

tests/integration/scripts/run-nodes-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run nodes integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-nodes-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" nodes

tests/integration/scripts/run-services-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run services integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-services-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" services

tests/integration/scripts/run-tests.sh

@@ -1,12 +1,25 @@
 #!/bin/bash
 # Run integration tests against a specific ROS distro.
-# Usage: ./tests/integration/scripts/run-tests.sh <distro>
+# Usage: ./tests/integration/scripts/run-tests.sh <distro> [module]
 # Example: ./tests/integration/scripts/run-tests.sh noetic
+# Example: ./tests/integration/scripts/run-tests.sh humble topics
 
 set -e
 cd "$(git rev-parse --show-toplevel)"
 
-DISTRO="${1:?Usage: $0 <melodic|noetic|humble|jazzy>}"
+if [ -z "${1:-}" ]; then
+    MODULES=$(ls tests/integration/test_*.py 2>/dev/null \
+        | sed 's|tests/integration/test_||;s|\.py||' \
+        | grep -v quick_detect \
+        | tr '\n' ', ' | sed 's/,$//')
+    echo "Usage: $0 <distro> [module]"
+    echo "Distros: melodic, noetic, humble, jazzy"
+    echo "Modules: $MODULES"
+    exit 1
+fi
+
+DISTRO="$1"
+MODULE="${2:-}"
 COMPOSE="tests/integration/docker-compose.yml"
 
 declare -A DOCKERFILES=(
@@ -54,7 +67,17 @@ uv run python tests/integration/test_quick_detect.py
 # Run pytest
 echo ""
 echo "--- Pytest ---"
-uv run pytest tests/integration/ -v --ros-distro "$DISTRO" --skip-compose
+if [ -n "$MODULE" ]; then
+    TEST_PATH="tests/integration/test_${MODULE}.py"
+    if [ ! -f "$TEST_PATH" ]; then
+        echo "Unknown module: $MODULE"
+        echo "Available: $(ls tests/integration/test_*.py | sed 's|tests/integration/test_||;s|\.py||' | tr '\n' ' ')"
+        exit 1
+    fi
+    uv run pytest "$TEST_PATH" -v --ros-distro "$DISTRO" --skip-compose
+else
+    uv run pytest tests/integration/ -v --ros-distro "$DISTRO" --skip-compose
+fi
 
 # Tear down
 echo ""

tests/integration/scripts/run-topics-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run topics integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-topics-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" topics

tests/integration/test_services.py

@@ -0,0 +1,112 @@
+"""Integration tests for service tools.
+
+These tests call the actual MCP tool functions (get_services, get_service_type,
+get_service_details, call_service) against a live rosbridge container.
+"""
+
+import time
+
+import pytest
+
+pytestmark = [pytest.mark.integration]
+
+
+class TestGetServices:
+    """Verify get_services MCP tool returns the service list."""
+
+    def test_returns_services(self, tools):
+        """get_services should return services and service_count."""
+        result = tools["get_services"]()
+        assert "services" in result
+        assert "service_count" in result
+        assert result["service_count"] > 0
+        assert result["service_count"] == len(result["services"])
+
+    def test_includes_rosapi_services(self, tools):
+        """rosapi services should be present (rosapi node is running)."""
+        result = tools["get_services"]()
+        services = result["services"]
+        assert any("nodes" in s for s in services), f"nodes service not in {services}"
+
+    def test_includes_turtlesim_services(self, tools):
+        """turtlesim services like /clear or /spawn should be present."""
+        result = tools["get_services"]()
+        services = result["services"]
+        assert any("spawn" in s for s in services), f"spawn not in {services}"
+
+
+class TestGetServiceType:
+    """Verify get_service_type MCP tool returns the service type."""
+
+    def test_known_service_type(self, tools):
+        """get_service_type for a turtlesim service should return a type string."""
+        result = tools["get_service_type"](service="/kill")
+        assert "type" in result
+        assert len(result["type"]) > 0
+
+    def test_empty_service_returns_error(self, tools):
+        """get_service_type with empty string should return error."""
+        result = tools["get_service_type"](service="")
+        assert "error" in result
+
+    def test_whitespace_service_returns_error(self, tools):
+        """get_service_type with whitespace should return error."""
+        result = tools["get_service_type"](service="   ")
+        assert "error" in result
+
+
+class TestGetServiceDetails:
+    """Verify get_service_details MCP tool returns full service definition."""
+
+    def test_spawn_details(self, tools):
+        """get_service_details for /spawn should return request/response fields."""
+        result = tools["get_service_details"](service="/spawn")
+        assert result["service"] == "/spawn"
+        assert len(result["type"]) > 0
+        assert "request" in result
+        assert "response" in result
+
+    def test_empty_service_returns_error(self, tools):
+        """get_service_details with empty string should return error."""
+        result = tools["get_service_details"](service="")
+        assert "error" in result
+
+    def test_whitespace_service_returns_error(self, tools):
+        """get_service_details with whitespace should return error."""
+        result = tools["get_service_details"](service="   ")
+        assert "error" in result
+
+
+class TestCallService:
+    """Verify call_service MCP tool can call a live service."""
+
+    def test_call_clear(self, tools):
+        """call_service for /clear should succeed (clears drawing traces)."""
+        type_result = tools["get_service_type"](service="/clear")
+        result = tools["call_service"](
+            service_name="/clear",
+            service_type=type_result["type"],
+            request={},
+        )
+        assert result["success"] is True
+
+    def test_call_spawn_turtle(self, tools):
+        """call_service for /spawn should create a new turtle."""
+        turtle_name = f"test_turtle_{int(time.time_ns())}"
+        type_result = tools["get_service_type"](service="/spawn")
+        spawn_type = type_result["type"]
+        result = tools["call_service"](
+            service_name="/spawn",
+            service_type=spawn_type,
+            request={"x": 3.0, "y": 3.0, "theta": 0.0, "name": turtle_name},
+        )
+        assert result["success"] is True
+
+    def test_call_nonexistent_service(self, tools):
+        """call_service for a nonexistent service should return an error."""
+        result = tools["call_service"](
+            service_name="/this_service_does_not_exist",
+            service_type="std_srvs/srv/Empty",
+            request={},
+        )
+        assert result["success"] is False