Qos system tests #1
Conversation
Added simple test for liveliness Signed-off-by: Devin Bonnie <[email protected]>
Added lifespan system test Signed-off-by: Devin Bonnie <[email protected]>
Signed-off-by: Devin Bonnie <[email protected]>
Added minor liveliness test cleanup Signed-off-by: Devin Bonnie <[email protected]>
|
Closing this PR and opening against ros2/system_tests per standup today. |
| * @param milliseconds | ||
| * @return tuple of milliseconds converted to <seconds, nanoseconds> | ||
| */ | ||
| std::tuple<size_t, size_t> chrono_milliseconds_to_size_t( |
There was a problem hiding this comment.
We should use a time type of some sort rather than generic tuple. e.g. rmw_time_t or rcl_duration, something.
There was a problem hiding this comment.
That could work. Might be nice to add in rcutils or in rclcpp for others to use and really be useful :-)
| const std::chrono::milliseconds & milliseconds); | ||
|
|
||
| /// Simple publishing node used for system tests | ||
| class Publisher : public rclcpp::Node |
There was a problem hiding this comment.
A namespace or prefixed naming would be nice for these - so that there's no possible confusion with the rclcpp types. I'd recommend something along the lines of QoSTestPublisherNode
| /// number of messages published | ||
| int sent_message_count_; | ||
| /// if true then currently has an active publishing timer | ||
| bool is_publishing = false; |
There was a problem hiding this comment.
May want to go private and public instead of using separate naming conventions (with or without the suffix _). Wouldn't hurt to go all-private and provide accessors for the ones you want to be externally used.
There was a problem hiding this comment.
This PR is stale, here's the most recent: ros2#347
| std::recursive_mutex toggle_publish_mutex; | ||
| }; | ||
|
|
||
| class Subscriber : public rclcpp::Node |
There was a problem hiding this comment.
All comments from Publisher apply
| using namespace std::chrono_literals; | ||
|
|
||
| /// Liveliness setup | ||
| class LivelinessSetup : public ::testing::Test |
There was a problem hiding this comment.
Utilities TestSetup? Quick eyeball looks identical
| ASSERT_EQ(0, event.not_alive_count); | ||
| ASSERT_EQ(-1, event.alive_count_change); | ||
| ASSERT_EQ(0, event.not_alive_count_change); | ||
| } |
| RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "QOSDeadlineOfferedInfo callback"); | ||
| total_number_of_publisher_deadline_events++; | ||
| // assert the correct value on a change | ||
| ASSERT_EQ(1, event.total_count_change); |
There was a problem hiding this comment.
Assert total_count against our locally-tracked counter total_number_of_publisher_deadline_events?
There was a problem hiding this comment.
This is a simple check to ensure the callback is working (to fail fast). The total count is checked at the end.
| RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "QOSDeadlineRequestedInfo callback"); | ||
| total_number_of_subscriber_deadline_events++; | ||
| // assert the correct value on a change | ||
| ASSERT_EQ(1, event.total_count_change); |
There was a problem hiding this comment.
Assert total_count against our locally-tracked counter total_number_of_subscriber_deadline_events?
| max_test_length / expected_number_of_pub_events, | ||
| [&publisher]() -> void { | ||
| // start / stop publishing to trigger deadline | ||
| publisher->toggle_publishing(); |
There was a problem hiding this comment.
Would it be a good idea to remove this step of complexity and just have the publisher never publish at all, since we're only trying to trigger the deadline events?
There was a problem hiding this comment.
Yeah great point, I think I'll add another test which does exactly that (it's a valid permutation that should be stressed).
| EXPECT_GT(subscriber->received_message_count_, 0); // check if we received anything | ||
| } | ||
|
|
||
| // todo multiple publisher test? |
There was a problem hiding this comment.
Don't leave the todo. I don't think this is necessary for this pass
Added basic system tests for the rclcpp QoS features