From 081a5889fd2ceae0e93b9d73a83ff1158144ce17 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 6 Jul 2016 15:32:50 -0700 Subject: [PATCH 1/5] pass node to client and service destroy funcs --- rcl/src/rcl/client.c | 2 +- rcl/src/rcl/service.c | 2 +- rcl/test/CMakeLists.txt | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 09ec8cd78..b66fd368a 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -103,7 +103,7 @@ rcl_client_fini(rcl_client_t * client, rcl_node_t * node) RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (client->impl) { rmw_ret_t ret = - rmw_destroy_client(client->impl->rmw_handle); + rmw_destroy_client(rcl_node_get_rmw_handle(node), client->impl->rmw_handle); if (ret != RMW_RET_OK) { RCL_SET_ERROR_MSG(rmw_get_error_string_safe()); result = RCL_RET_ERROR; diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 739906928..932003eb7 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -99,7 +99,7 @@ rcl_service_fini(rcl_service_t * service, rcl_node_t * node) RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (service->impl) { rmw_ret_t ret = - rmw_destroy_service(service->impl->rmw_handle); + rmw_destroy_service(rcl_node_get_rmw_handle(node), service->impl->rmw_handle); if (ret != RMW_RET_OK) { RCL_SET_ERROR_MSG(rmw_get_error_string_safe()); result = RCL_RET_ERROR; diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index ea04bd723..fee068917 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -61,7 +61,10 @@ function(test_target_function) ) # TODO(wjwwood): remove this when the graph API works properly for more than OpenSplice. - if(rmw_implementation STREQUAL "rmw_opensplice_cpp") + if( + rmw_implementation STREQUAL "rmw_opensplice_cpp" + OR rmw_implementation STREQUAL "rmw_connext_cpp" + ) rcl_add_custom_gtest(test_graph${target_suffix} SRCS rcl/test_graph.cpp ENV ${extra_test_env} From 54ea614d0397f62a4be9fb6c8459f0e9a44a4708 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 26 Oct 2016 19:06:22 -0700 Subject: [PATCH 2/5] address a flaky test with connext, will ticket --- rcl/test/rcl/test_graph.cpp | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/rcl/test/rcl/test_graph.cpp b/rcl/test/rcl/test_graph.cpp index 9d87c32ed..163ffba09 100644 --- a/rcl/test/rcl/test_graph.cpp +++ b/rcl/test/rcl/test_graph.cpp @@ -44,6 +44,9 @@ # define CLASSNAME(NAME, SUFFIX) NAME #endif +bool is_connext = + std::string(rmw_get_implementation_identifier()).find("connext") != std::string::npos; + class CLASSNAME (TestGraphFixture, RMW_IMPLEMENTATION) : public ::testing::Test { public: @@ -519,9 +522,23 @@ TEST_F(CLASSNAME(TestGraphFixture, RMW_IMPLEMENTATION), test_rcl_service_server_ std::to_string(time_to_sleep.count()).c_str()); ret = rcl_wait(this->wait_set_ptr, time_to_sleep.count()); if (ret == RCL_RET_TIMEOUT) { - continue; + if (!is_connext) { + // TODO(wjwwood): + // Connext has a race condition which can cause the graph guard + // condition to wake up due to the necessary topics going away, + // but afterwards rcl_service_server_is_available() still does + // not reflect that the service is "no longer available". + // The result is that some tests are flaky unless you not only + // check right after a graph change but again in the future where + // rcl_service_server_is_available() eventually reports the + // service is no longer there. This condition can be removed and + // we can always continue when we get RCL_RET_TIMEOUT once that + // is fixed. + continue; + } + } else { + ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); } - ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); ret = rcl_service_server_is_available(this->node_ptr, &client, &is_available); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); if (is_available == expected_state) { From 596c0ec9faaa81cb4ba9dc064c27ba340412c023 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Fri, 28 Oct 2016 17:07:53 -0700 Subject: [PATCH 3/5] invert test_graph exclusion logic --- rcl/test/CMakeLists.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt index fee068917..7c774dbd2 100644 --- a/rcl/test/CMakeLists.txt +++ b/rcl/test/CMakeLists.txt @@ -60,11 +60,13 @@ function(test_target_function) AMENT_DEPENDENCIES ${rmw_implementation} ) - # TODO(wjwwood): remove this when the graph API works properly for more than OpenSplice. + # TODO(wjwwood): remove this when the graph API works properly for FastRTPS if( - rmw_implementation STREQUAL "rmw_opensplice_cpp" - OR rmw_implementation STREQUAL "rmw_connext_cpp" + rmw_implementation STREQUAL "rmw_connext_dynamic_cpp" OR + rmw_implementation STREQUAL "rmw_fastrtps_cpp" ) + message(STATUS "Skipping test_graph${target_suffix} test.") + else() rcl_add_custom_gtest(test_graph${target_suffix} SRCS rcl/test_graph.cpp ENV ${extra_test_env} @@ -72,8 +74,6 @@ function(test_target_function) LIBRARIES ${PROJECT_NAME}${target_suffix} ${extra_test_libraries} AMENT_DEPENDENCIES ${rmw_implementation} "example_interfaces" "std_msgs" ) - else() - message(STATUS "Skipping test_graph${target_suffix} test.") endif() rcl_add_custom_gtest(test_rcl${target_suffix} From 5c98e085690104670a288749bcd79e8f2d6cbf18 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Fri, 28 Oct 2016 17:08:22 -0700 Subject: [PATCH 4/5] use zero initialization of tnat in test_graph --- rcl/test/rcl/test_graph.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/rcl/test/rcl/test_graph.cpp b/rcl/test/rcl/test_graph.cpp index 163ffba09..acd0b1c82 100644 --- a/rcl/test/rcl/test_graph.cpp +++ b/rcl/test/rcl/test_graph.cpp @@ -257,6 +257,7 @@ check_graph_state( ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); rcl_reset_error(); + tnat = rcl_get_zero_initialized_topic_names_and_types(); ret = rcl_get_topic_names_and_types(node_ptr, &tnat); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); rcl_reset_error(); From 0d9ecc1220eca945093a430b333e80e8711e6e44 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Fri, 28 Oct 2016 17:16:11 -0700 Subject: [PATCH 5/5] change is_connext logic --- rcl/test/rcl/test_graph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/test/rcl/test_graph.cpp b/rcl/test/rcl/test_graph.cpp index acd0b1c82..e185da060 100644 --- a/rcl/test/rcl/test_graph.cpp +++ b/rcl/test/rcl/test_graph.cpp @@ -45,7 +45,7 @@ #endif bool is_connext = - std::string(rmw_get_implementation_identifier()).find("connext") != std::string::npos; + std::string(rmw_get_implementation_identifier()).find("rmw_connext") == 0; class CLASSNAME (TestGraphFixture, RMW_IMPLEMENTATION) : public ::testing::Test {