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.

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

talker_listener_launch_test example is broken #97

Closed
cottsay opened this issue Nov 25, 2019 · 8 comments · Fixed by #121
Closed

talker_listener_launch_test example is broken #97

cottsay opened this issue Nov 25, 2019 · 8 comments · Fixed by #121
Labels
bug Something isn't working

Comments

@cottsay
Copy link
Member

cottsay commented Nov 25, 2019

Trying to run this launch test:

python3 -m launch_testing.launch_test src/ros2/launch_ros/launch_testing_ros/test/examples/talker_listener_launch_test.py

Yields:

Traceback (most recent call last):
  File "src/ros2/launch_ros/launch_testing_ros/test/examples/talker_listener_launch_test.py", line 72, in test_talker_transmits
    node = launch_context.locals.launch_ros_node
  File "build/launch/launch/launch_context.py", line 137, in __getattr__
    ', '.join(_dict.keys())
AttributeError: context.locals does not contain attribute 'launch_ros_node', it contains: []
@cottsay cottsay added the bug Something isn't working label Nov 25, 2019
@teutobod
Copy link

Any updates here? I'm stuck to the same issue.

@jacobperron
Copy link
Member

I can reproduce the issue.

Interestingly, if you run the test with the CLI tool ros2 test, then there is no error:

ros2 test src/ros2/launch_ros/launch_testing_ros/test/examples/talker_listener_launch_test.py

@jacobperron
Copy link
Member

I guess it kind of makes sense, since the ROS node is specific to launch_ros and not launch; the node is started by the default launch_ros description here:

def get_default_launch_description(*, rclpy_context=None):
"""
Return a LaunchDescription to be included before user descriptions.
:param: rclpy_context Provide a context other than the default rclpy context
to pass down to rclpy.init.
The context is expected to have already been initialized by the caller
using rclpy.init().
"""
default_ros_launch_description = launch.LaunchDescription([
# ROS initialization (create node and other stuff).
ROSSpecificLaunchStartup(rclpy_context=rclpy_context)
])

I think either the README should be updated to use ros2 test, or the example launch file should be updated to use the get_default_launch_description.

@jacobperron
Copy link
Member

Updating the launch file fixes the issue for launching with launch_test, e.g.:

    ld = launch_ros.get_default_launch_description()
    ld.add_action(talker_node)
    ld.add_action(listener_node)
    # Start tests right away - no need to wait for anything
    ld.add_action(launch_testing.actions.ReadyToTest())

But, then two nodes are created if we run with ros2 test and we get the following warning:

[WARN] [rcl.logging_rosout]: Publisher already registered for provided node name. If this is due to multiple nodes with the same name then all logs for that logger name will go out over the existing publisher. As soon as any node with that name is destructed it will unregister the publisher, preventing any further logs for that name from being published on the rosout topic.

Seems like a different approach is required.

@hidmic
Copy link
Contributor

hidmic commented Jan 2, 2020

There's no good solution here really, besides not mixing up launch_test and ros2 test. The former just runs the test, the latter injects launch_ros default launch description. There was a discussion about this in ros2/launch#208. I still think tools should not inject anything, but consensus back then was to reduce boilerplate as much as possible.

jacobperron added a commit that referenced this issue Jan 3, 2020
Fixes #97

Inject the ROS-specific preamble so that the test can be run as described in the README.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member

See #112 for a fix of the example.

Longer term, I think we need to remove the injection from the CLI tool, or be able to detect that the preamble has already been included to avoid including it more than once.

@stonier
Copy link

stonier commented Jan 23, 2020

Got burned by this last night as well.

+1 for removing the injection. Reasoning:

  • Prefer to have one launch test file format, not two (e.g. the current launch_test file is broken with launch_test, which makes it something else).
  • Prefer not to depend on user friendly tooling (i.e. ros2 test). There already is an execution framework (i.e. launch_test) so ros2 test should be there to provide conveniences, not dependencies.

jacobperron added a commit that referenced this issue Jan 30, 2020
The example should not be using the underlying launch_ros ROS context or ROS node since it is an
implementation detail. Instead, the example initailizes it's own context and ROS node.

Fixes #97.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Member

I've fixed the example in #121 and opened a ticket to discuss removing the injected launch description: #120

jacobperron added a commit that referenced this issue Jan 31, 2020
The example should not be using the underlying launch_ros ROS context or ROS node since it is an
implementation detail. Instead, the example initailizes it's own context and ROS node.

Fixes #97.

Signed-off-by: Jacob Perron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants