Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
|
Pulls: #442
|
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
@SuperJappie08 can you review this?
There was a problem hiding this comment.
Pull request overview
This PR fixes test failures in the rclc action client tests by addressing two key issues: replacing the deprecated rclcpp::spin_some(Node::SharedPtr) API with an executor-based approach, and fixing ROS 2 context initialization problems that occurred when running multiple tests sequentially.
Changes:
- Replaced deprecated
rclcpp::spin_some()with explicit executor usage - Added conditional
rclcpp::init()check to prevent multiple initialization failures - Ensured proper cleanup order for rclcpp resources before shutdown
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
i think this has been broke for certain time? but this fixes a couple of problems in my local environment.
SuperJappie08
left a comment
SuperJappie08
left a comment
There was a problem hiding this comment.
@fujitatomoya, I have some concerns about the rclcpp::Context check.
(See the comment)
However, I don't have much experience with complex testing for rclcpp (and rclc). So I might be a bit too cautious.
| if (!rclcpp::ok()) { | ||
| rclcpp::init(0, NULL); | ||
| } |
There was a problem hiding this comment.
This check worries me, as to my understanding this would imply that another section has initialized the rclcpp context, which in turn would/should tear it down as well.
I'm afraid this could cause issues in two cases:
- The other test shuts the context down earlier, which would stop this executor.
- The order of the tests change, causing an error in the other test.
How are multi-context cases handled when testing rclcpp as I would imagine they have to deal with the same issue?
There was a problem hiding this comment.
I did a quick search, and the other rclcpp::Context lives in test_action_server.cpp
2 participants
Description
this PR addresses 2 issues.
rclcpp::spin_some(Node::SharedPtr)is deprecated in favor of using an executor directly. see https://ci.ros2.org/job/ci_linux-aarch64/20707/clang-tidy/rclcpp::init()was called andrclcpp::init()was being called unconditionally, causing issues when running multiple tests.Fixes # (issue)
Is this user-facing behavior change?
No
Did you use Generative AI?
Copilot Claude Sonnet 4.5
Additional Information