Use wait_for_service to make Service tests less flaky (#132)

* use wait_for_service to make tests less flaky * realign timeouts * avoid using wait_for_service with fastrtps this can be undone once fastrtps supports wait_for_service * [test_communication] avoid wait_for_service with fastrtps it can be undone once fastrtps supports wait_for_service * add test to ensure wait_for_service wakes after shutdown/sigint
ros2 · Jun 23, 2016 · a66ae71 · a66ae71
1 parent fe70b33
commit a66ae71
Show file tree

Hide file tree

Showing 10 changed files with 180 additions and 13 deletions.
diff --git a/.gitignore b/.gitignore
@@ -0,0 +1 @@
+*.pyc
diff --git a/test_communication/CMakeLists.txt b/test_communication/CMakeLists.txt
@@ -134,7 +134,10 @@ if(BUILD_TESTING)
       # TODO(dirk-thomas) publishing DynamicArrayPrimitives in OpenSplice is buggy
       # https://github.com/ros2/rclcpp/issues/219
       if(NOT TEST_MESSAGE_TYPE STREQUAL "DynamicArrayPrimitives" OR NOT rmw_implementation1 STREQUAL "rmw_opensplice_cpp")
-        ament_add_nose_test(publisher_subscriber_cpp${test_suffix} "${CMAKE_CURRENT_BINARY_DIR}/test_publisher_subscriber_cpp${test_suffix}_$<CONFIGURATION>.py" TIMEOUT 15)
+        ament_add_nose_test(
+          publisher_subscriber_cpp${test_suffix}
+          "${CMAKE_CURRENT_BINARY_DIR}/test_publisher_subscriber_cpp${test_suffix}_$<CONFIGURATION>.py"
+          TIMEOUT 15)
         set_tests_properties(
           publisher_subscriber_cpp${test_suffix}
           PROPERTIES DEPENDS "test_publisher_cpp__${rmw_implementation1};test_subscriber_cpp__${rmw_implementation2}"
@@ -166,10 +169,15 @@ if(BUILD_TESTING)
         INPUT "${CMAKE_CURRENT_BINARY_DIR}/test_requester_replier_cpp${test_suffix}.py.configured"
       )
 
-      ament_add_nose_test(requester_replier_cpp${test_suffix} "${CMAKE_CURRENT_BINARY_DIR}/test_requester_replier_cpp${test_suffix}_$<CONFIGURATION>.py" TIMEOUT 15)
+      ament_add_nose_test(
+        requester_replier_cpp${test_suffix}
+        "${CMAKE_CURRENT_BINARY_DIR}/test_requester_replier_cpp${test_suffix}_$<CONFIGURATION>.py"
+        TIMEOUT 30
+      )
       set_tests_properties(
         requester_replier_cpp${test_suffix}
-        PROPERTIES DEPENDS "test_requester_cpp__${rmw_implementation1};test_replier_cpp__${rmw_implementation2}"
+        PROPERTIES DEPENDS
+          "test_requester_cpp__${rmw_implementation1};test_replier_cpp__${rmw_implementation2}"
       )
     endforeach()
     endif()

diff --git a/test_communication/test/test_requester.cpp b/test_communication/test/test_requester.cpp
@@ -14,11 +14,14 @@
 
 #include <chrono>
 #include <iostream>
+#include <stdexcept>
 #include <string>
+#include <thread>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 #include <utility>
 #include <vector>
 
 #include "rclcpp/rclcpp.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "service_fixtures.hpp"
 
@@ -35,14 +38,22 @@ int request(
   size_t number_of_cycles = 5)
 {
   int rc = 0;
-  auto start = std::chrono::steady_clock::now();
-
   auto requester = node->create_client<T>(std::string("test_service_") + service_type);
+  {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+    if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+      if (!requester->wait_for_service(20_s)) {
+        throw std::runtime_error("requester service not available after waiting");
+      }
+    } else {
+      std::this_thread::sleep_for(1_s);
+    }
+  }
 
   rclcpp::WallRate cycle_rate(1);
   auto wait_between_services = std::chrono::milliseconds(100);
   size_t cycle_index = 0;
   size_t service_index = 0;
+  auto start = std::chrono::steady_clock::now();
   // publish the first request up to number_of_cycles times, longer sleep between each cycle
   // publish all requests one by one, shorter sleep between each request
   while (rclcpp::ok() && cycle_index < number_of_cycles && service_index < services.size()) {

diff --git a/test_rclcpp/CMakeLists.txt b/test_rclcpp/CMakeLists.txt
@@ -91,9 +91,14 @@ if(BUILD_TESTING)
       OUTPUT "${test_name}${target_suffix}_$<CONFIGURATION>.py"
       INPUT "${CMAKE_CURRENT_BINARY_DIR}/${test_name}${target_suffix}.py.configure"
     )
-    ament_add_nose_test(${test_name}${target_suffix} "${CMAKE_CURRENT_BINARY_DIR}/${test_name}${target_suffix}_$<CONFIGURATION>.py" ${ARGN}
-      ENV RCL_ASSERT_RMW_ID_MATCHES=${rmw_implementation})
-    set_tests_properties(${test_name}${target_suffix} PROPERTIES DEPENDS "${executable1}${target_suffix} ${executable2}${target_suffix}")
+    ament_add_nose_test(${test_name}${target_suffix}
+      "${CMAKE_CURRENT_BINARY_DIR}/${test_name}${target_suffix}_$<CONFIGURATION>.py"
+      ${ARGN}
+      ENV RCL_ASSERT_RMW_ID_MATCHES=${rmw_implementation}
+    )
+    set_tests_properties(${test_name}${target_suffix}
+      PROPERTIES DEPENDS "${executable1}${target_suffix} ${executable2}${target_suffix}"
+    )
   endmacro()
 
   macro(targets)
@@ -102,6 +107,9 @@ if(BUILD_TESTING)
     custom_gtest(gtest_publisher
       "test/test_publisher.cpp"
       TIMEOUT 15)
+    custom_gtest(gtest_client_wait_for_service_shutdown
+      "test/test_client_wait_for_service_shutdown.cpp"
+      TIMEOUT 15)
     custom_gtest(gtest_executor
       "test/test_executor.cpp"
       TIMEOUT 30)
@@ -116,7 +124,7 @@ if(BUILD_TESTING)
       TIMEOUT 30)
     custom_gtest(gtest_multiple_service_calls
       "test/test_multiple_service_calls.cpp"
-      TIMEOUT 15)
+      TIMEOUT 30)
     custom_gtest(gtest_timer
       "test/test_timer.cpp"
       TIMEOUT 30)
@@ -163,7 +171,7 @@ if(BUILD_TESTING)
       ${GTEST_LIBRARIES})
     custom_launch_test(test_client_scope_cpp
       test_client_scope_server_cpp test_client_scope_client_cpp
-      TIMEOUT 15)
+      TIMEOUT 30)
   endmacro()
 
   call_for_each_rmw_implementation(targets)

diff --git a/test_rclcpp/test/test_client_scope_client.cpp b/test_rclcpp/test/test_client_scope_client.cpp
@@ -14,9 +14,12 @@
 
 #include <iostream>
 #include <memory>
+#include <string>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
+#include <thread>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "gtest/gtest.h"
 #include "rclcpp/rclcpp.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 #include "test_rclcpp/srv/add_two_ints.hpp"
 
 #ifdef RMW_IMPLEMENTATION
@@ -34,6 +37,13 @@ TEST(CLASSNAME(service_client, RMW_IMPLEMENTATION), client_scope_regression_test
     printf("creating first client\n");
     std::cout.flush();
     auto client1 = node->create_client<test_rclcpp::srv::AddTwoInts>("client_scope");
+    {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+      if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+        ASSERT_TRUE(client1->wait_for_service(20_s)) << "service not available after waiting";
+      } else {
+        std::this_thread::sleep_for(1_s);
+      }
+    }
     auto request1 = std::make_shared<test_rclcpp::srv::AddTwoInts::Request>();
     request1->a = 1;
     request1->b = 2;
@@ -58,6 +68,13 @@ TEST(CLASSNAME(service_client, RMW_IMPLEMENTATION), client_scope_regression_test
     printf("creating second client\n");
     std::cout.flush();
     auto client2 = node->create_client<test_rclcpp::srv::AddTwoInts>("client_scope");
+    {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+      if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+        ASSERT_TRUE(client2->wait_for_service(20_s)) << "service not available after waiting";
+      } else {
+        std::this_thread::sleep_for(1_s);
+      }
+    }
     auto request2 = std::make_shared<test_rclcpp::srv::AddTwoInts::Request>();
     request2->a = 2;
     request2->b = 3;

diff --git a/test_rclcpp/test/test_client_wait_for_service_shutdown.cpp b/test_rclcpp/test/test_client_wait_for_service_shutdown.cpp
@@ -0,0 +1,51 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <chrono>
+#include <string>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
+#include <thread>
+
+#include "gtest/gtest.h"
+#include "rclcpp/rclcpp.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
+#include "test_rclcpp/srv/add_two_ints.hpp"
+
+#ifdef RMW_IMPLEMENTATION
+# define CLASSNAME_(NAME, SUFFIX) NAME ## __ ## SUFFIX
+# define CLASSNAME(NAME, SUFFIX) CLASSNAME_(NAME, SUFFIX)
+#else
+# define CLASSNAME(NAME, SUFFIX) NAME
+#endif
+
+// rclcpp::shutdown() should wake up wait_for_service, even without spin.
+TEST(CLASSNAME(service_client, RMW_IMPLEMENTATION), wait_for_service_shutdown) {
+  // TODO(wjwwood): remove this "skip" when fastrtps supports wait_for_service.
+  if (std::string(rmw_get_implementation_identifier()) == "rmw_fastrtps_cpp") {
+    return;
+  }
+  rclcpp::init(0, nullptr);
+  auto node = rclcpp::node::Node::make_shared("wait_for_service_shutdown");
+
+  auto client = node->create_client<test_rclcpp::srv::AddTwoInts>("wait_for_service_shutdown");
+
+  auto shutdown_thread = std::thread([]() {
+    std::this_thread::sleep_for(1_s);
+    rclcpp::shutdown();
+  });
+  auto start = std::chrono::steady_clock::now();
+  client->wait_for_service(15_s);
+  auto end = std::chrono::steady_clock::now();
+  ASSERT_LE(end - start, 10_s);
+  shutdown_thread.join();
+}
diff --git a/test_rclcpp/test/test_executor.cpp b/test_rclcpp/test/test_executor.cpp
@@ -17,10 +17,12 @@
 #include <future>
 #include <stdexcept>
 #include <string>
+#include <thread>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "gtest/gtest.h"
 
 #include "rclcpp/rclcpp.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "test_rclcpp/utils.hpp"
 
@@ -203,6 +205,13 @@ TEST(CLASSNAME(test_executor, RMW_IMPLEMENTATION), notify) {
     auto client = node->create_client<test_rclcpp::srv::AddTwoInts>(
       "test_executor_notify_service"
       );
+    {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+      if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+        ASSERT_TRUE(client->wait_for_service(20_s)) << "service not available after waiting";
+      } else {
+        std::this_thread::sleep_for(1_s);
+      }
+    }
     auto request = std::make_shared<test_rclcpp::srv::AddTwoInts::Request>();
     request->a = 4;
     request->b = 2;

diff --git a/test_rclcpp/test/test_multiple_service_calls.cpp b/test_rclcpp/test/test_multiple_service_calls.cpp
@@ -13,12 +13,15 @@
 // limitations under the License.
 
 #include <iostream>
+#include <string>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
+#include <thread>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 #include <utility>
 #include <vector>
 
 #include "gtest/gtest.h"
 
 #include "rclcpp/rclcpp.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "test_rclcpp/srv/add_two_ints.hpp"
 
@@ -43,6 +46,13 @@ TEST(CLASSNAME(test_two_service_calls, RMW_IMPLEMENTATION), two_service_calls) {
     "test_two_service_calls", handle_add_two_ints);
 
   auto client = node->create_client<test_rclcpp::srv::AddTwoInts>("test_two_service_calls");
+  {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+    if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+      ASSERT_TRUE(client->wait_for_service(20_s)) << "service not available after waiting";
+    } else {
+      std::this_thread::sleep_for(1_s);
+    }
+  }
 
   auto request1 = std::make_shared<test_rclcpp::srv::AddTwoInts::Request>();
   request1->a = 1;
@@ -102,6 +112,13 @@ TEST(CLASSNAME(test_multiple_service_calls, RMW_IMPLEMENTATION), multiple_client
   fflush(stdout);
   // Send all the requests
   for (auto & pair : client_request_pairs) {
+    {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+      if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+        ASSERT_TRUE(pair.first->wait_for_service(20_s)) << "service not available after waiting";
+      } else {
+        std::this_thread::sleep_for(1_s);
+      }
+    }
     results.push_back(pair.first->async_send_request(pair.second));
   }
 

diff --git a/test_rclcpp/test/test_multithreaded.cpp b/test_rclcpp/test/test_multithreaded.cpp
@@ -14,13 +14,15 @@
 
 #include <limits>
 #include <string>
+#include <thread>  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 #include <utility>
 #include <vector>
 
 #include "gtest/gtest.h"
 
 #include "rclcpp/rclcpp.hpp"
 #include "rclcpp/executors.hpp"
+#include "rmw/rmw.h"  // TODO(wjwwood): remove me when fastrtps exclusion is removed
 
 #include "test_rclcpp/utils.hpp"
 #include "test_rclcpp/msg/u_int32.hpp"
@@ -192,6 +194,13 @@ TEST(CLASSNAME(test_multithreaded, RMW_IMPLEMENTATION), multi_consumer_clients)
     std::vector<SharedFuture> results;
     // Send all the requests
     for (auto & pair : client_request_pairs) {
+      {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+        if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+          ASSERT_TRUE(pair.first->wait_for_service(20_s)) << "service not available after waiting";
+        } else {
+          std::this_thread::sleep_for(1_s);
+        }
+      }
       results.push_back(pair.first->async_send_request(pair.second));
     }
     // Wait on the future produced by the first request
@@ -214,6 +223,13 @@ TEST(CLASSNAME(test_multithreaded, RMW_IMPLEMENTATION), multi_consumer_clients)
     std::vector<SharedFuture> results;
     // Send all the requests again
     for (auto & pair : client_request_pairs) {
+      {  // TODO(wjwwood): remove this block when fastrtps supports wait_for_service.
+        if (std::string(rmw_get_implementation_identifier()) != "rmw_fastrtps_cpp") {
+          ASSERT_TRUE(pair.first->wait_for_service(20_s)) << "service not available after waiting";
+        } else {
+          std::this_thread::sleep_for(1_s);
+        }
+      }
       results.push_back(pair.first->async_send_request(pair.second));
     }
     auto timer_callback = [&executor, &results]() {