From c38b5254de4e708bfe8c0cebd943f444009d7245 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 29 Mar 2021 17:46:14 -0700 Subject: [PATCH 1/5] Fix flaky lifecycle node tests Apparently, the topics and services that LifecycleNode provides are not available immediately after creating a node. Fix flaky tests by accounting for some delay between the LifecycleNode constructor and queries about its topics and services. Signed-off-by: Jacob Perron --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 137 ++++++++++++------ 1 file changed, 94 insertions(+), 43 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index 44ccb79390..d50a8c8e59 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -34,6 +34,84 @@ using lifecycle_msgs::msg::State; using lifecycle_msgs::msg::Transition; +static +bool wait_for_event( + std::shared_ptr node, + std::function predicate, + std::chrono::nanoseconds timeout, + std::chrono::nanoseconds sleep_period) +{ + auto start = std::chrono::steady_clock::now(); + std::chrono::microseconds time_slept(0); + + while (!predicate() && + time_slept < std::chrono::duration_cast(timeout)) + { + rclcpp::Event::SharedPtr graph_event = node->get_graph_event(); + node->wait_for_graph_change(graph_event, sleep_period); + time_slept = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start); + } + return predicate(); +} + +static +bool wait_for_topic( + std::shared_ptr node, + const std::string & topic, + std::chrono::nanoseconds timeout = std::chrono::seconds(3), + std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) +{ + return wait_for_event( + node, + [node, topic]() + { + auto topic_names_and_types = node->get_topic_names_and_types(); + return topic_names_and_types.end() != topic_names_and_types.find(topic); + }, + timeout, + sleep_period); +} + +static +bool wait_for_service( + std::shared_ptr node, + const std::string & service, + std::chrono::nanoseconds timeout = std::chrono::seconds(3), + std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) +{ + return wait_for_event( + node, + [node, service]() + { + auto service_names_and_types = node->get_service_names_and_types(); + return service_names_and_types.end() != service_names_and_types.find(service); + }, + timeout, + sleep_period); +} + +static +bool wait_for_service_by_node( + std::shared_ptr node, + const std::string & node_name, + const std::string & service, + std::chrono::nanoseconds timeout = std::chrono::seconds(3), + std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) +{ + return wait_for_event( + node, + [node, node_name, service]() + { + auto service_names_and_types_by_node = + node->get_service_names_and_types_by_node(node_name, ""); + return service_names_and_types_by_node.end() != + service_names_and_types_by_node.find(service); + }, + timeout, + sleep_period); +} + class TestDefaultStateMachine : public ::testing::Test { protected: @@ -559,10 +637,8 @@ TEST_F(TestDefaultStateMachine, test_graph_topics) { ASSERT_NE(names.end(), std::find(names.begin(), names.end(), std::string("/testnode"))); // Other topics may exist for an rclcpp::Node, but just checking the lifecycle one exists + ASSERT_TRUE(wait_for_topic(test_node, "/testnode/transition_event")); auto topic_names_and_types = test_node->get_topic_names_and_types(); - ASSERT_NE( - topic_names_and_types.end(), - topic_names_and_types.find(std::string("/testnode/transition_event"))); EXPECT_STREQ( topic_names_and_types["/testnode/transition_event"][0].c_str(), "lifecycle_msgs/msg/TransitionEvent"); @@ -580,39 +656,26 @@ TEST_F(TestDefaultStateMachine, test_graph_topics) { TEST_F(TestDefaultStateMachine, test_graph_services) { auto test_node = std::make_shared("testnode"); - auto service_names_and_types = test_node->get_service_names_and_types(); // These are specific to lifecycle nodes, other services are provided by rclcpp::Node - ASSERT_NE( - service_names_and_types.end(), - service_names_and_types.find(std::string("/testnode/change_state"))); + ASSERT_TRUE(wait_for_service(test_node, "/testnode/change_state")); + ASSERT_TRUE(wait_for_service(test_node, "/testnode/get_available_states")); + ASSERT_TRUE(wait_for_service(test_node, "/testnode/get_available_transitions")); + ASSERT_TRUE(wait_for_service(test_node, "/testnode/get_state")); + ASSERT_TRUE(wait_for_service(test_node, "/testnode/get_transition_graph")); + + auto service_names_and_types = test_node->get_service_names_and_types(); EXPECT_STREQ( service_names_and_types["/testnode/change_state"][0].c_str(), "lifecycle_msgs/srv/ChangeState"); - - ASSERT_NE( - service_names_and_types.end(), - service_names_and_types.find(std::string("/testnode/get_available_states"))); EXPECT_STREQ( service_names_and_types["/testnode/get_available_states"][0].c_str(), "lifecycle_msgs/srv/GetAvailableStates"); - - ASSERT_NE( - service_names_and_types.end(), - service_names_and_types.find(std::string("/testnode/get_available_transitions"))); EXPECT_STREQ( service_names_and_types["/testnode/get_available_transitions"][0].c_str(), "lifecycle_msgs/srv/GetAvailableTransitions"); - - ASSERT_NE( - service_names_and_types.end(), - service_names_and_types.find(std::string("/testnode/get_state"))); EXPECT_STREQ( service_names_and_types["/testnode/get_state"][0].c_str(), "lifecycle_msgs/srv/GetState"); - - ASSERT_NE( - service_names_and_types.end(), - service_names_and_types.find(std::string("/testnode/get_transition_graph"))); EXPECT_STREQ( service_names_and_types["/testnode/get_transition_graph"][0].c_str(), "lifecycle_msgs/srv/GetAvailableTransitions"); @@ -621,40 +684,28 @@ TEST_F(TestDefaultStateMachine, test_graph_services) { TEST_F(TestDefaultStateMachine, test_graph_services_by_node) { auto test_node = std::make_shared("testnode"); + // These are specific to lifecycle nodes, other services are provided by rclcpp::Node + ASSERT_TRUE(wait_for_service_by_node(test_node, "testnode", "/testnode/change_state")); + ASSERT_TRUE(wait_for_service_by_node(test_node, "testnode", "/testnode/get_available_states")); + ASSERT_TRUE( + wait_for_service_by_node(test_node, "testnode", "/testnode/get_available_transitions")); + ASSERT_TRUE(wait_for_service_by_node(test_node, "testnode", "/testnode/get_state")); + ASSERT_TRUE(wait_for_service_by_node(test_node, "testnode", "/testnode/get_transition_graph")); + auto service_names_and_types_by_node = test_node->get_service_names_and_types_by_node("testnode", ""); - // These are specific to lifecycle nodes, other services are provided by rclcpp::Node - ASSERT_NE( - service_names_and_types_by_node.end(), - service_names_and_types_by_node.find(std::string("/testnode/change_state"))); EXPECT_STREQ( service_names_and_types_by_node["/testnode/change_state"][0].c_str(), "lifecycle_msgs/srv/ChangeState"); - - ASSERT_NE( - service_names_and_types_by_node.end(), - service_names_and_types_by_node.find(std::string("/testnode/get_available_states"))); EXPECT_STREQ( service_names_and_types_by_node["/testnode/get_available_states"][0].c_str(), "lifecycle_msgs/srv/GetAvailableStates"); - - ASSERT_NE( - service_names_and_types_by_node.end(), - service_names_and_types_by_node.find(std::string("/testnode/get_available_transitions"))); EXPECT_STREQ( service_names_and_types_by_node["/testnode/get_available_transitions"][0].c_str(), "lifecycle_msgs/srv/GetAvailableTransitions"); - - ASSERT_NE( - service_names_and_types_by_node.end(), - service_names_and_types_by_node.find(std::string("/testnode/get_state"))); EXPECT_STREQ( service_names_and_types_by_node["/testnode/get_state"][0].c_str(), "lifecycle_msgs/srv/GetState"); - - ASSERT_NE( - service_names_and_types_by_node.end(), - service_names_and_types_by_node.find(std::string("/testnode/get_transition_graph"))); EXPECT_STREQ( service_names_and_types_by_node["/testnode/get_transition_graph"][0].c_str(), "lifecycle_msgs/srv/GetAvailableTransitions"); From 2c5545e400c3f329386f4d9800b338f6f62acdff Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 30 Mar 2021 11:06:27 -0700 Subject: [PATCH 2/5] minor refactor Signed-off-by: Jacob Perron --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index d50a8c8e59..0d87f29f4c 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -44,7 +44,8 @@ bool wait_for_event( auto start = std::chrono::steady_clock::now(); std::chrono::microseconds time_slept(0); - while (!predicate() && + bool predicate_result; + while (!(predicate_result = predicate()) && time_slept < std::chrono::duration_cast(timeout)) { rclcpp::Event::SharedPtr graph_event = node->get_graph_event(); @@ -52,7 +53,7 @@ bool wait_for_event( time_slept = std::chrono::duration_cast( std::chrono::steady_clock::now() - start); } - return predicate(); + return predicate_result; } static From 06341d26e5920768b2d526b67e16a392801861c4 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 30 Mar 2021 11:15:39 -0700 Subject: [PATCH 3/5] Uncrustify Signed-off-by: Jacob Perron --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index 0d87f29f4c..1962b7c3a9 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -64,14 +64,14 @@ bool wait_for_topic( std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) { return wait_for_event( - node, - [node, topic]() - { - auto topic_names_and_types = node->get_topic_names_and_types(); - return topic_names_and_types.end() != topic_names_and_types.find(topic); - }, - timeout, - sleep_period); + node, + [node, topic]() + { + auto topic_names_and_types = node->get_topic_names_and_types(); + return topic_names_and_types.end() != topic_names_and_types.find(topic); + }, + timeout, + sleep_period); } static @@ -82,14 +82,14 @@ bool wait_for_service( std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) { return wait_for_event( - node, - [node, service]() - { - auto service_names_and_types = node->get_service_names_and_types(); - return service_names_and_types.end() != service_names_and_types.find(service); - }, - timeout, - sleep_period); + node, + [node, service]() + { + auto service_names_and_types = node->get_service_names_and_types(); + return service_names_and_types.end() != service_names_and_types.find(service); + }, + timeout, + sleep_period); } static @@ -101,16 +101,16 @@ bool wait_for_service_by_node( std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) { return wait_for_event( - node, - [node, node_name, service]() - { - auto service_names_and_types_by_node = - node->get_service_names_and_types_by_node(node_name, ""); - return service_names_and_types_by_node.end() != - service_names_and_types_by_node.find(service); - }, - timeout, - sleep_period); + node, + [node, node_name, service]() + { + auto service_names_and_types_by_node = node->get_service_names_and_types_by_node( + node_name, ""); + return service_names_and_types_by_node.end() != service_names_and_types_by_node.find( + service); + }, + timeout, + sleep_period); } class TestDefaultStateMachine : public ::testing::Test From 71c392a4b7edcea1c5fde1b11d61b20a9b166482 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 30 Mar 2021 11:40:29 -0700 Subject: [PATCH 4/5] Make default timer values constants Signed-off-by: Jacob Perron --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index 1962b7c3a9..c2a1292aff 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -34,6 +34,9 @@ using lifecycle_msgs::msg::State; using lifecycle_msgs::msg::Transition; +static const std::chrono::nanoseconds DEFAULT_EVENT_TIMEOUT = std::chrono::seconds(3); +static const std::chrono::nanoseconds DEFAULT_EVENT_SLEEP_PERIOD = std::chrono::milliseconds(100); + static bool wait_for_event( std::shared_ptr node, @@ -60,8 +63,8 @@ static bool wait_for_topic( std::shared_ptr node, const std::string & topic, - std::chrono::nanoseconds timeout = std::chrono::seconds(3), - std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) + std::chrono::nanoseconds timeout = DEFAULT_EVENT_TIMEOUT, + std::chrono::nanoseconds sleep_period = DEFAULT_EVENT_SLEEP_PERIOD) { return wait_for_event( node, @@ -78,8 +81,8 @@ static bool wait_for_service( std::shared_ptr node, const std::string & service, - std::chrono::nanoseconds timeout = std::chrono::seconds(3), - std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) + std::chrono::nanoseconds timeout = DEFAULT_EVENT_TIMEOUT, + std::chrono::nanoseconds sleep_period = DEFAULT_EVENT_SLEEP_PERIOD) { return wait_for_event( node, From f113f84d47046500464293ab035ea827e298a672 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 30 Mar 2021 11:52:31 -0700 Subject: [PATCH 5/5] Use default values in wait_for_service_by_node Signed-off-by: Jacob Perron --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index c2a1292aff..7f39b3e7cc 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -100,8 +100,8 @@ bool wait_for_service_by_node( std::shared_ptr node, const std::string & node_name, const std::string & service, - std::chrono::nanoseconds timeout = std::chrono::seconds(3), - std::chrono::nanoseconds sleep_period = std::chrono::milliseconds(100)) + std::chrono::nanoseconds timeout = DEFAULT_EVENT_TIMEOUT, + std::chrono::nanoseconds sleep_period = DEFAULT_EVENT_SLEEP_PERIOD) { return wait_for_event( node,