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

launch testing : Wait for topics to publish #274

Merged
merged 11 commits into from
Oct 6, 2021

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Oct 4, 2021

This aims to close #272. It aims to provide an API in to be used with launch_testing where we provide a set of topics and we wait for at least a single message to appear on all of them.

Sample usage :

import unittest
import launch
import launch.actions
import launch_ros.actions
import launch_testing.actions
import launch_testing.markers
import pytest
import rclpy
from rclpy.node import Node
from std_msgs.msg import String

from launch_testing_ros import WaitForTopics

def generate_node(i):
    return launch_ros.actions.Node(
        executable='talker',
        package='demo_nodes_cpp',
        name='demo_node_' + str(i),
        remappings = [('chatter', 'chatter_' + str(i))]
    )

@pytest.mark.launch_test
@launch_testing.markers.keep_alive
def generate_test_description():
    # 'n' changes the number of nodes and topics
    n = 5
    description = [generate_node(i) for i in range(n)] + [launch_testing.actions.ReadyToTest()]
    return launch.LaunchDescription(description), {'count':n}

class TestFixture(unittest.TestCase):

    def test_check_if_msgs_published(self, proc_output, count):

        topic_list = [('chatter_' + str(i), String) for i in range(count)]
        with WaitForTopics(topic_list, timeout=5.0):
            print("The topics are alive !")


Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 self-assigned this Oct 4, 2021
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I think this is a good start. I've left some feedback below.

Also, I'm not sure that creating a separate node class adds much value; I would have just put all the logic inside WaitForTopics. I'm okay with it either way though.

launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
self.create_subscription(
topic_type,
topic_name,
self.callback_template(topic_name),
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we can use functools.partial.

functools.partial(self.topic_callback, topic_name)

and then the callback would look like:

def topic_callback(self, topic_name, data):
    ...

I don't think there's anything wrong with this, so feel free to leave it as-is.

launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
@jacobperron jacobperron added the enhancement New feature or request label Oct 5, 2021
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, two more comments below

launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
launch_testing_ros/launch_testing_ros/wait_for_topics.py Outdated Show resolved Hide resolved
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

CI :
build : --packages-above-and-dependencies launch_testing_ros
test : --packages-above launch_testing_ros

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

A couple suggestions for the test code. LGTM

Signed-off-by: Aditya Pande <aditya050995@gmail.com>
@adityapande-1995 adityapande-1995 merged commit bbcc0cc into master Oct 6, 2021
@delete-merged-branch delete-merged-branch bot deleted the aditya/wait_for_topic branch October 6, 2021 17:36
@Blast545
Copy link
Contributor

Blast545 commented Oct 7, 2021

👨‍🌾 It seems that this PR caused several regressions in the WIndows buildfarms jobs:

nightly_win_rel#2079
nightly_win_deb#2134

Do you think you can take a look to a possible fix @adityapande-1995? I think it's likely related to these lines:

https://github.com/ros2/launch_ros/blob/master/launch_testing_ros/launch_testing_ros/wait_for_topics.py#L20-L23

It seems to me that rclpy can't be imported there for some reason. I'll open a revert PR in the meantime to be sure this PR caused the regressions.

@clalancette
Copy link
Contributor

It's weird that this caused a regression, particularly because CI seems to have been run properly and came back green. But let's see if the revert PR fixes it.

Blast545 added a commit that referenced this pull request Oct 7, 2021
Blast545 added a commit that referenced this pull request Oct 7, 2021
This reverts commit bbcc0cc.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545
Copy link
Contributor

Blast545 commented Oct 7, 2021

I can't replicate the error in CI. I'll investigate a bit deeper before reaching any conclusion.

@aprotyas
Copy link
Member

aprotyas commented Oct 8, 2021

It seems to me that rclpy can't be imported there for some reason. I'll open a revert PR in the meantime to be sure this PR caused the regressions.

I believe that's for this. Not sure why it can't be imported before running tests though.
https://github.com/ros2/rclpy/blob/6dd9540661631c971499d5672fb50a8ec1e3ac22/rclpy/test/__init__.py#L20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request (launch_testing): Check if a topic is publishing messages, with a timeout
5 participants