From 8d829799a353ff478b6c1cca2456798d2f09dbcb Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 4 Aug 2020 16:02:17 -0700 Subject: [PATCH 1/8] Fix race with launch context changes when loading composable nodes This bug was discovered when trying load composable nodes from a GroupAction. The ROS namespace (and presumably other remaps) pushed onto the context stack was popped after the LoadComposableNodes execute() function finished. But because the loading happens asynchronously, we need to make sure we get the necessary information from the context before execute() finishes. Signed-off-by: Jacob Perron --- .../actions/load_composable_nodes.py | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/launch_ros/launch_ros/actions/load_composable_nodes.py b/launch_ros/launch_ros/actions/load_composable_nodes.py index a80f83a0..378b26b4 100644 --- a/launch_ros/launch_ros/actions/load_composable_nodes.py +++ b/launch_ros/launch_ros/actions/load_composable_nodes.py @@ -82,13 +82,13 @@ def __init__( def _load_node( self, - composable_node_description: ComposableNode, + request: composition_interfaces.srv.LoadNode.Request, context: LaunchContext ) -> None: """ Load node synchronously. - :param composable_node_description: description of composable node to be loaded + :param request: service request to load a node :param context: current launch context """ while not self.__rclpy_load_node_client.wait_for_service(timeout_sec=1.0): @@ -99,7 +99,6 @@ def _load_node( ) ) return - request = get_composable_node_load_request(composable_node_description, context) response = self.__rclpy_load_node_client.call(request) node_name = response.full_node_name if response.full_node_name else request.node_name if response.success: @@ -125,7 +124,7 @@ def _load_node( def _load_in_sequence( self, - composable_node_descriptions: List[ComposableNode], + load_node_requests: List[composition_interfaces.srv.LoadNode.Request], context: LaunchContext ) -> None: """ @@ -134,13 +133,13 @@ def _load_in_sequence( :param composable_node_descriptions: descriptions of composable nodes to be loaded :param context: current launch context """ - next_composable_node_description = composable_node_descriptions[0] - composable_node_descriptions = composable_node_descriptions[1:] - self._load_node(next_composable_node_description, context) - if len(composable_node_descriptions) > 0: + next_load_node_request = load_node_requests[0] + load_node_requests = load_node_requests[1:] + self._load_node(next_load_node_request, context) + if len(load_node_requests) > 0: context.add_completion_future( context.asyncio_loop.run_in_executor( - None, self._load_in_sequence, composable_node_descriptions, context + None, self._load_in_sequence, load_node_requests, context ) ) @@ -169,9 +168,16 @@ def execute( ) ) + # Generate load requests before execute() exits to avoid race with context changing + # due to scope change (e.g. if loading nodes from within a GroupAction). + load_node_requests = [ + get_composable_node_load_request(node_description, context) + for node_description in self.__composable_node_descriptions + ] + context.add_completion_future( context.asyncio_loop.run_in_executor( - None, self._load_in_sequence, self.__composable_node_descriptions, context + None, self._load_in_sequence, load_node_requests, context ) ) From 719ced845de88e3efcc3cf3a7dbf1f197e65adf2 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Thu, 6 Aug 2020 17:48:19 -0700 Subject: [PATCH 2/8] Add regression tests for LoadComposableNode Signed-off-by: Jacob Perron --- .../actions/load_composable_nodes.py | 6 +- .../actions/test_load_composable_nodes.py | 148 ++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py diff --git a/launch_ros/launch_ros/actions/load_composable_nodes.py b/launch_ros/launch_ros/actions/load_composable_nodes.py index 378b26b4..33d20dcf 100644 --- a/launch_ros/launch_ros/actions/load_composable_nodes.py +++ b/launch_ros/launch_ros/actions/load_composable_nodes.py @@ -79,6 +79,8 @@ def __init__( self.__target_container = target_container self.__final_target_container_name = None # type: Optional[Text] self.__logger = launch.logging.get_logger(__name__) + # Useful for regression testing + self.__load_node_requests = None def _load_node( self, @@ -170,14 +172,14 @@ def execute( # Generate load requests before execute() exits to avoid race with context changing # due to scope change (e.g. if loading nodes from within a GroupAction). - load_node_requests = [ + self.__load_node_requests = [ get_composable_node_load_request(node_description, context) for node_description in self.__composable_node_descriptions ] context.add_completion_future( context.asyncio_loop.run_in_executor( - None, self._load_in_sequence, load_node_requests, context + None, self._load_in_sequence, self.__load_node_requests, context ) ) diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py new file mode 100644 index 00000000..39caa9fd --- /dev/null +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -0,0 +1,148 @@ +# Copyright 2020 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for the LoadComposableNodes Action.""" + +import asyncio + +from launch import LaunchDescription +from launch import LaunchService +from launch.actions import GroupAction +from launch_ros.actions import ComposableNodeContainer +from launch_ros.actions import LoadComposableNodes +from launch_ros.actions import PushRosNamespace +from launch_ros.actions import SetRemap +from launch_ros.descriptions import ComposableNode +from launch_ros.utilities import get_node_name_count + +import osrf_pycommon.process_utils + +TEST_CONTAINER_NAME = 'test_load_composable_nodes_container' +TEST_NODE_NAME = 'test_load_composable_nodes_node' + + +def _assert_launch_no_errors(actions, *, timeout_sec=1): + ld = LaunchDescription(actions) + ls = LaunchService(debug=True) + ls.include_launch_description(ld) + + loop = osrf_pycommon.process_utils.get_loop() + launch_task = loop.create_task(ls.run_async()) + loop.run_until_complete(asyncio.sleep(timeout_sec)) + if not launch_task.done(): + loop.create_task(ls.shutdown()) + loop.run_until_complete(launch_task) + assert 0 == launch_task.result() + return ls.context + + +def _create_node_container(*, parameters=None, remappings=None, namespace=''): + return ComposableNodeContainer( + package='rclcpp_components', + executable='component_container', + name=TEST_CONTAINER_NAME, + namespace=namespace, + output='screen', + parameters=parameters, + remappings=remappings, + ) + + +def _load_composable_node(*, parameters=None, remappings=None): + return LoadComposableNodes( + target_container=TEST_CONTAINER_NAME, + composable_node_descriptions=[ + ComposableNode( + package='composition', + plugin='composition::Listener', + name=TEST_NODE_NAME, + parameters=parameters, + remappings=remappings, + ) + ]) + + +def test_load_invalid_node(): + """Test loading an invalid node.""" + load_composable_nodes = LoadComposableNodes( + target_container=TEST_CONTAINER_NAME, + composable_node_descriptions=[ + ComposableNode( + package='nonexistent_package', + plugin='this_plugin_should_not_exist', + name=TEST_NODE_NAME, + ) + ]) + context = _assert_launch_no_errors([_create_node_container(), load_composable_nodes]) + + assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 0 + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + + +def test_load_node(): + """Test loading a node.""" + context = _assert_launch_no_errors([ + _create_node_container(), _load_composable_node() + ]) + + assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1 + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + + +def test_load_node_with_global_remaps_in_group(): + """Test loading a node with global remaps scoped to a group.""" + load_composable_nodes_action = _load_composable_node() + context = _assert_launch_no_errors([ + _create_node_container(), + GroupAction( + [ + SetRemap('chatter', 'new_topic_name'), + load_composable_nodes_action, + ], + scoped=True, + ), + ]) + + assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1 + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + + # Check the remaps in load service request + assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1 + request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0] + assert len(request.remap_rules) == 1 + assert request.remap_rules[0] == 'chatter:=new_topic_name' + + +def test_load_node_with_namespace_in_group(): + """Test loading a node with namespace scoped to a group.""" + namespace = '/foo' + load_composable_nodes_action = _load_composable_node() + context = _assert_launch_no_errors([ + _create_node_container(), + GroupAction( + [ + PushRosNamespace(namespace), + load_composable_nodes_action, + ], + scoped=True, + ), + ]) + + assert get_node_name_count(context, f'{namespace}/{TEST_NODE_NAME}') == 1 + assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + + # Check the namespace in load service request + assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1 + request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0] + assert request.node_namespace == f'{namespace}' From a12906b14e6bda5db44e3fcf3bd12e8e616e2f46 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Fri, 7 Aug 2020 11:21:27 -0700 Subject: [PATCH 3/8] Parameterize target container in text fixture Signed-off-by: Jacob Perron --- .../actions/test_load_composable_nodes.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py index 39caa9fd..e50f54e7 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -59,9 +59,14 @@ def _create_node_container(*, parameters=None, remappings=None, namespace=''): ) -def _load_composable_node(*, parameters=None, remappings=None): +def _load_composable_node( + *, + parameters=None, + remappings=None, + target_container=TEST_CONTAINER_NAME +): return LoadComposableNodes( - target_container=TEST_CONTAINER_NAME, + target_container=target_container, composable_node_descriptions=[ ComposableNode( package='composition', From 5d9dcf380a1cfd1dab3f283657c598f55dc55105 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 10 Aug 2020 12:10:45 -0700 Subject: [PATCH 4/8] Refactor tests to use mock component container Signed-off-by: Jacob Perron --- .../actions/test_load_composable_nodes.py | 252 ++++++++++++++---- 1 file changed, 194 insertions(+), 58 deletions(-) diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py index e50f54e7..88205ef1 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -15,11 +15,13 @@ """Tests for the LoadComposableNodes Action.""" import asyncio +import threading + +from composition_interfaces.srv import LoadNode from launch import LaunchDescription from launch import LaunchService from launch.actions import GroupAction -from launch_ros.actions import ComposableNodeContainer from launch_ros.actions import LoadComposableNodes from launch_ros.actions import PushRosNamespace from launch_ros.actions import SetRemap @@ -28,10 +30,58 @@ import osrf_pycommon.process_utils -TEST_CONTAINER_NAME = 'test_load_composable_nodes_container' +import pytest + +from rcl_interfaces.msg import ParameterType + +import rclpy +import rclpy.context +import rclpy.executors +import rclpy.node + +TEST_CONTAINER_NAME = 'mock_component_container' TEST_NODE_NAME = 'test_load_composable_nodes_node' +class MockComponentContainer(rclpy.node.Node): + + def __init__(self): + # List of LoadNode requests received + self.requests = [] + + self._context = rclpy.context.Context() + rclpy.init(context=self._context) + + super().__init__(TEST_CONTAINER_NAME, context=self._context) + + self.load_node_service = self.create_service( + LoadNode, + '~/_container/load_node', + self.load_node_callback + ) + + self._executor = rclpy.executors.SingleThreadedExecutor(context=self._context) + + # Start spinning in a thread + self._thread = threading.Thread( + target=rclpy.spin, + args=(self, self._executor), + daemon=True + ) + self._thread.start() + + def load_node_callback(self, request, response): + self.requests.append(request) + response.success = True + response.full_node_name = f'{request.node_namespace}/{request.node_name}' + response.unique_id = len(self.requests) + return response + + def shutdown(self): + rclpy.shutdown(context=self._context) + self._thread.join() + + def _assert_launch_no_errors(actions, *, timeout_sec=1): ld = LaunchDescription(actions) ls = LaunchService(debug=True) @@ -47,107 +97,193 @@ def _assert_launch_no_errors(actions, *, timeout_sec=1): return ls.context -def _create_node_container(*, parameters=None, remappings=None, namespace=''): - return ComposableNodeContainer( - package='rclcpp_components', - executable='component_container', - name=TEST_CONTAINER_NAME, - namespace=namespace, - output='screen', - parameters=parameters, - remappings=remappings, - ) - - def _load_composable_node( *, + package, + plugin, + name, + namespace='', parameters=None, remappings=None, - target_container=TEST_CONTAINER_NAME + target_container=f'/{TEST_CONTAINER_NAME}' ): return LoadComposableNodes( target_container=target_container, composable_node_descriptions=[ ComposableNode( - package='composition', - plugin='composition::Listener', - name=TEST_NODE_NAME, + package=package, + plugin=plugin, + name=name, + namespace=namespace, parameters=parameters, remappings=remappings, ) ]) -def test_load_invalid_node(): - """Test loading an invalid node.""" - load_composable_nodes = LoadComposableNodes( - target_container=TEST_CONTAINER_NAME, - composable_node_descriptions=[ - ComposableNode( - package='nonexistent_package', - plugin='this_plugin_should_not_exist', - name=TEST_NODE_NAME, - ) - ]) - context = _assert_launch_no_errors([_create_node_container(), load_composable_nodes]) - - assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 0 - assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 +@pytest.fixture +def mock_component_container(): + container = MockComponentContainer() + yield container + container.shutdown() -def test_load_node(): +def test_load_node(mock_component_container): """Test loading a node.""" context = _assert_launch_no_errors([ - _create_node_container(), _load_composable_node() + _load_composable_node( + package='foo_package', + plugin='bar_plugin', + name='test_node_name', + namespace='test_node_namespace' + ) ]) - assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1 - assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + # Check that launch is aware of loaded component + assert get_node_name_count(context, '/test_node_namespace/test_node_name') == 1 + + # Check that container recieved correct request + assert len(mock_component_container.requests) == 1 + request = mock_component_container.requests[0] + assert request.package_name == 'foo_package' + assert request.plugin_name == 'bar_plugin' + assert request.node_name == 'test_node_name' + assert request.node_namespace == '/test_node_namespace' + assert len(request.remap_rules) == 0 + assert len(request.parameters) == 0 + assert len(request.extra_arguments) == 0 + + +def test_load_node_with_remaps(mock_component_container): + """Test loading a node with remappings.""" + context = _assert_launch_no_errors([ + _load_composable_node( + package='foo_package', + plugin='bar_plugin', + name='test_node_name', + namespace='test_node_namespace', + remappings=[ + ('test_topic1', 'test_remap_topic1'), + ('test/topic/two', 'test/remap_topic2') + ] + ) + ]) + + # Check that launch is aware of loaded component + assert get_node_name_count(context, '/test_node_namespace/test_node_name') == 1 + + # Check that container recieved correct request + assert len(mock_component_container.requests) == 1 + request = mock_component_container.requests[0] + assert request.package_name == 'foo_package' + assert request.plugin_name == 'bar_plugin' + assert request.node_name == 'test_node_name' + assert request.node_namespace == '/test_node_namespace' + assert len(request.remap_rules) == 2 + assert request.remap_rules[0] == 'test_topic1:=test_remap_topic1' + assert request.remap_rules[1] == 'test/topic/two:=test/remap_topic2' + assert len(request.parameters) == 0 + assert len(request.extra_arguments) == 0 + + +def test_load_node_with_params(mock_component_container): + """Test loading a node with parameters.""" + context = _assert_launch_no_errors([ + _load_composable_node( + package='foo_package', + plugin='bar_plugin', + name='test_node_name', + namespace='test_node_namespace', + parameters=[{ + 'test_param1': 'test_value_param1', + 'test.param2': '42.0', + }] + ) + ]) + + # Check that launch is aware of loaded component + assert get_node_name_count(context, '/test_node_namespace/test_node_name') == 1 + + # Check that container recieved correct request + assert len(mock_component_container.requests) == 1 + request = mock_component_container.requests[0] + assert request.package_name == 'foo_package' + assert request.plugin_name == 'bar_plugin' + assert request.node_name == 'test_node_name' + assert request.node_namespace == '/test_node_namespace' + assert len(request.remap_rules) == 0 + assert len(request.parameters) == 2 + assert request.parameters[0].name == 'test_param1' + assert request.parameters[0].value.type == ParameterType.PARAMETER_STRING + assert request.parameters[0].value.string_value == 'test_value_param1' + assert request.parameters[1].name == 'test.param2' + # TODO(jacobperron): I would expect this to be a double value, but we can only pass strings + # assert request.parameters[1].value.type == ParameterType.PARAMETER_DOUBLE + # assert request.parameters[1].value.double_value == 42.0 + assert request.parameters[1].value.string_value == '42.0' + assert len(request.extra_arguments) == 0 -def test_load_node_with_global_remaps_in_group(): +def test_load_node_with_global_remaps_in_group(mock_component_container): """Test loading a node with global remaps scoped to a group.""" - load_composable_nodes_action = _load_composable_node() context = _assert_launch_no_errors([ - _create_node_container(), GroupAction( [ SetRemap('chatter', 'new_topic_name'), - load_composable_nodes_action, + _load_composable_node( + package='foo_package', + plugin='bar_plugin', + name='test_node_name', + namespace='test_node_namespace' + ), ], scoped=True, ), ]) - assert get_node_name_count(context, f'/{TEST_NODE_NAME}') == 1 - assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + # Check that launch is aware of loaded component + assert get_node_name_count(context, '/test_node_namespace/test_node_name') == 1 - # Check the remaps in load service request - assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1 - request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0] + # Check that container recieved correct request + assert len(mock_component_container.requests) == 1 + request = mock_component_container.requests[0] + assert request.package_name == 'foo_package' + assert request.plugin_name == 'bar_plugin' + assert request.node_name == 'test_node_name' + assert request.node_namespace == '/test_node_namespace' assert len(request.remap_rules) == 1 assert request.remap_rules[0] == 'chatter:=new_topic_name' + assert len(request.parameters) == 0 + assert len(request.extra_arguments) == 0 -def test_load_node_with_namespace_in_group(): +def test_load_node_with_namespace_in_group(mock_component_container): """Test loading a node with namespace scoped to a group.""" - namespace = '/foo' - load_composable_nodes_action = _load_composable_node() context = _assert_launch_no_errors([ - _create_node_container(), GroupAction( [ - PushRosNamespace(namespace), - load_composable_nodes_action, + PushRosNamespace('foo'), + _load_composable_node( + package='foo_package', + plugin='bar_plugin', + name='test_node_name', + namespace='test_node_namespace' + ), ], scoped=True, ), ]) - assert get_node_name_count(context, f'{namespace}/{TEST_NODE_NAME}') == 1 - assert get_node_name_count(context, f'/{TEST_CONTAINER_NAME}') == 1 + # Check that launch is aware of loaded component + assert get_node_name_count(context, '/foo/test_node_namespace/test_node_name') == 1 - # Check the namespace in load service request - assert len(load_composable_nodes_action._LoadComposableNodes__load_node_requests) == 1 - request = load_composable_nodes_action._LoadComposableNodes__load_node_requests[0] - assert request.node_namespace == f'{namespace}' + # Check that container recieved correct request + assert len(mock_component_container.requests) == 1 + request = mock_component_container.requests[0] + assert request.package_name == 'foo_package' + assert request.plugin_name == 'bar_plugin' + assert request.node_name == 'test_node_name' + assert request.node_namespace == '/foo/test_node_namespace' + assert len(request.remap_rules) == 0 + assert len(request.parameters) == 0 + assert len(request.extra_arguments) == 0 From 074bdac63cf88ca7430a2caf292a4809830e1b7f Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 10 Aug 2020 12:18:41 -0700 Subject: [PATCH 5/8] Don't store requests This was only needed for the tests before the refactor. Signed-off-by: Jacob Perron --- launch_ros/launch_ros/actions/load_composable_nodes.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/launch_ros/launch_ros/actions/load_composable_nodes.py b/launch_ros/launch_ros/actions/load_composable_nodes.py index 33d20dcf..378b26b4 100644 --- a/launch_ros/launch_ros/actions/load_composable_nodes.py +++ b/launch_ros/launch_ros/actions/load_composable_nodes.py @@ -79,8 +79,6 @@ def __init__( self.__target_container = target_container self.__final_target_container_name = None # type: Optional[Text] self.__logger = launch.logging.get_logger(__name__) - # Useful for regression testing - self.__load_node_requests = None def _load_node( self, @@ -172,14 +170,14 @@ def execute( # Generate load requests before execute() exits to avoid race with context changing # due to scope change (e.g. if loading nodes from within a GroupAction). - self.__load_node_requests = [ + load_node_requests = [ get_composable_node_load_request(node_description, context) for node_description in self.__composable_node_descriptions ] context.add_completion_future( context.asyncio_loop.run_in_executor( - None, self._load_in_sequence, self.__load_node_requests, context + None, self._load_in_sequence, load_node_requests, context ) ) From d57bc8d2798bb40be90504721004f86a57c25f08 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 10 Aug 2020 19:36:12 -0700 Subject: [PATCH 6/8] Launch actions synchronously There's no need to run asynchronously and manually send a shutdown signal. Signed-off-by: Jacob Perron --- .../actions/test_load_composable_nodes.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py index 88205ef1..b6904d2d 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -14,7 +14,6 @@ """Tests for the LoadComposableNodes Action.""" -import asyncio import threading from composition_interfaces.srv import LoadNode @@ -28,8 +27,6 @@ from launch_ros.descriptions import ComposableNode from launch_ros.utilities import get_node_name_count -import osrf_pycommon.process_utils - import pytest from rcl_interfaces.msg import ParameterType @@ -82,18 +79,11 @@ def shutdown(self): self._thread.join() -def _assert_launch_no_errors(actions, *, timeout_sec=1): +def _assert_launch_no_errors(actions): ld = LaunchDescription(actions) ls = LaunchService(debug=True) ls.include_launch_description(ld) - - loop = osrf_pycommon.process_utils.get_loop() - launch_task = loop.create_task(ls.run_async()) - loop.run_until_complete(asyncio.sleep(timeout_sec)) - if not launch_task.done(): - loop.create_task(ls.shutdown()) - loop.run_until_complete(launch_task) - assert 0 == launch_task.result() + assert 0 == ls.run() return ls.context From 9c50db2d509912fbb244ab8b858b5c57e4b890c8 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Fri, 14 Aug 2020 12:27:12 -0700 Subject: [PATCH 7/8] Properly shutdown mock conatiner node Also added some debug logs to the load node action for posterity. Signed-off-by: Jacob Perron --- launch_ros/launch_ros/actions/load_composable_nodes.py | 6 ++++++ .../test_launch_ros/actions/test_load_composable_nodes.py | 3 +++ 2 files changed, 9 insertions(+) diff --git a/launch_ros/launch_ros/actions/load_composable_nodes.py b/launch_ros/launch_ros/actions/load_composable_nodes.py index 378b26b4..b08d2b8c 100644 --- a/launch_ros/launch_ros/actions/load_composable_nodes.py +++ b/launch_ros/launch_ros/actions/load_composable_nodes.py @@ -99,7 +99,13 @@ def _load_node( ) ) return + self.__logger.debug( + "Calling the '{}' service with request '{}'".format( + self.__rclpy_load_node_client.srv_name, request + ) + ) response = self.__rclpy_load_node_client.call(request) + self.__logger.debug("Received response '{}'".format(response)) node_name = response.full_node_name if response.full_node_name else request.node_name if response.success: if node_name is not None: diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py index b6904d2d..de21fd89 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -75,6 +75,8 @@ def load_node_callback(self, request, response): return response def shutdown(self): + self._executor.shutdown() + self.destroy_node() rclpy.shutdown(context=self._context) self._thread.join() @@ -144,6 +146,7 @@ def test_load_node(mock_component_container): assert len(request.extra_arguments) == 0 + def test_load_node_with_remaps(mock_component_container): """Test loading a node with remappings.""" context = _assert_launch_no_errors([ From cf1437ad56b1b6c38b871b6dccf2cc2e6a8d2994 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Fri, 14 Aug 2020 12:48:39 -0700 Subject: [PATCH 8/8] Fix lint Signed-off-by: Jacob Perron --- .../test/test_launch_ros/actions/test_load_composable_nodes.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py index de21fd89..60ba8107 100644 --- a/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py +++ b/test_launch_ros/test/test_launch_ros/actions/test_load_composable_nodes.py @@ -146,7 +146,6 @@ def test_load_node(mock_component_container): assert len(request.extra_arguments) == 0 - def test_load_node_with_remaps(mock_component_container): """Test loading a node with remappings.""" context = _assert_launch_no_errors([