Skip to content

Conversation

@Theonewhomadethings
Copy link
Contributor

Overview

This PR replaces uses of the deprecated rclcpp::spin_some(node) with rclcpp::executors::SingleThreadedExecutor in the realtime tools tests, addressing issue #411

What changed

Files changed are:

  • realtime_tools/test/realtime_server_goal_handle_tests.cpp
  • realtime_tools/test/realtime_publisher_tests.cpp

Notes for Reviewer

Happy to make any changes that are requested. Please also double check my approach with the spin_some() usage in the TEST(RealtimeServerGoalHandle, set_canceled) test.

…r::spin_some() in realtime_server_goal_handle_tests.cpp.

Refactor polling in ActionServerCallbacks::wait_for_handle, ActionClientCallbacks::wait_for_feedback, and send_goal() to create a SingleThreadedExecutor, add_node(node), and call spin_some(). In set_canceled, use a short-lived SingleThreadedExecutor inside the wait loop before spin_some() to process events without reusing an executor. This modernizes the tests for current rclcpp APIs and removes deprecation warnings with no change to test behavior.
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.32%. Comparing base (1e5fa50) to head (c7ec02b).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...e_tools/test/realtime_server_goal_handle_tests.cpp 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #448      +/-   ##
==========================================
+ Coverage   85.20%   85.32%   +0.11%     
==========================================
  Files          17       17              
  Lines        1386     1397      +11     
  Branches      132      132              
==========================================
+ Hits         1181     1192      +11     
  Misses        119      119              
  Partials       86       86              
Flag Coverage Δ
unittests 85.32% <87.50%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
realtime_tools/test/realtime_publisher_tests.cpp 92.85% <100.00%> (+0.54%) ⬆️
...e_tools/test/realtime_server_goal_handle_tests.cpp 92.80% <84.61%> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saikishor saikishor added backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted. labels Nov 4, 2025
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was thinking if the executor shouldn't be part of a test fixture instead of creating one all over the place. But I think that inside the tests this is also fine.

@christophfroehlich christophfroehlich merged commit 59ff73b into ros-controls:master Nov 4, 2025
19 checks passed
mergify bot pushed a commit that referenced this pull request Nov 4, 2025
mergify bot pushed a commit that referenced this pull request Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants