Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the ros2 daemon is stopped before running system tests #460

Merged
merged 2 commits into from
Jan 8, 2021
Merged

Ensure that the ros2 daemon is stopped before running system tests #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28594a7
Remove Opensplice from test_communication.
clalancette Jan 7, 2021
283ccd6
Kill off the ros2 daemon before running tests.
clalancette Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions test_communication/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,15 +191,6 @@ if(BUILD_TESTING)
set(rmw_implementation2_is_cyclonedds TRUE)
endif()

set(rmw_implementation1_is_opensplice FALSE)
set(rmw_implementation2_is_opensplice FALSE)
if(rmw_implementation1 MATCHES "(.*)opensplice(.*)")
set(rmw_implementation1_is_opensplice TRUE)
endif()
if(rmw_implementation2 MATCHES "(.*)opensplice(.*)")
set(rmw_implementation2_is_opensplice TRUE)
endif()

set(PUBLISHER_RMW ${rmw_implementation1})
set(SUBSCRIBER_RMW ${rmw_implementation2})
set(TEST_MESSAGE_TYPES "")
Expand All @@ -215,15 +206,6 @@ if(BUILD_TESTING)
)
continue()
endif()
# TODO(dirk-thomas) OpenSplice doesn't support WString and the rmw impl.
# maps it to string which has a different wire representation
if(
"${message_type}" STREQUAL "WStrings" AND
(rmw_implementation1_is_opensplice OR rmw_implementation2_is_opensplice) AND
NOT "${rmw_implementation1_is_opensplice}" STREQUAL "${rmw_implementation2_is_opensplice}"
)
continue()
endif()
# TODO(dirk-thomas) Connext and CycloneDDS don't interoperate for WString
if(
"${message_type}" STREQUAL "WStrings" AND
Expand Down Expand Up @@ -468,7 +450,7 @@ if(BUILD_TESTING)

macro(serialize)
set(SKIP_TEST "")
if(${rmw_implementation} STREQUAL "rmw_connext_dynamic_cpp" OR ${rmw_implementation} STREQUAL "rmw_opensplice_cpp")
if(${rmw_implementation} STREQUAL "rmw_connext_dynamic_cpp")
message(STATUS "skipping serialize tests for ${rmw_implementation}")
set(SKIP_TEST "SKIP_TEST")
endif()
Expand Down Expand Up @@ -500,7 +482,7 @@ if(BUILD_TESTING)

macro(pub_sub_serialized)
set(SKIP_TEST "")
if(${rmw_implementation} STREQUAL "rmw_connext_dynamic_cpp" OR ${rmw_implementation} STREQUAL "rmw_opensplice_cpp")
if(${rmw_implementation} STREQUAL "rmw_connext_dynamic_cpp")
message(STATUS "skipping serialize tests for ${rmw_implementation}")
set(SKIP_TEST "SKIP_TEST")
endif()
Expand Down
1 change: 1 addition & 0 deletions test_communication/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
<test_depend>rclpy</test_depend>
<test_depend>rmw_implementation</test_depend>
<test_depend>rmw_implementation_cmake</test_depend>
<test_depend>ros2cli</test_depend>
<test_depend>test_msgs</test_depend>

<export>
Expand Down
47 changes: 25 additions & 22 deletions test_communication/test/test_action_client_server.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,41 +23,44 @@ def generate_test_description(action_type):
launch_description = LaunchDescription()

action_client_cmd = ['@TEST_ACTION_CLIENT_EXECUTABLE@', action_type, namespace]
action_server_cmd = ['@TEST_ACTION_SERVER_EXECUTABLE@', action_type, namespace]

action_server_env = dict(os.environ)
action_client_env = dict(os.environ)

if '@TEST_ACTION_SERVER_RCL@' == 'rclpy':
action_server_cmd.insert(0, sys.executable)
action_server_env['PYTHONUNBUFFERED'] = '1'
if '@TEST_ACTION_CLIENT_RCL@' == 'rclpy':
action_client_cmd.insert(0, sys.executable)
action_client_env['PYTHONUNBUFFERED'] = '1'

action_server_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@ACTION_SERVER_RMW@'
action_server_env['RMW_IMPLEMENTATION'] = '@ACTION_SERVER_RMW@'

launch_description.add_action(ExecuteProcess(
cmd=action_server_cmd,
name='test_action_server',
env=action_server_env,
))

action_client_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@ACTION_CLIENT_RMW@'
action_client_env['RMW_IMPLEMENTATION'] = '@ACTION_CLIENT_RMW@'

action_client_process = ExecuteProcess(
cmd=action_client_cmd,
name='test_action_client',
env=action_client_env,
)
launch_description.add_action(action_client_process)

launch_description.add_action(
launch_testing.actions.ReadyToTest()
action_server_cmd = ['@TEST_ACTION_SERVER_EXECUTABLE@', action_type, namespace]
action_server_env = dict(os.environ)
if '@TEST_ACTION_SERVER_RCL@' == 'rclpy':
action_server_cmd.insert(0, sys.executable)
action_server_env['PYTHONUNBUFFERED'] = '1'
action_server_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@ACTION_SERVER_RMW@'
action_server_env['RMW_IMPLEMENTATION'] = '@ACTION_SERVER_RMW@'

action_server_process = ExecuteProcess(
cmd=action_server_cmd,
name='test_action_server',
env=action_server_env,
)
return launch_description, locals()

return LaunchDescription([
# Always stop daemon to isolate tests.
ExecuteProcess(
cmd=['ros2', 'daemon', 'stop'],
name='daemon-stop',
Comment on lines +55 to +56
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we have a daemon running in the background in the first place.
This looks fine as a workaround, but all previous process that created a daemon should not be "leaking it".

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can have something running in CI that detects when a test "leaks" a ros 2 daemon, but it doesn't sound like something trivial to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder why we have a daemon running in the background in the first place.
This looks fine as a workaround, but all previous process that created a daemon should not be "leaking it".

In other places in the codebase, we are careful to kill the daemon before running the tests. For instance: https://github.com/ros2/ros2cli/blob/f6c4a5b05d172f8156645b8fcf8bd2dfa23e7bb2/ros2node/test/test_cli_duplicate_node_names.py#L55 . From that perspective, it is good for each test to be "defensive", and make sure the environment is the way it should be before starting.

But we are not similarly careful about tests cleaning up after themselves. In that same example in ros2node, we kill the daemon on startup, but then we do not kill it at the end. Maybe we should enhance all of the tests do do that, but it is honestly not clear to me. Partly the problem is that starting up the daemon can be a side-effect of doing other things (like running a ros2 cli command), so it is not always clear which tests need to kill it at the end.

Copy link
Member

Choose a reason for hiding this comment

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

but then we do not kill it at the end. Maybe we should enhance all of the tests do do that,

👍 I think that killing the daemon at the end of those tests would be a good idea.

Partly the problem is that starting up the daemon can be a side-effect of doing other things (like running a ros2 cli command), so it is not always clear which tests need to kill it at the end.

Yes, having a defensive ros2 daemon stop before testing doesn't look bad, but it's also not clear which tests can be affected by the presence of a "leaked" daemon and which ones not.
So it sounds like we would actually like to run "ros2 daemon stop" before running each test.

on_exit=[
action_server_process,
action_client_process,
launch_testing.actions.ReadyToTest()
]
)
]), locals()


class TestActionClientServer(unittest.TestCase):
Expand Down
37 changes: 20 additions & 17 deletions test_communication/test/test_publisher_subscriber.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,40 +23,43 @@ def generate_test_description(message_type):
launch_description = LaunchDescription()

publisher_cmd = ['@TEST_PUBLISHER_EXECUTABLE@', message_type, namespace]
subscriber_cmd = ['@TEST_SUBSCRIBER_EXECUTABLE@', message_type, namespace]

publisher_env = dict(os.environ)
subscriber_env = dict(os.environ)

if '@TEST_PUBLISHER_RCL@' == 'rclpy':
publisher_cmd.insert(0, sys.executable)
publisher_env['PYTHONUNBUFFERED'] = '1'
if '@TEST_SUBSCRIBER_RCL@' == 'rclpy':
subscriber_cmd.insert(0, sys.executable)
subscriber_env['PYTHONUNBUFFERED'] = '1'

publisher_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@PUBLISHER_RMW@'
publisher_env['RMW_IMPLEMENTATION'] = '@PUBLISHER_RMW@'

launch_description.add_action(ExecuteProcess(
publisher_process = ExecuteProcess(
cmd=publisher_cmd,
name='test_publisher',
env=publisher_env,
))
)

subscriber_cmd = ['@TEST_SUBSCRIBER_EXECUTABLE@', message_type, namespace]
subscriber_env = dict(os.environ)
if '@TEST_SUBSCRIBER_RCL@' == 'rclpy':
subscriber_cmd.insert(0, sys.executable)
subscriber_env['PYTHONUNBUFFERED'] = '1'
subscriber_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@SUBSCRIBER_RMW@'
subscriber_env['RMW_IMPLEMENTATION'] = '@SUBSCRIBER_RMW@'

subscriber_process = ExecuteProcess(
cmd=subscriber_cmd,
name='test_subscriber',
env=subscriber_env,
)
launch_description.add_action(subscriber_process)
launch_description.add_action(
launch_testing.actions.ReadyToTest()
)
return launch_description, locals()

return LaunchDescription([
# Always stop daemon to isolate tests.
ExecuteProcess(
cmd=['ros2', 'daemon', 'stop'],
name='daemon-stop',
on_exit=[
publisher_process,
subscriber_process,
launch_testing.actions.ReadyToTest()
]
)
]), locals()


class TestPublisherSubscriber(unittest.TestCase):
Expand Down
38 changes: 19 additions & 19 deletions test_communication/test/test_requester_replier.py.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,44 +20,44 @@ SERVICE_TYPES = '@TEST_SERVICE_TYPES@'.split(';')
def generate_test_description(service_type):
namespace = '/test_time_%s' % time.strftime('%H_%M_%S', time.gmtime())

launch_description = LaunchDescription()

requester_cmd = ['@TEST_REQUESTER_EXECUTABLE@', service_type, namespace]
replier_cmd = ['@TEST_REPLIER_EXECUTABLE@', service_type, namespace]

replier_env = dict(os.environ)
requester_env = dict(os.environ)

if '@TEST_REPLIER_RCL@' == 'rclpy':
replier_cmd.insert(0, sys.executable)
replier_env['PYTHONUNBUFFERED'] = '1'
if '@TEST_REQUESTER_RCL@' == 'rclpy':
requester_cmd.insert(0, sys.executable)
requester_env['PYTHONUNBUFFERED'] = '1'

replier_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@REPLIER_RMW@'
replier_env['RMW_IMPLEMENTATION'] = '@REPLIER_RMW@'

launch_description.add_action(ExecuteProcess(
replier_process = ExecuteProcess(
cmd=replier_cmd,
name='test_replier',
env=replier_env,
))
)

requester_cmd = ['@TEST_REQUESTER_EXECUTABLE@', service_type, namespace]
requester_env = dict(os.environ)
if '@TEST_REQUESTER_RCL@' == 'rclpy':
requester_cmd.insert(0, sys.executable)
requester_env['PYTHONUNBUFFERED'] = '1'
requester_env['RCL_ASSERT_RMW_ID_MATCHES'] = '@REQUESTER_RMW@'
requester_env['RMW_IMPLEMENTATION'] = '@REQUESTER_RMW@'

requester_process = ExecuteProcess(
cmd=requester_cmd,
name='test_requester',
env=requester_env,
)
launch_description.add_action(requester_process)

launch_description.add_action(
launch_testing.actions.ReadyToTest()
)
return launch_description, locals()
return LaunchDescription([
# Always stop daemon to isolate tests.
ExecuteProcess(
cmd=['ros2', 'daemon', 'stop'],
name='daemon-stop',
on_exit=[
replier_process,
requester_process,
launch_testing.actions.ReadyToTest()
]
)
]), locals()


class TestRequesterReplier(unittest.TestCase):
Expand Down