Add tests for launch_utils of controller manager (#2147) #2768

robkwan · 2025-11-02T07:03:37Z

Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
Give your PR a descriptive title. Add a short summary, if required.
Make sure the pipeline is green.
Don’t be afraid to request reviews from maintainers.
New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

Fork the repository.
Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
Commit to your fork using clear commit messages.
Send a pull request, answering any default questions in the pull request interface.
Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

…er manager (ros-controls#2147)

…os-controls#2147)

christophfroehlich

The tests you added are some sort of unit tests, checking if the returned objects are of expected type/size. But it does not check if the returned actions work as expected (let's call this integration test).

Can you please extract the new tests from this file (new tests are not testing ros2_control_node at all, and don't need launch_ros or launch_testing) in a launch_utils_unit_test file, and add distinct files for the integration tests of the three methods of the launch_utils? You could also extend the existing test_ros2_control_node_launch.py by adding more controllers, and spawn different combinations with the different methods at once.

Please also install pre-commit and fix the failing tests by running pre-commit run --all

codecov · 2025-11-02T12:06:02Z

Codecov Report

❌ Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.65%. Comparing base (a7e0b3a) to head (05e3d32).

Files with missing lines	Patch %	Lines
...ller_manager/test/test_ros2_control_node_launch.py	88.23%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2768      +/-   ##
==========================================
+ Coverage   89.62%   89.65%   +0.03%     
==========================================
  Files         152      152              
  Lines       17637    17670      +33     
  Branches     1448     1451       +3     
==========================================
+ Hits        15807    15842      +35     
+ Misses       1246     1241       -5     
- Partials      584      587       +3

Flag	Coverage Δ
unittests	`89.65% <88.23%> (+0.03%)`	⬆️

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

Files with missing lines	Coverage Δ
...ller_manager/test/test_ros2_control_node_launch.py	`94.66% <88.23%> (-5.34%)`	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

robkwan · 2025-11-03T06:22:48Z

Thanks for the comment, @christophfroehlich! Let me try again then...

)

robkwan · 2025-11-09T19:50:53Z

Hi @christophfroehlich, I hope that it will work ok this time please. I first did the pre-commit ok in my jazzy branch but once I merged back to the master branch, I forgot to redo the pre-commit and so I had another commit to fix that again! Thanks!

christophfroehlich

Thanks for the work.
Without detailed review, I have two requests:

We have so many files already in the test folder. Please move the new files in a subfolder of test (call it test_launch_utils or similar), maybe with a dedicated cmake file, which is just included from the main one via add_subdirectory(test/test_launch_utils).
Follow the best practices for launch files, see #2767.

…ls#2147 PR(ros-controls#2768)

robkwan · 2025-11-16T02:16:56Z

Hi @christophfroehlich, thanks for the feedback. And I just tried to make the changes and committed again [ef44894]. Hope that it will be ok this time. By the way, I will be out of town from next weekend and so I won't be able to work on it till I am back early next year. So I hope that I can help to close this before I depart then. Thanks!

christophfroehlich

I reviewed the first python file, if you agree please apply the changes also to the others.

christophfroehlich · 2025-11-16T20:09:58Z

controller_manager/CMakeLists.txt

+  find_package(launch_testing_ament_cmake REQUIRED)
+  find_package(rclpy REQUIRED)


Suggested change

find_package(launch_testing_ament_cmake REQUIRED)

find_package(rclpy REQUIRED)

As this is also included in the cmake file in the subdir

christophfroehlich · 2025-11-16T20:11:40Z

controller_manager/test/test_launch_utils/CMakeLists.txt

+ament_add_pytest_test(test_launch_utils_integration_list
+  test_launch_utils_integration_list.py
+  TIMEOUT 60
+)
+ament_add_pytest_test(test_launch_utils_integration_dict
+  test_launch_utils_integration_dict.py
+  TIMEOUT 60
+)
+ament_add_pytest_test(test_launch_utils_integration_load
+  test_launch_utils_integration_load.py
+  TIMEOUT 60
+)


Suggested change

ament_add_pytest_test(test_launch_utils_integration_list

test_launch_utils_integration_list.py

TIMEOUT 60

)

ament_add_pytest_test(test_launch_utils_integration_dict

test_launch_utils_integration_dict.py

TIMEOUT 60

)

ament_add_pytest_test(test_launch_utils_integration_load

test_launch_utils_integration_load.py

TIMEOUT 60

)

ament_add_pytest_test(test_launch_utils_integration_list

test_launch_utils_integration_list.py

)

ament_add_pytest_test(test_launch_utils_integration_dict

test_launch_utils_integration_dict.py

)

ament_add_pytest_test(test_launch_utils_integration_load

test_launch_utils_integration_load.py

)

nitpick: Isn't 60 the default value?

christophfroehlich · 2025-11-16T20:14:42Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+
+        print("[POST-SHUTDOWN] ? All processes exited as expected")
+
+    def test_cleanup_temp_files(self, temp_dir):


can you explain why this is necessary? We don't do any cleanups in other pytests.

christophfroehlich · 2025-11-16T20:15:57Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+    def test_controllers_loaded(self, proc_info, controller_list):
+        """Test that controllers were loaded (poll until they appear)."""
+        node = rclpy.create_node("test_controller_query_node")
+
+        try:
+            from controller_manager_msgs.srv import ListControllers
+
+            client = node.create_client(ListControllers, "/controller_manager/list_controllers")
+
+            print("\n[TEST] Waiting for controller_manager service...")
+            wait_for_svc_timeout = 30.0
+            if not client.wait_for_service(timeout_sec=wait_for_svc_timeout):
+                process_names = proc_info.process_names()
+                self.fail(
+                    f"Controller manager service not available after {wait_for_svc_timeout}s.\n"
+                    f"Active processes: {process_names}"
+                )
+
+            # Poll for controllers to be registered
+            print("[TEST] Service available, polling for controllers (timeout 30s)...")
+            deadline = node.get_clock().now() + rclpy.duration.Duration(seconds=30.0)
+            seen = []
+            while node.get_clock().now() < deadline:
+                req = ListControllers.Request()
+                fut = client.call_async(req)
+                rclpy.spin_until_future_complete(node, fut, timeout_sec=2.0)
+                if fut.done() and fut.result() is not None:
+                    response = fut.result()
+                    seen = [c.name for c in response.controller]
+                    if all(ctrl in seen for ctrl in controller_list):
+                        print(f"[TEST] Loaded controllers: {seen}")
+                        break
+                # small sleep to avoid tight-loop
+                time.sleep(0.2)
+            else:
+                # timeout expired
+                self.fail(
+                    f"Timeout waiting for controllers to be loaded. "
+                    f"Expected: {controller_list}, saw: {seen}"
+                )
+
+            # Final assert (defensive)
+            for controller in controller_list:
+                self.assertIn(
+                    controller,
+                    seen,
+                    f"Controller '{controller}' was not loaded. Available: {seen}",
+                )
+
+            print(f"[TEST] ? All {len(controller_list)} controllers loaded successfully")
+
+        finally:
+            node.destroy_node()


Why not using this?

Suggested change

def test_controllers_loaded(self, proc_info, controller_list):

"""Test that controllers were loaded (poll until they appear)."""

node = rclpy.create_node("test_controller_query_node")

try:

from controller_manager_msgs.srv import ListControllers

client = node.create_client(ListControllers, "/controller_manager/list_controllers")

print("\n[TEST] Waiting for controller_manager service...")

wait_for_svc_timeout = 30.0

if not client.wait_for_service(timeout_sec=wait_for_svc_timeout):

process_names = proc_info.process_names()

self.fail(

f"Controller manager service not available after {wait_for_svc_timeout}s.\n"

f"Active processes: {process_names}"

)

# Poll for controllers to be registered

print("[TEST] Service available, polling for controllers (timeout 30s)...")

deadline = node.get_clock().now() + rclpy.duration.Duration(seconds=30.0)

seen = []

while node.get_clock().now() < deadline:

req = ListControllers.Request()

fut = client.call_async(req)

rclpy.spin_until_future_complete(node, fut, timeout_sec=2.0)

if fut.done() and fut.result() is not None:

response = fut.result()

seen = [c.name for c in response.controller]

if all(ctrl in seen for ctrl in controller_list):

print(f"[TEST] Loaded controllers: {seen}")

break

# small sleep to avoid tight-loop

time.sleep(0.2)

else:

# timeout expired

self.fail(

f"Timeout waiting for controllers to be loaded. "

f"Expected: {controller_list}, saw: {seen}"

)

# Final assert (defensive)

for controller in controller_list:

self.assertIn(

controller,

seen,

f"Controller '{controller}' was not loaded. Available: {seen}",

)

print(f"[TEST] ? All {len(controller_list)} controllers loaded successfully")

finally:

node.destroy_node()

def test_controllers_start(self):

cnames = ["ctrl_with_parameters_and_type"]

check_controllers_running(self.node, cnames, state="active")

Needs
from controller_manager.test_utils import check_controllers_running

If you see an improvement to check_controllers_running, please open a PR for it! :)

christophfroehlich · 2025-11-16T20:17:56Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+        print("\n[POST-SHUTDOWN] Process exit codes:")
+        for process_name in proc_info.process_names():
+            info = proc_info[process_name]
+            print(f"  {process_name}: {info.returncode}")
+
+        for process_name in proc_info.process_names():
+            info = proc_info[process_name]
+
+            if "ros2_control_node" in process_name:
+                self.assertEqual(
+                    info.returncode, 0, f"{process_name} exited with {info.returncode}"
+                )
+            elif "spawner" in process_name:
+                # Spawner should complete successfully (0) or be terminated
+                self.assertIn(
+                    info.returncode,
+                    [0, -2, -15],
+                    f"Spawner {process_name} exited with {info.returncode}",
+                )
+            else:
+                self.assertIn(
+                    info.returncode, [0, -2, -15], f"{process_name} exited with {info.returncode}"
+                )
+
+        print("[POST-SHUTDOWN] ? All processes exited as expected")


Is this verbose test case necessary? Wouldn't this one be also fine?

Suggested change

print("\n[POST-SHUTDOWN] Process exit codes:")

for process_name in proc_info.process_names():

info = proc_info[process_name]

print(f" {process_name}: {info.returncode}")

for process_name in proc_info.process_names():

info = proc_info[process_name]

if "ros2_control_node" in process_name:

self.assertEqual(

info.returncode, 0, f"{process_name} exited with {info.returncode}"

)

elif "spawner" in process_name:

# Spawner should complete successfully (0) or be terminated

self.assertIn(

info.returncode,

[0, -2, -15],

f"Spawner {process_name} exited with {info.returncode}",

)

else:

self.assertIn(

info.returncode, [0, -2, -15], f"{process_name} exited with {info.returncode}"

)

print("[POST-SHUTDOWN] ? All processes exited as expected")

launch_testing.asserts.assertExitCodes(proc_info)

christophfroehlich · 2025-11-16T20:18:56Z

controller_manager/test/test_ros2_control_node_launch.py

Are these changes intentional?

This is just the clean-up the old changes I made but they should be moved to the subfolder /test_launch_utils now. Thanks.

christophfroehlich · 2025-11-16T20:22:01Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+
+    # URDF path (pathlib version, no xacro)
+    urdf = (
+        Path(get_package_share_directory("ros2_control_test_assets"))


Suggested change

Path(get_package_share_directory("ros2_control_test_assets"))

PathSubstitution(FindPackageShare("ros2_control_test_assets"))

From

from launch.substitutions import PathSubstitution from launch_ros.substitutions import FindPackageShare

would be the ROS-ish way.

christophfroehlich · 2025-11-16T20:22:18Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+
+    # Path to combined YAML
+    robot_controllers = (
+        Path(get_package_prefix("controller_manager"))


Suggested change

Path(get_package_prefix("controller_manager"))

PathSubstitution(FindPackagePrefix("controller_manager"))

from

from launch_ros.substitutions import FindPackagePrefix

christophfroehlich · 2025-11-16T20:38:47Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+    robot_state_pub_node = launch_ros.actions.Node(
+        package="robot_state_publisher",
+        executable="robot_state_publisher",
+        output="both",
+        parameters=[robot_description],
+    )
+
+    # ===== START CONTROLLER MANAGER (ros2_control_node) =====
+    control_node = launch_ros.actions.Node(
+        package="controller_manager",
+        executable="ros2_control_node",
+        parameters=[str(robot_controllers)],  # Use the combined config file
+        output="both",
+    )
+
+    # The dictionary keys are the controller names to be spawned/started.
+    # Values can be empty lists since config is provided via the main YAML.
+    ctrl_dict = {
+        "joint_state_broadcaster": [str(robot_controllers)],
+        "controller1": [str(robot_controllers)],
+        "controller2": [str(robot_controllers)],
+    }
+    controller_list = list(ctrl_dict.keys())
+
+    # ===== GENERATE SPAWNER LAUNCH DESCRIPTION =====
+    print(f"Spawning controllers: {controller_list}")
+
+    # Correct function name and call
+    spawner_ld = generate_controllers_spawner_launch_description_from_dict(
+        controller_info_dict=ctrl_dict,
+    )
+
+    # ===== CREATE LAUNCH DESCRIPTION =====
+    ld = LaunchDescription(
+        [robot_state_pub_node, control_node, ReadyToTest()] + spawner_ld.entities
+    )
+
+    # Return tuple with launch description and test context
+    return ld, {


Suggested change

robot_state_pub_node = launch_ros.actions.Node(

package="robot_state_publisher",

executable="robot_state_publisher",

output="both",

parameters=[robot_description],

)

# ===== START CONTROLLER MANAGER (ros2_control_node) =====

control_node = launch_ros.actions.Node(

package="controller_manager",

executable="ros2_control_node",

parameters=[str(robot_controllers)], # Use the combined config file

output="both",

)

# The dictionary keys are the controller names to be spawned/started.

# Values can be empty lists since config is provided via the main YAML.

ctrl_dict = {

"joint_state_broadcaster": [str(robot_controllers)],

"controller1": [str(robot_controllers)],

"controller2": [str(robot_controllers)],

}

controller_list = list(ctrl_dict.keys())

# ===== GENERATE SPAWNER LAUNCH DESCRIPTION =====

print(f"Spawning controllers: {controller_list}")

# Correct function name and call

spawner_ld = generate_controllers_spawner_launch_description_from_dict(

controller_info_dict=ctrl_dict,

)

# ===== CREATE LAUNCH DESCRIPTION =====

ld = LaunchDescription(

[robot_state_pub_node, control_node, ReadyToTest()] + spawner_ld.entities

)

# Return tuple with launch description and test context

return ld, {

# The dictionary keys are the controller names to be spawned/started.

# Values can be empty lists since config is provided via the main YAML.

ctrl_dict = {

"joint_state_broadcaster": [str(robot_controllers)],

"controller1": [str(robot_controllers)],

"controller2": [str(robot_controllers)],

}

controller_list = list(ctrl_dict.keys())

# ===== GENERATE SPAWNER LAUNCH DESCRIPTION =====

print(f"Spawning controllers: {controller_list}")

# Return tuple with launch description and test context

return LaunchDescription([

launch_ros.actions.Node(

package="robot_state_publisher",

executable="robot_state_publisher",

output="both",

parameters=[robot_description],

),

launch_ros.actions.Node(

package="controller_manager",

executable="ros2_control_node",

parameters=[str(robot_controllers)], # Use the combined config file

output="both",

),

generate_controllers_spawner_launch_description_from_dict(

controller_info_dict=ctrl_dict,

),

ReadyToTest()]), {

Do we need the variables, can't we just directly return them?

christophfroehlich · 2025-11-16T20:39:51Z

controller_manager/test/test_launch_utils/test_launch_utils_integration_dict.py

+    return ld, {
+        "controller_list": controller_list,  # Key name updated to match the test function
+        "robot_controllers": robot_controllers,
+        "urdf_file": urdf,
+        "temp_dir": temp_dir,
+    }


Suggested change

return ld, {

"controller_list": controller_list, # Key name updated to match the test function

"robot_controllers": robot_controllers,

"urdf_file": urdf,

"temp_dir": temp_dir,

}

return ld

Why returning this? (I'm not very familiar with pytest, just following the ROS integration tests tutorials).

Thanks @christophfroehlich for the detailed review. I will try to look into it in the next two days.

…ols#2833)

…ntrols#2817) --------- Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com> Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com> Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>

robkwan · 2025-11-21T10:22:38Z

Hi @christophfroehlich, after changing to use the PathSubstitution approach, I found that there're more changes involved or it would fail for the difference between path and string as I could see from the colcon test-result --verbose. Therefore, I hope that it is ok this time. If more change is needed and it is not urgent, I can resume after I come back from my trip in early Jan. Thanks.

christophfroehlich

There is some issue with the git history here. I suggest that you

interactive rebase from 27e3b68
drop all commits which aren't yours
merge origin/master into your fork
force push

mergify · 2025-11-23T18:08:24Z

This pull request is in conflict. Could you fix it @robkwan?

robkwan · 2025-11-24T23:11:07Z

Hi @christophfroehlich , thanks for the comment! I will try to do as suggested after I will be back home after new year then...Thanks.

robkwan added 3 commits November 1, 2025 23:56

Merge back from jazzy branch - add tests for launch_utils of controll…

e7aa06e

…er manager (ros-controls#2147)

Fix missing line to Add tests for launch_utils of controller manager (r…

8e7d6bc

…os-controls#2147)

Fix missing line to Add tests for launch_utils of controller manager (r…

05e3d32

…os-controls#2147)

github-actions bot requested review from ARK3r, aprotyas, christophfroehlich, harderthan, mcbed, saikishor and xguay November 2, 2025 07:03

christophfroehlich mentioned this pull request Nov 2, 2025

Jazzy -New add tests for launch_utils of controller manager (#2147) #2749

Closed

6 tasks

christophfroehlich requested changes Nov 2, 2025

View reviewed changes

robkwan added 2 commits November 9, 2025 11:34

Add more tests for launch_utils of controller manager (ros-controls#2147

52bcf35

)

Fixed pre-commit error in master branch for ros-controls#2147

d161825

Merge branch 'master' into master

645f32b

christophfroehlich requested changes Nov 14, 2025

View reviewed changes

robkwan added a commit to robkwan/ros2_control that referenced this pull request Nov 16, 2025

Changes - Add tests for launch_utils of controller manager ros-contro…

27e3b68

…ls#2147 PR(ros-controls#2768)

jordan-palacios and others added 3 commits November 15, 2025 18:09

Improving differential_transmission configure checks (ros-controls#2812)

b36e7ea

Use kilted branch for ros2_controllers (ros-controls#2813)

bae4788

Changes - Add tests for launch_utils of controller manager ros-contro…

ef44894

…ls#2147 PR(ros-controls#2768)

Merge branch 'master' into master

65f8a84

christophfroehlich requested changes Nov 16, 2025

View reviewed changes

Noel215 and others added 4 commits November 21, 2025 02:16

Use tinyxml2 package instead of deprecated tinyxml2_vendor (ros-contr…

e5768ba

…ols#2833)

Avoid C++20 structured binding capture (ros-controls#2832)

70a8eca

Fix dependencies of controller_manager (ros-controls#2836)

f5c420f

Use fmt for correct int64_t format specifier across platforms (ros-co…

b786d53

…ntrols#2817) --------- Signed-off-by: Dhruv Patel <dhruvpatel2991998@gmail.com> Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com> Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>

Modify tests for launch_utils of controller manager (ros-controls#2147)

d078b5d

christophfroehlich requested changes Nov 23, 2025

View reviewed changes

		find_package(launch_testing_ament_cmake REQUIRED)
		find_package(rclpy REQUIRED)


		print("[POST-SHUTDOWN] ? All processes exited as expected")

		def test_cleanup_temp_files(self, temp_dir):

	Path(get_package_share_directory("ros2_control_test_assets"))
	PathSubstitution(FindPackageShare("ros2_control_test_assets"))

	Path(get_package_prefix("controller_manager"))
	PathSubstitution(FindPackagePrefix("controller_manager"))

Add tests for launch_utils of controller manager (#2147) #2768

Are you sure you want to change the base?

Add tests for launch_utils of controller manager (#2147) #2768

Conversation

robkwan commented Nov 2, 2025

Uh oh!

christophfroehlich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robkwan commented Nov 3, 2025

Uh oh!

robkwan commented Nov 9, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

robkwan commented Nov 16, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robkwan commented Nov 21, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Nov 23, 2025

Uh oh!

robkwan commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

christophfroehlich left a comment •

edited

Loading

codecov bot commented Nov 2, 2025 •

edited

Loading