Skip to content

Commit 292fe84

Browse files
author
iRobot ROS
authored
Merge pull request #28 from alsora/asoragna/merge-conflicts
Asoragna/merge conflicts
2 parents 35741d3 + b7174da commit 292fe84

17 files changed

+97
-28
lines changed

‎rclcpp/include/rclcpp/executors/static_executor_entities_collector.hpp‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,14 @@ class StaticExecutorEntitiesCollector final
6464
void
6565
init(
6666
rcl_wait_set_t * p_wait_set,
67-
rclcpp::memory_strategy::MemoryStrategy::SharedPtr & memory_strategy,
67+
rclcpp::memory_strategy::MemoryStrategy::SharedPtr memory_strategy,
6868
rcl_guard_condition_t * executor_guard_condition);
6969

70+
/// Finalize StaticExecutorEntitiesCollector to clear resources
71+
RCLCPP_PUBLIC
72+
void
73+
fini();
74+
7075
RCLCPP_PUBLIC
7176
void
7277
execute() override;

‎rclcpp/include/rclcpp/executors/static_single_threaded_executor.hpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ class StaticSingleThreadedExecutor : public rclcpp::Executor
193193
std::chrono::nanoseconds timeout_left = timeout_ns;
194194

195195
entities_collector_->init(&wait_set_, memory_strategy_, &interrupt_guard_condition_);
196+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
196197

197198
while (rclcpp::ok(this->context_)) {
198199
// Do one set of work.

‎rclcpp/src/rclcpp/executors/static_executor_entities_collector.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ StaticExecutorEntitiesCollector::~StaticExecutorEntitiesCollector()
6161
void
6262
StaticExecutorEntitiesCollector::init(
6363
rcl_wait_set_t * p_wait_set,
64-
rclcpp::memory_strategy::MemoryStrategy::SharedPtr & memory_strategy,
64+
rclcpp::memory_strategy::MemoryStrategy::SharedPtr memory_strategy,
6565
rcl_guard_condition_t * executor_guard_condition)
6666
{
6767
// Empty initialize executable list
@@ -80,6 +80,13 @@ StaticExecutorEntitiesCollector::init(
8080
execute();
8181
}
8282

83+
void
84+
StaticExecutorEntitiesCollector::fini()
85+
{
86+
memory_strategy_->clear_handles();
87+
exec_list_.clear();
88+
}
89+
8390
void
8491
StaticExecutorEntitiesCollector::execute()
8592
{

‎rclcpp/src/rclcpp/executors/static_single_threaded_executor.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ StaticSingleThreadedExecutor::spin()
4242
// Set memory_strategy_ and exec_list_ based on weak_nodes_
4343
// Prepare wait_set_ based on memory_strategy_
4444
entities_collector_->init(&wait_set_, memory_strategy_, &interrupt_guard_condition_);
45+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
4546

4647
while (rclcpp::ok(this->context_) && spinning.load()) {
4748
// Refresh wait set and wait for work

‎rclcpp/test/CMakeLists.txt‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,9 @@ if(TARGET test_create_subscription)
102102
"test_msgs"
103103
)
104104
endif()
105-
ament_add_gtest(test_add_callback_groups_to_executor rclcpp/test_add_callback_groups_to_executor.cpp)
105+
ament_add_gtest(test_add_callback_groups_to_executor
106+
rclcpp/test_add_callback_groups_to_executor.cpp
107+
TIMEOUT 120)
106108
if(TARGET test_add_callback_groups_to_executor)
107109
target_link_libraries(test_add_callback_groups_to_executor ${PROJECT_NAME})
108110
ament_target_dependencies(test_add_callback_groups_to_executor
@@ -214,7 +216,8 @@ if(TARGET test_node_interfaces__node_clock)
214216
target_link_libraries(test_node_interfaces__node_clock ${PROJECT_NAME})
215217
endif()
216218
ament_add_gtest(test_node_interfaces__node_graph
217-
rclcpp/node_interfaces/test_node_graph.cpp)
219+
rclcpp/node_interfaces/test_node_graph.cpp
220+
TIMEOUT 120)
218221
if(TARGET test_node_interfaces__node_graph)
219222
ament_target_dependencies(
220223
test_node_interfaces__node_graph
@@ -329,7 +332,7 @@ ament_add_gtest(test_parameter_map rclcpp/test_parameter_map.cpp)
329332
if(TARGET test_parameter_map)
330333
target_link_libraries(test_parameter_map ${PROJECT_NAME})
331334
endif()
332-
ament_add_gtest(test_publisher rclcpp/test_publisher.cpp)
335+
ament_add_gtest(test_publisher rclcpp/test_publisher.cpp TIMEOUT 120)
333336
if(TARGET test_publisher)
334337
ament_target_dependencies(test_publisher
335338
"rcl"
@@ -412,7 +415,8 @@ if(TARGET test_service)
412415
)
413416
target_link_libraries(test_service ${PROJECT_NAME} mimick)
414417
endif()
415-
ament_add_gtest(test_subscription rclcpp/test_subscription.cpp)
418+
# Creating and destroying nodes is slow with Connext, so this needs larger timeout.
419+
ament_add_gtest(test_subscription rclcpp/test_subscription.cpp TIMEOUT 120)
416420
if(TARGET test_subscription)
417421
ament_target_dependencies(test_subscription
418422
"rcl_interfaces"
@@ -583,7 +587,7 @@ if(TARGET test_multi_threaded_executor)
583587
endif()
584588

585589
ament_add_gtest(test_static_executor_entities_collector rclcpp/executors/test_static_executor_entities_collector.cpp
586-
APPEND_LIBRARY_DIRS "${append_library_dirs}")
590+
APPEND_LIBRARY_DIRS "${append_library_dirs}" TIMEOUT 120)
587591
if(TARGET test_static_executor_entities_collector)
588592
ament_target_dependencies(test_static_executor_entities_collector
589593
"rcl"
@@ -673,7 +677,7 @@ endif()
673677

674678
ament_add_gtest(test_executor rclcpp/test_executor.cpp
675679
APPEND_LIBRARY_DIRS "${append_library_dirs}"
676-
)
680+
TIMEOUT 120)
677681
if(TARGET test_executor)
678682
ament_target_dependencies(test_executor "rcl")
679683
target_link_libraries(test_executor ${PROJECT_NAME} mimick)

‎rclcpp/test/rclcpp/executors/test_executors.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,9 @@ class ExecutorTypeNames
120120
}
121121
};
122122

123-
// TYPED_TEST_CASE is deprecated as of gtest 1.9, use TYPED_TEST_SUITE when gtest dependency
123+
// TYPED_TEST_SUITE is deprecated as of gtest 1.9, use TYPED_TEST_SUITE when gtest dependency
124124
// is updated.
125-
TYPED_TEST_CASE(TestExecutors, ExecutorTypes, ExecutorTypeNames);
125+
TYPED_TEST_SUITE(TestExecutors, ExecutorTypes, ExecutorTypeNames);
126126

127127
// StaticSingleThreadedExecutor is not included in these tests for now, due to:
128128
// https://github.com/ros2/rclcpp/issues/1219
@@ -131,7 +131,7 @@ using StandardExecutors =
131131
rclcpp::executors::SingleThreadedExecutor,
132132
rclcpp::executors::MultiThreadedExecutor,
133133
rclcpp::executors::EventsExecutor>;
134-
TYPED_TEST_CASE(TestExecutorsStable, StandardExecutors, ExecutorTypeNames);
134+
TYPED_TEST_SUITE(TestExecutorsStable, StandardExecutors, ExecutorTypeNames);
135135

136136
// Make sure that executors detach from nodes when destructing
137137
TYPED_TEST(TestExecutors, detachOnDestruction) {
@@ -421,6 +421,11 @@ class TestWaitable : public rclcpp::Waitable
421421
if (on_destruction_callback_) {
422422
on_destruction_callback_(this);
423423
}
424+
425+
rcl_ret_t ret = rcl_guard_condition_fini(&gc_);
426+
if (RCL_RET_OK != ret) {
427+
fprintf(stderr, "failed to call rcl_guard_condition_fini\n");
428+
}
424429
}
425430

426431
bool

‎rclcpp/test/rclcpp/executors/test_static_executor_entities_collector.cpp‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, add_remove_basic_node) {
170170
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
171171

172172
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
173+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
173174
EXPECT_EQ(
174175
expected_number_of_entities->subscriptions,
175176
entities_collector_->get_number_of_subscriptions());
@@ -217,6 +218,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, add_remove_node_out_of_scope) {
217218

218219
// Expect weak_node pointers to be cleaned up and used
219220
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
221+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
220222
EXPECT_EQ(0u, entities_collector_->get_number_of_subscriptions());
221223
EXPECT_EQ(0u, entities_collector_->get_number_of_timers());
222224
EXPECT_EQ(0u, entities_collector_->get_number_of_services());
@@ -278,6 +280,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, add_remove_node_with_entities) {
278280
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
279281

280282
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
283+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
281284

282285
EXPECT_EQ(
283286
1u + expected_number_of_entities->subscriptions,
@@ -356,6 +359,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, prepare_wait_set_rcl_wait_set_clear_
356359
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
357360

358361
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
362+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
359363

360364
{
361365
auto mock = mocking_utils::patch_and_return("lib:rclcpp", rcl_wait_set_clear, RCL_RET_ERROR);
@@ -365,7 +369,6 @@ TEST_F(TestStaticExecutorEntitiesCollector, prepare_wait_set_rcl_wait_set_clear_
365369
}
366370

367371
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
368-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
369372
}
370373

371374
TEST_F(TestStaticExecutorEntitiesCollector, prepare_wait_set_rcl_wait_set_resize_error) {
@@ -386,6 +389,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, prepare_wait_set_rcl_wait_set_resize
386389
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
387390

388391
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
392+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
389393

390394
{
391395
auto mock = mocking_utils::patch_and_return("lib:rclcpp", rcl_wait_set_resize, RCL_RET_ERROR);
@@ -395,7 +399,6 @@ TEST_F(TestStaticExecutorEntitiesCollector, prepare_wait_set_rcl_wait_set_resize
395399
}
396400

397401
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
398-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
399402
}
400403

401404
TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_not_initialized) {
@@ -423,6 +426,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_rcl_wait_failed) {
423426
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
424427

425428
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
429+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
426430

427431
{
428432
auto mock = mocking_utils::patch_and_return("lib:rclcpp", rcl_wait, RCL_RET_ERROR);
@@ -432,7 +436,6 @@ TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_rcl_wait_failed) {
432436
}
433437

434438
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
435-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
436439
}
437440

438441
TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_add_handles_to_wait_set_failed) {
@@ -472,6 +475,7 @@ TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_add_handles_to_wait
472475
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
473476

474477
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
478+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
475479

476480
{
477481
auto mock = mocking_utils::patch_and_return(
@@ -483,7 +487,6 @@ TEST_F(TestStaticExecutorEntitiesCollector, refresh_wait_set_add_handles_to_wait
483487
}
484488

485489
entities_collector_->remove_node(node->get_node_base_interface());
486-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
487490
}
488491

489492
TEST_F(TestStaticExecutorEntitiesCollector, add_to_wait_set_nullptr) {
@@ -504,14 +507,14 @@ TEST_F(TestStaticExecutorEntitiesCollector, add_to_wait_set_nullptr) {
504507
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
505508

506509
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
510+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
507511

508512
RCLCPP_EXPECT_THROW_EQ(
509513
entities_collector_->add_to_wait_set(nullptr),
510514
std::runtime_error("Executor waitable: couldn't add guard condition to wait set"));
511515
rcl_reset_error();
512516

513517
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
514-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
515518
}
516519

517520
TEST_F(TestStaticExecutorEntitiesCollector, fill_memory_strategy_invalid_group) {
@@ -540,10 +543,10 @@ TEST_F(TestStaticExecutorEntitiesCollector, fill_memory_strategy_invalid_group)
540543
cb_group.reset();
541544

542545
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
546+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
543547
ASSERT_EQ(entities_collector_->get_all_callback_groups().size(), 1u);
544548

545549
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
546-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
547550
}
548551

549552
TEST_F(TestStaticExecutorEntitiesCollector, remove_callback_group_after_node) {
@@ -610,10 +613,10 @@ TEST_F(
610613
rcl_guard_condition_t rcl_guard_condition = guard_condition.get_rcl_guard_condition();
611614

612615
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
616+
RCLCPP_SCOPE_EXIT(entities_collector_->fini());
613617

614618
cb_group->get_associated_with_executor_atomic().exchange(false);
615619
entities_collector_->execute();
616620

617621
EXPECT_TRUE(entities_collector_->remove_node(node->get_node_base_interface()));
618-
entities_collector_->init(&wait_set, memory_strategy, &rcl_guard_condition);
619622
}

‎rclcpp/test/rclcpp/test_add_callback_groups_to_executor.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@
3030
#include "rclcpp/executor.hpp"
3131
#include "rclcpp/rclcpp.hpp"
3232

33+
// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
34+
// that this test can complete fully, or adjust the timeout as necessary.
35+
// See https://github.com/ros2/rmw_connext/issues/325 for resolution]
36+
3337
using namespace std::chrono_literals;
3438

3539
template<typename T>
@@ -80,7 +84,7 @@ class ExecutorTypeNames
8084
}
8185
};
8286

83-
TYPED_TEST_CASE(TestAddCallbackGroupsToExecutor, ExecutorTypes, ExecutorTypeNames);
87+
TYPED_TEST_SUITE(TestAddCallbackGroupsToExecutor, ExecutorTypes, ExecutorTypeNames);
8488

8589
/*
8690
* Test adding callback groups.

‎rclcpp/test/rclcpp/test_publisher.cpp‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@
2929

3030
#include "test_msgs/msg/empty.hpp"
3131

32+
// Note: This is a long running test with rmw_connext_cpp, if you change this file, please check
33+
// that this test can complete fully, or adjust the timeout as necessary.
34+
// See https://github.com/ros2/rmw_connext/issues/325 for resolution
35+
3236
class TestPublisher : public ::testing::Test
3337
{
3438
public:
@@ -180,7 +184,7 @@ static std::vector<TestParameters> invalid_qos_profiles()
180184
return parameters;
181185
}
182186

183-
INSTANTIATE_TEST_CASE_P(
187+
INSTANTIATE_TEST_SUITE_P(
184188
TestPublisherThrows, TestPublisherInvalidIntraprocessQos,
185189
::testing::ValuesIn(invalid_qos_profiles()),
186190
::testing::PrintToStringParamName());
@@ -274,6 +278,9 @@ TEST_F(TestPublisher, serialized_message_publish) {
274278
auto publisher = node->create_publisher<test_msgs::msg::Empty>("topic", 10, options);
275279

276280
rclcpp::SerializedMessage serialized_msg;
281+
// Mock successful rcl publish because the serialized_msg above is poorly formed
282+
auto mock = mocking_utils::patch_and_return(
283+
"self", rcl_publish_serialized_message, RCL_RET_OK);
277284
EXPECT_NO_THROW(publisher->publish(serialized_msg));
278285

279286
EXPECT_NO_THROW(publisher->publish(serialized_msg.get_rcl_serialized_message()));
@@ -411,14 +418,22 @@ TEST_F(TestPublisher, inter_process_publish_failures) {
411418
EXPECT_THROW(publisher->publish(msg), rclcpp::exceptions::RCLError);
412419
}
413420

414-
rclcpp::SerializedMessage serialized_msg;
415-
EXPECT_NO_THROW(publisher->publish(serialized_msg));
421+
{
422+
// Using 'self' instead of 'lib:rclcpp' because `rcl_publish_serialized_message` is entirely
423+
// defined in a header. Also, this one requires mocking because the serialized_msg is poorly
424+
// formed and this just tests rclcpp functionality.
425+
auto mock = mocking_utils::patch_and_return(
426+
"self", rcl_publish_serialized_message, RCL_RET_OK);
427+
rclcpp::SerializedMessage serialized_msg;
428+
EXPECT_NO_THROW(publisher->publish(serialized_msg));
429+
}
416430

417431
{
418432
// Using 'self' instead of 'lib:rclcpp' because `rcl_publish_serialized_message` is entirely
419433
// defined in a header
420434
auto mock = mocking_utils::patch_and_return(
421435
"self", rcl_publish_serialized_message, RCL_RET_ERROR);
436+
rclcpp::SerializedMessage serialized_msg;
422437
EXPECT_THROW(publisher->publish(serialized_msg), rclcpp::exceptions::RCLError);
423438
}
424439

‎rclcpp/test/rclcpp/test_publisher_subscription_count_api.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ using AllTestDescriptions = ::testing::Types<
192192
TwoSubscriptionsInTwoContextsWithIntraprocessComm,
193193
TwoSubscriptionsInTwoContextsWithoutIntraprocessComm
194194
>;
195-
TYPED_TEST_CASE(TestPublisherSubscriptionCount, AllTestDescriptions, PrintTestDescription);
195+
TYPED_TEST_SUITE(TestPublisherSubscriptionCount, AllTestDescriptions, PrintTestDescription);
196196

197197

198198
using test_msgs::msg::Empty;

0 commit comments

Comments
 (0)