Properly test get_service_names_and_types_by_node in rclcpp_lifecycle#2599
Merged
christophebedard merged 1 commit intorollingfrom Aug 8, 2024
Conversation
3d43045 to
d2c5a49
Compare
Member
Author
|
I thought that maybe this should instead test |
Collaborator
|
A small suggestion. The current test code only checks whether LifecycleNode::get_service_names_and_types_by_node() is called successfully. Should we pick services (such as /lc_talker/get_state, /lc_talker/change_state) to check if their names exist and their types are correct ? |
2e86fc6 to
6221368
Compare
Member
Author
|
good idea, done. |
The current `TestLifecycleServiceClient.get_service_names_and_types_by_node` test was using `LifecycleServiceClient`, which is just a normal `rclcpp::Node` with some `rclcpp::Client`s, to test `NodeGraph::get_service_names_and_types_by_node()`. However, `NodeGraph::get_service_names_and_types_by_node()` is for service servers, not clients. The test just ended up checking the built-in services of an `rclcpp::Node`, since it wasn't actually checking the names of the services, and not checking the clients of the `LifecycleServiceClient` or the built-in services of a `rclcpp_lifecycle::LifecycleNode`. I believe this was probably not the intention, since this is an `rclcpp_lifecycle` test. Instead, use an actual `rclcpp_lifecycle::LifecycleNode` and check its service servers by calling `NodeGraph::get_service_names_and_types_by_node()` through `LifecycleNode::get_service_names_and_types_by_node()`. Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
6221368 to
cccc995
Compare
fujitatomoya
approved these changes
Aug 8, 2024
Collaborator
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current
TestLifecycleServiceClient.get_service_names_and_types_by_nodetest was usingLifecycleServiceClient, which is just a normalrclcpp::Nodewith somerclcpp::Clients, to testNodeGraph::get_service_names_and_types_by_node(). However,NodeGraph::get_service_names_and_types_by_node()is for service servers, not clients. The test just ended up checking the built-in services of anrclcpp::Node, since it wasn't actually checking the names of the services, and not checking the clients of theLifecycleServiceClientor the built-in services of arclcpp_lifecycle::LifecycleNode. I believe this was probably not the intention, since this is anrclcpp_lifecycletest.Instead, use an actual
rclcpp_lifecycle::LifecycleNodeand check its service servers by callingNodeGraph::get_service_names_and_types_by_node()throughLifecycleNode::get_service_names_and_types_by_node().