From 79d70612135a2edc84d5f7e6d9943f6e53951707 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 5 Jan 2021 21:30:06 +0000 Subject: [PATCH 1/3] Make sure to wait for graph change events in test_node_graph. While debugging something else, I came across this fairly rare flake in the test_node_graph tests. Essentially, it may take some time for node changes to propagate into the graph. If the NodeGraph client asks for data before the changes are in the graph, they may get something they don't expect. The fix is to wait for an event on the NodeGraph to happen, and then look for the data you are interested in. Before this change, I could get the flake to happen on a loaded aarch64 system fairly regularly. After this change, I can no longer make the flake happen on a loaded system. Signed-off-by: Chris Lalancette --- .../node_interfaces/test_node_graph.cpp | 67 ++++++++++++++++--- 1 file changed, 57 insertions(+), 10 deletions(-) diff --git a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp index cff77e9ae7..0f19346e98 100644 --- a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp +++ b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp @@ -66,6 +66,47 @@ class TestNodeGraph : public ::testing::Test const rclcpp::node_interfaces::NodeGraph * node_graph() const {return node_graph_;} + size_t get_num_topics() + { + constexpr std::chrono::milliseconds timeout(100); + + int tries = 0; + size_t num_topics = 0; + while (num_topics < 1u && tries++ < 5) { + auto event = node()->get_graph_event(); + EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); + + auto topic_names_and_types = node_graph()->get_topic_names_and_types(); + num_topics = topic_names_and_types.size(); + } + + // Run notify_graph_change() here to remove the graph_event users we created + // in the loop above. + node_graph_->notify_graph_change(); + + return num_topics; + } + + size_t get_num_services() + { + constexpr std::chrono::milliseconds timeout(100); + + int tries = 0; + size_t num_services = 0; + while (num_services < 1u && tries++ < 5) { + auto event = node()->get_graph_event(); + EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); + + auto service_names_and_types = node_graph()->get_service_names_and_types(); + num_services = service_names_and_types.size(); + } + + // Run notify_graph_change() here to remove the graph_event users we created + // in the loop above. + node_graph_->notify_graph_change(); + return num_services; + } + private: std::shared_ptr node_; rclcpp::node_interfaces::NodeGraph * node_graph_; @@ -73,11 +114,9 @@ class TestNodeGraph : public ::testing::Test TEST_F(TestNodeGraph, construct_from_node) { - auto topic_names_and_types = node_graph()->get_topic_names_and_types(false); - EXPECT_LT(0u, topic_names_and_types.size()); + EXPECT_LT(0u, get_num_topics()); - auto service_names_and_types = node_graph()->get_service_names_and_types(); - EXPECT_LT(0u, service_names_and_types.size()); + EXPECT_LT(0u, get_num_services()); auto names = node_graph()->get_node_names(); EXPECT_EQ(1u, names.size()); @@ -96,8 +135,7 @@ TEST_F(TestNodeGraph, construct_from_node) TEST_F(TestNodeGraph, get_topic_names_and_types) { - auto topic_names_and_types = node_graph()->get_topic_names_and_types(); - EXPECT_LT(0u, topic_names_and_types.size()); + ASSERT_LT(0u, get_num_topics()); } TEST_F(TestNodeGraph, get_topic_names_and_types_rcl_error) @@ -126,8 +164,7 @@ TEST_F(TestNodeGraph, get_topic_names_and_types_rcl_names_and_types_fini_error) TEST_F(TestNodeGraph, get_service_names_and_types) { - auto service_names_and_types = node_graph()->get_service_names_and_types(); - EXPECT_LT(0u, service_names_and_types.size()); + ASSERT_LT(0u, get_num_services()); } TEST_F(TestNodeGraph, get_service_names_and_types_rcl_error) @@ -309,8 +346,18 @@ TEST_F(TestNodeGraph, get_info_by_topic) EXPECT_EQ(0u, node_graph()->get_publishers_info_by_topic("topic", true).size()); - auto publishers = node_graph()->get_publishers_info_by_topic("topic", false); - ASSERT_EQ(1u, publishers.size()); + constexpr std::chrono::milliseconds timeout(100); + int tries = 0; + size_t num_publishers = 0; + std::vector publishers; + while (num_publishers < 1u && tries++ < 5) { + auto event = node()->get_graph_event(); + EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); + + publishers = node_graph()->get_publishers_info_by_topic("topic", false); + num_publishers = publishers.size(); + } + ASSERT_EQ(1u, num_publishers); auto publisher_endpoint_info = publishers[0]; const auto const_publisher_endpoint_info = publisher_endpoint_info; From 71fc062814566db454ad09f3f133542146d83281 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 5 Jan 2021 22:04:11 +0000 Subject: [PATCH 2/3] cpplint fixes. Signed-off-by: Chris Lalancette --- rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp index 0f19346e98..9cbde5a817 100644 --- a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp +++ b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include "rcl/graph.h" #include "rcl/node_options.h" From a6f6a74828a590751843887021bac0c0825be5b8 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 5 Jan 2021 22:59:08 +0000 Subject: [PATCH 3/3] Introduce get_num_graph_things(). This takes a predicate in and allows us to remove some duplicated code. Signed-off-by: Chris Lalancette --- .../node_interfaces/test_node_graph.cpp | 64 +++++++++---------- 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp index 9cbde5a817..0d2d90c9a5 100644 --- a/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp +++ b/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp @@ -14,6 +14,7 @@ #include +#include #include #include #include @@ -67,45 +68,45 @@ class TestNodeGraph : public ::testing::Test const rclcpp::node_interfaces::NodeGraph * node_graph() const {return node_graph_;} - size_t get_num_topics() + size_t get_num_graph_things(std::function predicate) { constexpr std::chrono::milliseconds timeout(100); - int tries = 0; - size_t num_topics = 0; - while (num_topics < 1u && tries++ < 5) { + size_t tries = 0; + size_t num_things = 0; + while (tries++ < 5) { + num_things = predicate(); + if (num_things >= 1) { + break; + } + auto event = node()->get_graph_event(); EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); - - auto topic_names_and_types = node_graph()->get_topic_names_and_types(); - num_topics = topic_names_and_types.size(); } // Run notify_graph_change() here to remove the graph_event users we created // in the loop above. node_graph_->notify_graph_change(); - return num_topics; + return num_things; } - size_t get_num_services() + size_t get_num_topics() { - constexpr std::chrono::milliseconds timeout(100); - - int tries = 0; - size_t num_services = 0; - while (num_services < 1u && tries++ < 5) { - auto event = node()->get_graph_event(); - EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); - - auto service_names_and_types = node_graph()->get_service_names_and_types(); - num_services = service_names_and_types.size(); - } + return get_num_graph_things( + [this]() -> size_t { + auto topic_names_and_types = node_graph()->get_topic_names_and_types(); + return topic_names_and_types.size(); + }); + } - // Run notify_graph_change() here to remove the graph_event users we created - // in the loop above. - node_graph_->notify_graph_change(); - return num_services; + size_t get_num_services() + { + return get_num_graph_things( + [this]() -> size_t { + auto service_names_and_types = node_graph()->get_service_names_and_types(); + return service_names_and_types.size(); + }); } private: @@ -347,17 +348,12 @@ TEST_F(TestNodeGraph, get_info_by_topic) EXPECT_EQ(0u, node_graph()->get_publishers_info_by_topic("topic", true).size()); - constexpr std::chrono::milliseconds timeout(100); - int tries = 0; - size_t num_publishers = 0; std::vector publishers; - while (num_publishers < 1u && tries++ < 5) { - auto event = node()->get_graph_event(); - EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout)); - - publishers = node_graph()->get_publishers_info_by_topic("topic", false); - num_publishers = publishers.size(); - } + size_t num_publishers = get_num_graph_things( + [this, &publishers]() { + publishers = node_graph()->get_publishers_info_by_topic("topic", false); + return publishers.size(); + }); ASSERT_EQ(1u, num_publishers); auto publisher_endpoint_info = publishers[0];