Add `WaitForTopics` example to integration testing tutorial

[christophebedard]

Issue Type

• 🐛 Bug / Problem
• ✏️ Typo / Grammar
• 📖 Outdated Content
• 🚀 Enhancement

Distribution

No response

Description

Creating this issue as a follow-up to this comment: ros2/launch_ros#356 (comment).

An integration testing tutorial was added in #4881: https://docs.ros.org/en/rolling/Tutorials/Intermediate/Testing/Integration.html.

It mentions launch_testing_ros' WaitForTopics, but it should ideally include an example, especially since WaitForTopics could replace some of the common logic in one of the example tests, see #4881 (comment).

Affected Pages/Sections

https://docs.ros.org/en/rolling/Tutorials/Intermediate/Testing/Integration.html

Screenshots or Examples (if applicable)

No response

Suggested Fix

No response

Additional Context

No response

[LastStarDust]

@christophebedard If nobody else is working on this, I think I can do it.

[LastStarDust]

I read the article and found it very interesting. My implementation shares many similarities, and I really wish this tutorial had been available when I first started working with ROS.

I have a couple of questions:

1. As the author mentions in the article, WaitForTopics achieves similar behavior. That’s absolutely true. Essentially, the author is just reinventing WaitForTopics. My question is: would you prefer that I replace the example with one that uses WaitForTopics to achieve the same behavior, or should I add an alternative implementation using WaitForTopics alongside the current one?

2. The author made a very insightful comment:

   If you want to go further, you can implement a third test that publishes a twist message, asking the turtle to move, and subsequently checks that it moved by asserting that the pose message changed.

   This is exactly what I’m trying to address in WaitForTopics: let the user inject a callaback to be executed after starting the subscribers launch_ros#356. Would you like me to implement the Twist message trigger using the new feature I introduced?

3. This question is somewhat beyond the scope of this ticket, but I noticed several magic numbers in the example, such as the 0.5 second wait and the 10 seconds interval, which appear arbitrary. How were these values chosen? What is the purpose of the 0.5 second wait specifically? I’d be hesitant to include code like this in a tutorial, especially one that may be followed by young or inexperienced users. It could encourage the use of magic numbers and arbitrary delays in tests, which is bad practice IMHO. If the 0.5 delay is actually needed and bad things happen without it, isn't that code smell hinting at a bug in the launch_ros package?

[christophebedard]

1. As the author mentions in the article, WaitForTopics achieves similar behavior. That’s absolutely true. Essentially, the author is just reinventing WaitForTopics. My question is: would you prefer that I replace the example with one that uses WaitForTopics to achieve the same behavior, or should I add an alternative implementation using WaitForTopics alongside the current one?

I think you should just replace the logic with WaitForTopics and not have 2 ways to do the same thing to keep it simple. However, do make sure to properly explain what WaitForTopics does.

2. The author made a very insightful comment:

   If you want to go further, you can implement a third test that publishes a twist message, asking the turtle to move, and subsequently checks that it moved by asserting that the pose message changed.

   This is exactly what I’m trying to address in WaitForTopics: let the user inject a callaback to be executed after starting the subscribers launch_ros#356. Would you like me to implement the Twist message trigger using the new feature I introduced?

Absolutely!

3. This question is somewhat beyond the scope of this ticket, but I noticed several magic numbers in the example, such as the 0.5 second wait and the 10 seconds interval, which appear arbitrary. How were these values chosen? What is the purpose of the 0.5 second wait specifically? I’d be hesitant to include code like this in a tutorial, especially one that may be followed by young or inexperienced users. It could encourage the use of magic numbers and arbitrary delays in tests, which is bad practice IMHO. If the 0.5 delay is actually needed and bad things happen without it, isn't that code smell hinting at a bug in the launch_ros package?

The "bug" is that the active tests might start executing before the active tests run, so the hardcoded 0.5-second wait before launch_testing.actions.ReadyToTest() is probably to ensure that the node is ready before then, but yeah, this method isn't great. The tests should instead wait until the publisher/whatever is available, potentially up to some timeout value.

I think the 10-second value, combined with an expectation of 100 messages received, is meant to assert the publisher rate, but that's indeed a weirdly-specific and time-sensitive assertion. I also wouldn't do that unless there's a better reason for it: just "checking whether pose messages get published" is not a good reason to do it this way IMO, and it sets a bad example, like you said.

Overall, I would indeed try to replace these hardcoded wait/sleep values and cross my fingers that the tests still work reliably. The unfortunate reality is that there's a lot of those hardcoded sleep/wait values in tests for reasons. Hopefully with WaitForTopics and future improvements, we can remove these arbitrary waits/sleeps everywhere 😁

---

Does that answer your questions?

By the way, you could move the code to separate files and include them with the .. literalinclude:: directive, potentially specifying a line range with :lines: 1-2 (search the docs source files for some examples). This way it would be a lot easier to actually build & run them.