Increase test timeouts of slow running tests with rmw_connext_cpp (#1400

) * Increase test timeouts of slow running tests with rmw_connext_cpp Signed-off-by: Stephen Brawner <brawner@gmail.com> * Fix other issues with connext Signed-off-by: Stephen Brawner <brawner@gmail.com>
ros2 · Oct 14, 2020 · 31c202e · 31c202e
1 parent b5b8782
commit 31c202e
Show file tree

Hide file tree

Showing 9 changed files with 58 additions and 11 deletions.
diff --git a/rclcpp/test/CMakeLists.txt b/rclcpp/test/CMakeLists.txt
@@ -102,7 +102,9 @@ if(TARGET test_create_subscription)
     "test_msgs"
   )
 endif()
-ament_add_gtest(test_add_callback_groups_to_executor rclcpp/test_add_callback_groups_to_executor.cpp)
+ament_add_gtest(test_add_callback_groups_to_executor
+  rclcpp/test_add_callback_groups_to_executor.cpp
+  TIMEOUT 120)
 if(TARGET test_add_callback_groups_to_executor)
   target_link_libraries(test_add_callback_groups_to_executor ${PROJECT_NAME})
   ament_target_dependencies(test_add_callback_groups_to_executor
@@ -214,7 +216,8 @@ if(TARGET test_node_interfaces__node_clock)
   target_link_libraries(test_node_interfaces__node_clock ${PROJECT_NAME})
 endif()
 ament_add_gtest(test_node_interfaces__node_graph
-  rclcpp/node_interfaces/test_node_graph.cpp)
+  rclcpp/node_interfaces/test_node_graph.cpp
+  TIMEOUT 120)
 if(TARGET test_node_interfaces__node_graph)
   ament_target_dependencies(
     test_node_interfaces__node_graph
@@ -329,7 +332,7 @@ ament_add_gtest(test_parameter_map rclcpp/test_parameter_map.cpp)
 if(TARGET test_parameter_map)
   target_link_libraries(test_parameter_map ${PROJECT_NAME})
 endif()
-ament_add_gtest(test_publisher rclcpp/test_publisher.cpp)
+ament_add_gtest(test_publisher rclcpp/test_publisher.cpp TIMEOUT 120)
 if(TARGET test_publisher)
   ament_target_dependencies(test_publisher
     "rcl"
@@ -412,7 +415,8 @@ if(TARGET test_service)
   )
   target_link_libraries(test_service ${PROJECT_NAME} mimick)
 endif()
-ament_add_gtest(test_subscription rclcpp/test_subscription.cpp)
+# Creating and destroying nodes is slow with Connext, so this needs larger timeout.
+ament_add_gtest(test_subscription rclcpp/test_subscription.cpp TIMEOUT 120)
 if(TARGET test_subscription)
   ament_target_dependencies(test_subscription
     "rcl_interfaces"
@@ -565,7 +569,7 @@ if(TARGET test_multi_threaded_executor)
 endif()
 
 ament_add_gtest(test_static_executor_entities_collector rclcpp/executors/test_static_executor_entities_collector.cpp
-  APPEND_LIBRARY_DIRS "${append_library_dirs}")
+  APPEND_LIBRARY_DIRS "${append_library_dirs}" TIMEOUT 120)
 if(TARGET test_static_executor_entities_collector)
   ament_target_dependencies(test_static_executor_entities_collector
     "rcl"
@@ -647,7 +651,7 @@ endif()
 
 ament_add_gtest(test_executor rclcpp/test_executor.cpp
   APPEND_LIBRARY_DIRS "${append_library_dirs}"
-)
+  TIMEOUT 120)
 if(TARGET test_executor)
   ament_target_dependencies(test_executor "rcl")
   target_link_libraries(test_executor ${PROJECT_NAME} mimick)

diff --git a/rclcpp/test/rclcpp/test_add_callback_groups_to_executor.cpp b/rclcpp/test/rclcpp/test_add_callback_groups_to_executor.cpp
@@ -30,6 +30,10 @@
 #include "rclcpp/executor.hpp"
 #include "rclcpp/rclcpp.hpp"
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution]
+
 using namespace std::chrono_literals;
 
 template<typename T>

diff --git a/rclcpp/test/rclcpp/test_publisher.cpp b/rclcpp/test/rclcpp/test_publisher.cpp
@@ -29,6 +29,10 @@
 
 #include "test_msgs/msg/empty.hpp"
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution
+
 class TestPublisher : public ::testing::Test
 {
 public:
@@ -274,6 +278,9 @@ TEST_F(TestPublisher, serialized_message_publish) {
   auto publisher = node->create_publisher<test_msgs::msg::Empty>("topic", 10, options);
 
   rclcpp::SerializedMessage serialized_msg;
+  // Mock successful rcl publish because the serialized_msg above is poorly formed
+  auto mock = mocking_utils::patch_and_return(
+    "self", rcl_publish_serialized_message, RCL_RET_OK);
   EXPECT_NO_THROW(publisher->publish(serialized_msg));
 
   EXPECT_NO_THROW(publisher->publish(serialized_msg.get_rcl_serialized_message()));
@@ -411,14 +418,22 @@ TEST_F(TestPublisher, inter_process_publish_failures) {
     EXPECT_THROW(publisher->publish(msg), rclcpp::exceptions::RCLError);
   }
 
-  rclcpp::SerializedMessage serialized_msg;
-  EXPECT_NO_THROW(publisher->publish(serialized_msg));
+  {
+    // Using 'self' instead of 'lib:rclcpp' because `rcl_publish_serialized_message` is entirely
+    // defined in a header. Also, this one requires mocking because the serialized_msg is poorly
+    // formed and this just tests rclcpp functionality.
+    auto mock = mocking_utils::patch_and_return(
+      "self", rcl_publish_serialized_message, RCL_RET_OK);
+    rclcpp::SerializedMessage serialized_msg;
+    EXPECT_NO_THROW(publisher->publish(serialized_msg));
+  }
 
   {
     // Using 'self' instead of 'lib:rclcpp' because `rcl_publish_serialized_message` is entirely
     // defined in a header
     auto mock = mocking_utils::patch_and_return(
       "self", rcl_publish_serialized_message, RCL_RET_ERROR);
+    rclcpp::SerializedMessage serialized_msg;
     EXPECT_THROW(publisher->publish(serialized_msg), rclcpp::exceptions::RCLError);
   }
 

diff --git a/rclcpp/test/rclcpp/test_subscription.cpp b/rclcpp/test/rclcpp/test_subscription.cpp
@@ -29,6 +29,10 @@
 
 #include "test_msgs/msg/empty.hpp"
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution
+
 using namespace std::chrono_literals;
 
 class TestSubscription : public ::testing::Test

diff --git a/rclcpp_action/CMakeLists.txt b/rclcpp_action/CMakeLists.txt
@@ -86,7 +86,7 @@ if(BUILD_TESTING)
     )
   endif()
 
-  ament_add_gtest(test_server test/test_server.cpp)
+  ament_add_gtest(test_server test/test_server.cpp TIMEOUT 180)
   if(TARGET test_server)
     ament_target_dependencies(test_server
       "rcutils"

diff --git a/rclcpp_action/test/test_server.cpp b/rclcpp_action/test/test_server.cpp
@@ -29,6 +29,10 @@
 #include "rclcpp_action/server.hpp"
 #include "./mocking_utils/patch.hpp"
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution
+
 using Fibonacci = test_msgs::action::Fibonacci;
 using CancelResponse = typename Fibonacci::Impl::CancelGoalService::Response;
 using GoalUUID = rclcpp_action::GoalUUID;

diff --git a/rclcpp_lifecycle/CMakeLists.txt b/rclcpp_lifecycle/CMakeLists.txt
@@ -51,7 +51,7 @@ if(BUILD_TESTING)
   set(ament_cmake_cppcheck_ADDITIONAL_INCLUDE_DIRS ${rclcpp_INCLUDE_DIRS})
   ament_lint_auto_find_test_dependencies()
 
-  ament_add_gtest(test_lifecycle_node test/test_lifecycle_node.cpp)
+  ament_add_gtest(test_lifecycle_node test/test_lifecycle_node.cpp TIMEOUT 120)
   if(TARGET test_lifecycle_node)
     ament_target_dependencies(test_lifecycle_node
       "rcl_lifecycle"
@@ -69,7 +69,7 @@ if(BUILD_TESTING)
     )
     target_link_libraries(test_lifecycle_publisher ${PROJECT_NAME})
   endif()
-  ament_add_gtest(test_lifecycle_service_client test/test_lifecycle_service_client.cpp)
+  ament_add_gtest(test_lifecycle_service_client test/test_lifecycle_service_client.cpp TIMEOUT 120)
   if(TARGET test_lifecycle_service_client)
     ament_target_dependencies(test_lifecycle_service_client
       "rcl_lifecycle"

diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp
@@ -31,6 +31,10 @@
 
 #include "./mocking_utils/patch.hpp"
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution
+
 using lifecycle_msgs::msg::State;
 using lifecycle_msgs::msg::Transition;
 

diff --git a/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp b/rclcpp_lifecycle/test/test_lifecycle_service_client.cpp
@@ -52,6 +52,10 @@ constexpr char const * node_get_transition_graph_topic =
   "/lc_talker/get_transition_graph";
 const lifecycle_msgs::msg::State unknown_state = lifecycle_msgs::msg::State();
 
+// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
+// that this test can complete fully, or adjust the timeout as necessary.
+// See https://github.com/ros2/rmw_connext/issues/325 for resolution
+
 class EmptyLifecycleNode : public rclcpp_lifecycle::LifecycleNode
 {
 public:
@@ -372,6 +376,14 @@ TEST_F(TestLifecycleServiceClient, get_service_names_and_types_by_node)
     std::runtime_error);
   auto service_names_and_types1 = node_graph->get_service_names_and_types_by_node("client1", "/");
   auto service_names_and_types2 = node_graph->get_service_names_and_types_by_node("client2", "/");
+  auto start = std::chrono::steady_clock::now();
+  while (0 == service_names_and_types1.size() ||
+    service_names_and_types1.size() != service_names_and_types2.size() ||
+    (std::chrono::steady_clock::now() - start) < std::chrono::seconds(1))
+  {
+    service_names_and_types1 = node_graph->get_service_names_and_types_by_node("client1", "/");
+    service_names_and_types2 = node_graph->get_service_names_and_types_by_node("client2", "/");
+  }
   EXPECT_EQ(service_names_and_types1.size(), service_names_and_types2.size());
 }