Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added missing tests for rclcpp lifecycle #1240

Merged
merged 5 commits into from
Aug 12, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 27, 2020

  • remove_on_set_parameters_callback
  • notify_graph_change
  • get_service_names_and_types_by_node

Related to this https://ci.ros2.org/job/nightly_linux_coverage/lastBuild/cobertura/src_ros2_rclcpp_rclcpp_lifecycle_src/lifecycle_node_cpp/lifecycle_node_cpp/

Signed-off-by: ahcorde ahcorde@gmail.com

 - remove_on_set_parameters_callback
 - notify_graph_change
 - get_service_names_and_types_by_node

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added the enhancement New feature or request label Jul 27, 2020
@ahcorde ahcorde requested a review from brawner July 27, 2020 08:54
@ahcorde ahcorde self-assigned this Jul 27, 2020
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 27, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@brawner brawner changed the title Added missing test rclcpp lifecycle Added missing tests for rclcpp lifecycle Jul 27, 2020
rclcpp_lifecycle/test/test_lifecycle_service_client.cpp Outdated Show resolved Hide resolved
{
auto node = std::make_shared<LifecycleServiceClient>("client_wait_for_graph_change");
auto * node_graph =
dynamic_cast<rclcpp::node_interfaces::NodeGraph *>(node->get_node_graph_interface().get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the dynamic_cast necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy this line from the original test

const auto * node_graph =
dynamic_cast<rclcpp::node_interfaces::NodeGraph *>(node->get_node_graph_interface().get());

Copy link
Contributor

@brawner brawner Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That test was for node_interfaces explicitly, and I wanted to verify that the cast to NodeGraph* was doing what I thought it was.

I think for this test, the std::shared_ptr<NodeGraphInterface> from get_node_graph_interface() should be sufficient.

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from brawner July 28, 2020 19:46
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 29, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ahcorde ahcorde requested a review from brawner August 10, 2020 07:10
Copy link
Contributor

@brawner brawner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this review slipped through the cracks

@@ -355,3 +356,52 @@ TEST_F(TestLifecycleServiceClient, lifecycle_transitions) {
transitions = lifecycle_client()->get_available_transitions();
EXPECT_EQ(transitions.size(), 0u);
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra line

@ahcorde
Copy link
Contributor Author

ahcorde commented Aug 12, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/test/add_missing_rclcpp_lifecycle branch from 3b4bfba to 49dd1bd Compare August 12, 2020 07:46
@ahcorde ahcorde requested a review from brawner August 12, 2020 08:07
@ahcorde ahcorde merged commit 42682d1 into master Aug 12, 2020
peterpena pushed a commit that referenced this pull request Aug 18, 2020
* Added missing test rclcpp lifecycle
 - remove_on_set_parameters_callback
 - notify_graph_change
 - get_service_names_and_types_by_node

Signed-off-by: ahcorde <ahcorde@gmail.com>

* omit the name of the argument in lambda function

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Removed extra line

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Pedro Pena <peter.a.pena@gmail.com>
ahcorde added a commit that referenced this pull request Oct 22, 2020
* Added missing test rclcpp lifecycle
 - remove_on_set_parameters_callback
 - notify_graph_change
 - get_service_names_and_types_by_node

Signed-off-by: ahcorde <ahcorde@gmail.com>

* omit the name of the argument in lambda function

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Removed extra line

Signed-off-by: ahcorde <ahcorde@gmail.com>
ahcorde added a commit that referenced this pull request Nov 3, 2020
* Added missing test rclcpp lifecycle
 - remove_on_set_parameters_callback
 - notify_graph_change
 - get_service_names_and_types_by_node

Signed-off-by: ahcorde <ahcorde@gmail.com>

* omit the name of the argument in lambda function

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added feedback

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Removed extra line

Signed-off-by: ahcorde <ahcorde@gmail.com>
@clalancette clalancette deleted the ahcorde/test/add_missing_rclcpp_lifecycle branch January 15, 2021 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants