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

Get content of topic from launch_testing_ros::WaitForTopics #346

Closed
LastStarDust opened this issue Jan 25, 2023 · 6 comments
Closed

Get content of topic from launch_testing_ros::WaitForTopics #346

LastStarDust opened this issue Jan 25, 2023 · 6 comments
Assignees

Comments

@LastStarDust
Copy link
Contributor

LastStarDust commented Jan 25, 2023

Feature request

Feature description

In the WaitForTopics sample code here, I see how I can easily test that a node has published a topic with a certain name, but it is not shown how to test the actual content of the topic.

It would be very helpful if the content of the topic could be available to the tester, too.

Implementation considerations

Looking deeper at the source code here, it seems that only the topic name is stored.

Because of Python duck-typing it should be more or less easy to just return the content of the topic. Something like

    def callback_template(self, topic_name):

        def topic_callback(data):
            if topic_name not in self.received_topics:
                self.get_logger().debug('Message received for ' + topic_name)
                self.received_topics.add(topic_name)
                self.received_messages[topic_name].append(data)
            if self.received_topics == self.expected_topics:
                self.msg_event_object.set()

        return topic_callback

where self.received_messages is a dictionary where the key is the topic name and the value is the list of received messages. Just as an example.

@LastStarDust
Copy link
Contributor Author

If you deem this feature useful, I would be available to implement it and make a pull request.

@adityapande-1995
Copy link
Contributor

Hi @LastStarDust ! That is a good suggestion. While WaitForTopics was originally meant to just block until all the mentioned topics receive messages, it can be useful to know what the message was. A couple of questions :

  1. Should the message stored be the last message before WaitForTopics() returns, or the first message a topic receives ?
  2. Should this workflow be preferred over adding a separate subscriber ?

If you could open a PR, I'll be happy to review it.

@LastStarDust
Copy link
Contributor Author

@adityapande-1995 Hello. Thank you for your feedback. I created a pull request #353 for this feature as you suggested.

LastStarDust added a commit to LastStarDust/launch_ros that referenced this issue Feb 13, 2023
LastStarDust added a commit to LastStarDust/launch_ros that referenced this issue May 22, 2023
adityapande-1995 pushed a commit that referenced this issue Jun 22, 2023
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
@Ryanf55
Copy link

Ryanf55 commented Jan 16, 2024

Hello, I would like to re-iterate how important this feature is for creating production-ready ROS code that is tested at the middleware level.

In following the V-model of development in commercial projects that use ROS 2, it is very important to have tooling to send and receive messages, services, and actions to validate that the behavior of a node is in line with the requirements, and to be able to have reliable automated tests that can prove adherence to the requirements.

Asserting that the node published a message, but ignoring the content is not sufficient enough of a test to demonstrate adherence to design requirements for any asynchronous publishers.

A great example is to check that the header timestamp is not zero and is the same as the event that triggered the publish. If a node receives an event, and a developer incorrectly uses the wrong timestamping by calling node->now(), that is a big problem for time-critical behaviors, and it's super common in development to mess up the timestamping. Take a look at my recent NAV2 work on TwistStamped support as proof these pitfalls occur in some of the "best" FOSS ROS 2 code.

Currently, my team is maintaining a layer on top of launch_ros to perform this kind of testing, and would much prefer to work with the ROS community to improve test infrastructure. Being able to have a batteries-included test infrastructure will help companies focus on testing their node behaviors rather than writing boilerplate or infra.

LastStarDust has put a great amount of work to supporting this kind of testing, however his work hasn't gotten to humble where it can be used in production. I ask the maintainers and OSRC to focus some renewed time and energy on contributions that support the production testing of ROS 2.

mergify bot pushed a commit that referenced this issue Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)
adityapande-1995 pushed a commit that referenced this issue Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)
Signed-off-by: Aditya Pande <[email protected]>
mergify bot pushed a commit that referenced this issue Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
adityapande-1995 pushed a commit that referenced this issue Jan 16, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>
adityapande-1995 pushed a commit that referenced this issue Jan 17, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>
mjcarroll pushed a commit that referenced this issue Jan 20, 2024
* Get content of topic from launch_testing_ros::WaitForTopics
#346

---------


(cherry picked from commit 2d125a5)

Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Co-authored-by: Giorgio Pintaudi <[email protected]>
mjcarroll pushed a commit that referenced this issue Jan 20, 2024
… (#389)

* `WaitForTopics`: get content of messages for each topic (#353)

* Get content of topic from launch_testing_ros::WaitForTopics
#346
Signed-off-by: Pintaudi Giorgio <[email protected]>
---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
(cherry picked from commit 2d125a5)

# Conflicts:
#	launch_testing_ros/launch_testing_ros/wait_for_topics.py
#	launch_testing_ros/test/examples/wait_for_topic_launch_test.py
Signed-off-by: Aditya Pande <[email protected]>

* Resolve conflicts (#390)

Signed-off-by: Ryan Friedman <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>

---------

Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Aditya Pande <[email protected]>
Signed-off-by: Ryan Friedman <[email protected]>
Co-authored-by: Giorgio Pintaudi <[email protected]>
Co-authored-by: Ryan <[email protected]>
@Ryanf55
Copy link

Ryanf55 commented Jan 20, 2024

This is now backported to all active distros. It is now solved. Please close the issue as completed.

@clalancette
Copy link
Contributor

Closing as solved by #353

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

No branches or pull requests

4 participants