add launch-based nosetest with simple pub/sub testing for all rmw implementations #7
Conversation
| while (rclcpp::ok() and i <= number_of_messages) { | ||
| set_data_func(ros_msg, i); | ||
| p->publish(ros_msg); | ||
| std::cout << "published ros msg #" << i << std::endl; |
There was a problem hiding this comment.
I thought the line was important 😉
Fixed.
|
lgtm |
dirk-thomas
force-pushed
the
launch_package
branch
from
c6b4048
to
c541473
Compare
|
Other than the comments, this lgtm and I think it should be merged. I'm excited to see some really easy to use process management in tests, and this is a good proof of concept. However, I think to be scalable we have to streamline the patterns here some more. However, I don't just want to complain without giving constructive feedback, so we should have a meeting about this to discuss details about testing. I would just list suggestions on how we could stream line testing, but it hinges on some fundamental points about testing, for example do we test before or after installing the package we are testing? And do we source the installation target space before running tests? Based on the answers to those questions I would suggest different strategies to streamline the process for making tests like this. But since it's neither clear to me what the right answers to those questions are nor clear to me what we're currently doing, I think we should discuss our test process before working further on making writing tests easy and low effort. |
| rclcpp::Node::SharedPtr node, | ||
| void (*set_data_func)(typename T::Ptr&, size_t), | ||
| size_t number_of_messages | ||
| ) { |
There was a problem hiding this comment.
We should discuss this particular point of code style again outside of the scope of this pr, because I would have written this as:
template<typename T>
int publish(
rclcpp::Node::SharedPtr node,
void (*set_data_func)(typename T::Ptr&, size_t),
size_t number_of_messages)
{
...
}Such that the opening { is visually lined up with its counter part on line 34, rather than the one on line 27, which it is lined up with but does not match. Normally I would say what you wrote above is good because it minimizes the number of lines of delta to add a parameter to the end or parameters, but since you cannot leave a trailing , it's not the case here, so it doesn't really make it better to put the closing ) on it's own line.
|
Another question, do we have a consensus on whether or not test code should have a license and copyright block at the top of files? |
| auto node = rclcpp::Node::make_shared("test_subscriber"); | ||
|
|
||
| rclcpp::subscription::SubscriptionBase::SharedPtr sub; | ||
| sub = subscribe<simple_msgs::Uint32>(node, print_counter_data); |
There was a problem hiding this comment.
You could simplify this by using auto:
auto sub = subscribe<simple_msgs::Uint32>(node, print_counter_data);
dirk-thomas
force-pushed
the
launch_package
branch
from
c541473
to
8080180
Compare
|
Ok, I'm done with my review, |
dirk-thomas
force-pushed
the
launch_package
branch
from
8080180
to
41d370a
Compare
|
I think any code file should have copyright / license information. The only files excluded are commonly CMakeLists.txt / setup.py / Makefiles. I will wait for the |
dirk-thomas
force-pushed
the
launch_package
branch
from
41d370a
to
d6a0137
Compare
|
@wjwwood Thanks for the comments - I have addressed them. |
|
Sounds good to me. |
|
+1 |
add launch-based nosetest with simple pub/sub testing for all rmw implementations
Connects to ros2/ros2#4
Current state of these unit tests:
double free or corruption: Non deterministic double free or corruption on ctrl-c rclcpp#4Depends on ros2/rclcpp#17