diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index 958c3e9819..f4dc24ddb2 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -231,8 +231,6 @@ function(create_tests_for_rmw_implementation) LINK_LIBS rosbag2_transport AMENT_DEPS test_msgs rosbag2_test_common) - ament_add_test_label(test_record_services${target_suffix} xfail) - if(${rmw_implementation} MATCHES "rmw_cyclonedds(.*)") ament_add_test_label(test_play_services__rmw_cyclonedds_cpp xfail) endif() diff --git a/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp b/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp index c8a55c2f19..0609c4134e 100644 --- a/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp +++ b/rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp @@ -57,12 +57,7 @@ class MockSequentialWriter : public rosbag2_cpp::writer_interfaces::BaseWriterIn messages_per_topic_[message->topic_name] += 1; messages_per_file_ += 1; if (messages_per_file_ == max_messages_per_file_) { // "Split" the bag every few messages - auto info = std::make_shared(); - info->closed_file = "BagFile" + std::to_string(file_number_); - file_number_ += 1; - info->opened_file = "BagFile" + std::to_string(file_number_); - callback_manager_.execute_callbacks(rosbag2_cpp::bag_events::BagEvent::WRITE_SPLIT, info); - messages_per_file_ = 0; + this->split_bagfile(); } } @@ -81,12 +76,6 @@ class MockSequentialWriter : public rosbag2_cpp::writer_interfaces::BaseWriterIn info->opened_file = "BagFile" + std::to_string(file_number_); callback_manager_.execute_callbacks(rosbag2_cpp::bag_events::BagEvent::WRITE_SPLIT, info); messages_per_file_ = 0; - split_bagfile_called_ = true; - } - - bool split_bagfile_called() - { - return split_bagfile_called_; } void @@ -120,6 +109,11 @@ class MockSequentialWriter : public rosbag2_cpp::writer_interfaces::BaseWriterIn return topics_; } + void set_max_messages_per_file(size_t max_messages_per_file) + { + max_messages_per_file_ = max_messages_per_file; + } + size_t max_messages_per_file() const { return max_messages_per_file_; @@ -134,8 +128,7 @@ class MockSequentialWriter : public rosbag2_cpp::writer_interfaces::BaseWriterIn bool snapshot_mode_ = false; rosbag2_cpp::bag_events::EventCallbackManager callback_manager_; size_t file_number_ = 0; - const size_t max_messages_per_file_ = 5; - bool split_bagfile_called_ = false; + size_t max_messages_per_file_ = 0; }; #endif // ROSBAG2_TRANSPORT__MOCK_SEQUENTIAL_WRITER_HPP_ diff --git a/rosbag2_transport/test/rosbag2_transport/test_record.cpp b/rosbag2_transport/test/rosbag2_transport/test_record.cpp index ece991a127..71456d30d1 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record.cpp @@ -324,6 +324,9 @@ TEST_F(RecordIntegrationTestFixture, write_split_callback_is_called) }; writer_->add_event_callbacks(callbacks); + auto & mock_writer = dynamic_cast(writer_->get_implementation_handle()); + mock_writer.set_max_messages_per_file(5); + rosbag2_transport::RecordOptions record_options = {false, false, {string_topic}, "rmw_format", 100ms}; auto recorder = std::make_shared( @@ -333,8 +336,7 @@ TEST_F(RecordIntegrationTestFixture, write_split_callback_is_called) start_async_spin(recorder); auto & writer = recorder->get_writer_handle(); - MockSequentialWriter & mock_writer = - static_cast(writer.get_implementation_handle()); + mock_writer = dynamic_cast(writer.get_implementation_handle()); const size_t expected_messages = mock_writer.max_messages_per_file() + 1; diff --git a/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp b/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp index 66f9298c8f..66108d67a4 100644 --- a/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp +++ b/rosbag2_transport/test/rosbag2_transport/test_record_services.cpp @@ -26,11 +26,7 @@ #include "rosbag2_transport/recorder.hpp" #include "rosbag2_test_common/publication_manager.hpp" -#include "rosbag2_test_common/wait_for.hpp" -#include "test_msgs/msg/arrays.hpp" -#include "test_msgs/msg/basic_types.hpp" -#include "test_msgs/msg/strings.hpp" #include "test_msgs/message_fixtures.hpp" #include "record_integration_fixture.hpp" @@ -43,7 +39,7 @@ class RecordSrvsTest : public RecordIntegrationTestFixture using Snapshot = rosbag2_interfaces::srv::Snapshot; using SplitBagfile = rosbag2_interfaces::srv::SplitBagfile; - explicit RecordSrvsTest(const bool snapshot_mode) + explicit RecordSrvsTest(bool snapshot_mode = false) : RecordIntegrationTestFixture(), snapshot_mode_(snapshot_mode) {} @@ -59,10 +55,6 @@ class RecordSrvsTest : public RecordIntegrationTestFixture { } - void subscription_callback(const test_msgs::msg::Strings::SharedPtr) - { - } - /// Use SetUp instead of ctor because we want to ASSERT some preconditions for the tests void SetUp() override { @@ -79,7 +71,7 @@ class RecordSrvsTest : public RecordIntegrationTestFixture auto string_message = get_messages_strings()[1]; rosbag2_test_common::PublicationManager pub_manager; - pub_manager.setup_publisher(test_topic_, string_message, 50); + pub_manager.setup_publisher(test_topic_, string_message, 10); const std::string ns = "/" + recorder_name_; cli_snapshot_ = client_node_->create_client(ns + "/snapshot"); @@ -131,7 +123,7 @@ class RecordSrvsTest : public RecordIntegrationTestFixture // Basic configuration const std::string recorder_name_ = "rosbag2_recorder_for_test_srvs"; const std::chrono::seconds service_wait_timeout_ {2}; - const std::chrono::seconds service_call_timeout_ {1}; + const std::chrono::seconds service_call_timeout_ {2}; const std::string test_topic_ = "/recorder_srvs_test_topic"; // Orchestration @@ -157,32 +149,52 @@ class RecordSrvsSnapshotTest : public RecordSrvsTest TEST_F(RecordSrvsSnapshotTest, trigger_snapshot) { auto & writer = recorder_->get_writer_handle(); - MockSequentialWriter & mock_writer = - static_cast(writer.get_implementation_handle()); + auto & mock_writer = dynamic_cast(writer.get_implementation_handle()); EXPECT_THAT(mock_writer.get_messages().size(), Eq(0u)); - // Sleep for 2 seconds to allow messages to accumulate in snapshot buffer - std::chrono::duration duration(2.0); - std::this_thread::sleep_for(duration); - EXPECT_THAT(mock_writer.get_snapshot_buffer().size(), Gt(0u)); + // Wait for messages to be appeared in snapshot_buffer + std::chrono::duration timeout = std::chrono::seconds(10); + using clock = std::chrono::steady_clock; + auto start = clock::now(); + while (mock_writer.get_snapshot_buffer().empty()) { + std::this_thread::sleep_for(std::chrono::milliseconds(30)); + EXPECT_LE((clock::now() - start), timeout) << "Failed to capture messages in time"; + } + EXPECT_THAT(mock_writer.get_messages().size(), Eq(0u)); successful_service_request(cli_snapshot_); EXPECT_THAT(mock_writer.get_messages().size(), Ne(0u)); } -class RecordSrvsSplitBagfileTest : public RecordSrvsTest -{ -protected: - RecordSrvsSplitBagfileTest() - : RecordSrvsTest(false /*snapshot_mode*/) {} -}; - -TEST_F(RecordSrvsSplitBagfileTest, split_bagfile) +TEST_F(RecordSrvsTest, split_bagfile) { auto & writer = recorder_->get_writer_handle(); - MockSequentialWriter & mock_writer = - static_cast(writer.get_implementation_handle()); - EXPECT_FALSE(mock_writer.split_bagfile_called()); + auto & mock_writer = dynamic_cast(writer.get_implementation_handle()); + bool callback_called = false; + std::string closed_file, opened_file; + rosbag2_cpp::bag_events::WriterEventCallbacks callbacks; + callbacks.write_split_callback = + [&callback_called, &closed_file, &opened_file](rosbag2_cpp::bag_events::BagSplitInfo & info) { + closed_file = info.closed_file; + opened_file = info.opened_file; + callback_called = true; + }; + mock_writer.add_event_callbacks(callbacks); + + // Wait for messages to be appeared in writer buffer + std::chrono::duration timeout = std::chrono::seconds(10); + using clock = std::chrono::steady_clock; + auto start = clock::now(); + while (mock_writer.get_messages().empty()) { + std::this_thread::sleep_for(std::chrono::milliseconds(30)); + EXPECT_LE((clock::now() - start), timeout) << "Failed to capture messages in time"; + } + + ASSERT_FALSE(callback_called); successful_service_request(cli_split_bagfile_); - EXPECT_TRUE(mock_writer.split_bagfile_called()); + + // Confirm that the callback was called and the file names have been sent with the event + ASSERT_TRUE(callback_called); + EXPECT_EQ(closed_file, "BagFile0"); + EXPECT_EQ(opened_file, "BagFile1"); }