From ed9a33199639cb07e5a1df77b8cc39e29a525982 Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 29 Nov 2023 21:34:16 -0600 Subject: [PATCH 1/2] simplify the missing_event test to not spin on the node itself Signed-off-by: William Woodall --- .../test/rclcpp/executors/test_executors.cpp | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/rclcpp/test/rclcpp/executors/test_executors.cpp b/rclcpp/test/rclcpp/executors/test_executors.cpp index 85586b90c0..89cba84b90 100644 --- a/rclcpp/test/rclcpp/executors/test_executors.cpp +++ b/rclcpp/test/rclcpp/executors/test_executors.cpp @@ -668,26 +668,19 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) rclcpp::Node::SharedPtr node(this->node); auto callback_group = node->create_callback_group( rclcpp::CallbackGroupType::MutuallyExclusive, - true); + false); auto waitable_interfaces = node->get_node_waitables_interface(); auto my_waitable = std::make_shared(); auto my_waitable2 = std::make_shared(); waitable_interfaces->add_waitable(my_waitable, callback_group); waitable_interfaces->add_waitable(my_waitable2, callback_group); - executor.add_node(this->node); + executor.add_callback_group(callback_group, node->get_node_base_interface()); my_waitable->trigger(); my_waitable2->trigger(); - // a node has some default subscribers, that need to get executed first, therefore the loop - for (int i = 0; i < 10; i++) { - executor.spin_once(std::chrono::milliseconds(10)); - if (my_waitable->get_count() > 0) { - // stop execution, after the first waitable has been executed - break; - } - } + executor.spin_once(std::chrono::milliseconds(10)); EXPECT_EQ(1u, my_waitable->get_count()); EXPECT_EQ(0u, my_waitable2->get_count()); @@ -696,16 +689,16 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) // This removes my_waitable2 from the list of ready events, and triggers a call to wait_for_work callback_group->can_be_taken_from().exchange(false); - //now there should be no ready event + // now there should be no ready event executor.spin_once(std::chrono::milliseconds(10)); EXPECT_EQ(1u, my_waitable->get_count()); EXPECT_EQ(0u, my_waitable2->get_count()); - //unblock the callback group + // unblock the callback group callback_group->can_be_taken_from().exchange(true); - //now the second waitable should get processed + // now the second waitable should get processed executor.spin_once(std::chrono::milliseconds(10)); EXPECT_EQ(1u, my_waitable->get_count()); From b4b71d93e8069c85d767199d36cde61a8deb55ee Mon Sep 17 00:00:00 2001 From: William Woodall Date: Wed, 29 Nov 2023 23:46:49 -0600 Subject: [PATCH 2/2] use futures to make the test rebust to spurious spins Signed-off-by: William Woodall --- .../test/rclcpp/executors/test_executors.cpp | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/rclcpp/test/rclcpp/executors/test_executors.cpp b/rclcpp/test/rclcpp/executors/test_executors.cpp index 89cba84b90..5b3206b131 100644 --- a/rclcpp/test/rclcpp/executors/test_executors.cpp +++ b/rclcpp/test/rclcpp/executors/test_executors.cpp @@ -454,6 +454,14 @@ class TestWaitable : public rclcpp::Waitable (void) data; count_++; std::this_thread::sleep_for(3ms); + try { + std::lock_guard lock(execute_promise_mutex_); + execute_promise_.set_value(); + } catch (const std::future_error & future_error) { + if (future_error.code() != std::future_errc::promise_already_satisfied) { + throw; + } + } } void @@ -480,8 +488,18 @@ class TestWaitable : public rclcpp::Waitable return count_; } + std::future + reset_execute_promise_and_get_future() + { + std::lock_guard lock(execute_promise_mutex_); + execute_promise_ = std::promise(); + return execute_promise_.get_future(); + } + private: bool is_ready_called_before_take_data = false; + std::promise execute_promise_; + std::mutex execute_promise_mutex_; size_t count_ = 0; rclcpp::GuardCondition gc_; }; @@ -670,6 +688,7 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) rclcpp::CallbackGroupType::MutuallyExclusive, false); + std::chrono::seconds max_spin_duration(2); auto waitable_interfaces = node->get_node_waitables_interface(); auto my_waitable = std::make_shared(); auto my_waitable2 = std::make_shared(); @@ -680,7 +699,10 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) my_waitable->trigger(); my_waitable2->trigger(); - executor.spin_once(std::chrono::milliseconds(10)); + { + auto my_waitable_execute_future = my_waitable->reset_execute_promise_and_get_future(); + executor.spin_until_future_complete(my_waitable_execute_future, max_spin_duration); + } EXPECT_EQ(1u, my_waitable->get_count()); EXPECT_EQ(0u, my_waitable2->get_count()); @@ -690,7 +712,13 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) callback_group->can_be_taken_from().exchange(false); // now there should be no ready event - executor.spin_once(std::chrono::milliseconds(10)); + { + auto my_waitable2_execute_future = my_waitable2->reset_execute_promise_and_get_future(); + auto future_code = executor.spin_until_future_complete( + my_waitable2_execute_future, + std::chrono::milliseconds(100)); // expected to timeout + EXPECT_EQ(future_code, rclcpp::FutureReturnCode::TIMEOUT); + } EXPECT_EQ(1u, my_waitable->get_count()); EXPECT_EQ(0u, my_waitable2->get_count()); @@ -699,7 +727,10 @@ TYPED_TEST(TestExecutorsOnlyNode, missing_event) callback_group->can_be_taken_from().exchange(true); // now the second waitable should get processed - executor.spin_once(std::chrono::milliseconds(10)); + { + auto my_waitable2_execute_future = my_waitable2->reset_execute_promise_and_get_future(); + executor.spin_until_future_complete(my_waitable2_execute_future, max_spin_duration); + } EXPECT_EQ(1u, my_waitable->get_count()); EXPECT_EQ(1u, my_waitable2->get_count());