Added Unit Test for Republisher Node on Noetic Branch #39
Conversation
IrinaTerebiznik
changed the title
IrinaTerebiznik
changed the title
There was a problem hiding this comment.
Reviewed! The test looks good, I added a few questions and change requests.
Also, please rename the UnitTests folder to use lowecase and underscores, as we adhere to PEP8 style guide: https://peps.python.org/pep-0008/#package-and-module-names
Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability. Python packages should also have short, all-lowercase names, although the use of underscores is discouraged.
| @@ -1 +1,6 @@ | |||
| __pycache__/ | |||
| /inorbit_republisher/test/sample_data/build/ | |||
There was a problem hiding this comment.
These ignores shouldn't be needed if running on a docker container
| @@ -54,6 +54,16 @@ republishers: | |||
| topic: "/inorbit/custom_data/0" | |||
| key: "navsat" | |||
|
|
|||
| # Añadido para los tópicos input y output | |||
There was a problem hiding this comment.
Please avoid using language other than english
| @@ -54,6 +54,16 @@ republishers: | |||
| topic: "/inorbit/custom_data/0" | |||
| key: "navsat" | |||
|
|
|||
| # Añadido para los tópicos input y output | |||
| - topic: "/input_topic" | |||
There was a problem hiding this comment.
I wouldn't use this file for testing purposes, as it's the example used on the main README. Instead, I'd create a separate configuration file e.g. unittest_config.yaml under the test folder, and only include this section.
| 1. **Build the Workspace**: | ||
| Ensure the workspace is built and the environment is sourced: | ||
| ```bash | ||
| cd ~/catkin_ws |
There was a problem hiding this comment.
Please add instructions for running tests on a docker container (see package README). This seems to run on a host machine
| 2. **Source and Run the Workspace**: | ||
| ```bash | ||
| . ~/catkin_ws/devel/setup.bash | ||
| rostest inorbit_republisher test_republisher.test |
| self.test_pub = rospy.Publisher('/input_topic', String, queue_size=10) | ||
| self.received_messages = [] | ||
| rospy.Subscriber('/output_topic', String, self.callback) | ||
| rospy.sleep(1) |
There was a problem hiding this comment.
Do you need a sleep here? Why?
Note that sleeps or wait times on unittests are typically a smell and are discouraged.
| expected_message = f"input_to_output={test_message}" | ||
| rospy.loginfo(f"Publishing: {test_message} to /input_topic") | ||
| self.test_pub.publish(String(data=test_message)) | ||
| rospy.sleep(1) |
There was a problem hiding this comment.
Have you tried rospy.wait_for_message instead of the sleep?
| rospy.loginfo(f"Messages received: {self.received_messages}") | ||
| self.assertIn(expected_message, self.received_messages) | ||
|
|
||
| def test_no_message(self): |
There was a problem hiding this comment.
I don't see the value of this test. self.received_messages is initialized as an empty list on the setUp, so it will always have len == 0.
This repository contains a unit test for a ROS node that republishes messages from
/input_topicto/output_topic. The test ensures the republisher node processes and republishes messages correctly.Note that this test is non-automated and must be run manually, as explained in the README in the test/UnitTests folder.