refactor(topics): migrate to rosapi_service()/rosapi_type(), add tool tests (step3)#281
refactor(topics): migrate to rosapi_service()/rosapi_type(), add tool tests (step3)#281
Conversation
r-johnv
left a comment
r-johnv
left a comment
There was a problem hiding this comment.
Review comments on error handling and test correctness.
| "op": "call_service", | ||
| "service": "/rosapi/topics", | ||
| "type": "rosapi/Topics", | ||
| "service": rosapi_service("topics"), |
There was a problem hiding this comment.
This PR inherits the same DetectionError crash path flagged on #280. If detection fails silently in connect_to_robot, all 6 topic tool functions will crash with an unhandled DetectionError when they call rosapi_type(). This is a regression from the hardcoded strings that always worked regardless of detection state.
See the detailed comment on #280: #280 (review)
This will be resolved once the fix lands in step 2 — no action needed here, just flagging the dependency.
There was a problem hiding this comment.
Agreed. Step2 fixes should be already in this branch via rebase. get_type() should now fall back to ROS 2 format and connect_to_robot surfaces a warning.
| assert "error" in result | ||
|
|
||
|
|
||
| class TestPublishOnce: |
There was a problem hiding this comment.
subscribe_for_duration returns a ToolResult object (line 573 of topics.py), not a dict. These assertions will raise TypeError at runtime:
assert "messages" in result
assert result["collected_count"] > 0
assert result["topic"] == "/turtle1/pose"ToolResult doesn't support in or [] for string keys. The test needs to parse the summary from result.content[0].text via json.loads().
There was a problem hiding this comment.
Full disclosure, this assertion one was Claude's catch and recommendation. I figured that I would put it down anyway.
There was a problem hiding this comment.
Fixed. test now should partse the ToolResult summary.
Note: it seems only the happy-path test needed updating; the error-path test was already correct since early returns are plain dicts.
Fair enough,. good catch either wya!
Tests now call get_nodes() and get_node_details() tool functions directly. Includes negative tests for empty/whitespace node names. No skips — empty name is caught by the tool's own validation before reaching rosapi_node.
… unknown Address PR #280 review feedback: - connection.py: surface detection failure as warning in return value - rosapi_types.py: get_type() falls back to ROS 2 format instead of crashing - rosapi_types.py: bump distro detection failure log to warning level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
run-tests.sh now accepts optional second arg: distro + module e.g. ./run-tests.sh humble topics Per-module wrappers: run-connection-tests.sh, run-detect_version-tests.sh, run-nodes-tests.sh, run-topics-tests.sh
- Add tests for subscribe_for_duration, publish_once, publish_for_durations - Add end-to-end test: publish cmd_vel, verify turtle moves via pose - Extract _get_type() helper to guard against missing type responses - Relax published_count assertion (rosbridge timing varies by distro) - All 8 topic tools now tested (20 tests)
2 participants
Summary
Step 3 of 6 — targets
feature/step2(#280)/rosapi/strings intopics.py(6 service paths + 6 type strings) withrosapi_service()/rosapi_type()toolsfixture--helpforrun-tests.shChanges
ros_mcp/tools/topics.py— migrate all hardcoded paths/types (+13 -12)tests/integration/test_topics.py— 11 tests:get_topics(3),get_topic_type(2),get_topic_details(2),get_message_details(2),subscribe_once(2)tests/integration/scripts/— per-module wrapper scripts, module filter inrun-tests.shTesting
11 topic tests + all previous tests pass across all 4 distros (melodic, noetic, humble, jazzy), zero skips