rosbag2_cpp: test more than one storage plugin

mcap_vendor/CMakeLists.txt

@@ -28,7 +28,7 @@ macro(build_mcap_vendor)
   include(FetchContent)
   fetchcontent_declare(mcap
     GIT_REPOSITORY https://github.com/foxglove/mcap.git
-    GIT_TAG dc6561d9ba867901709e36526dcf7f7359861e9c # releases/cpp/v0.7.0
+    GIT_TAG 801c4ae3f34b23e9a27eb34b88ab7a0180d4b40f # v0.8.0
   )
   fetchcontent_makeavailable(mcap)
 

ros2bag/package.xml

@@ -22,6 +22,7 @@
   <test_depend>launch_testing</test_depend>
   <test_depend>launch_testing_ros</test_depend>
   <test_depend>python3-pytest</test_depend>
+  <test_depend>rosbag2_storage_default_plugins</test_depend>
 
   <export>
     <build_type>ament_python</build_type>

rosbag2_cpp/package.xml

@@ -26,7 +26,9 @@
   <depend>rosidl_typesupport_introspection_cpp</depend>
   <depend>shared_queues_vendor</depend>
 
-  <exec_depend>rosbag2_storage_default_plugins</exec_depend>
+  <test_depend>rosbag2_storage_default_plugins</test_depend>
+  <!-- TODO: remove dependency when rosbag2_storage_default_plugins depends on MCAP -->
+  <test_depend>rosbag2_storage_mcap</test_depend>
 
   <test_depend>ament_cmake_gmock</test_depend>
   <test_depend>ament_lint_auto</test_depend>

rosbag2_cpp/test/rosbag2_cpp/test_info.cpp

@@ -25,18 +25,18 @@
 #include "rosbag2_cpp/writer.hpp"
 
 #include "rosbag2_storage/bag_metadata.hpp"
-#include "rosbag2_storage/default_storage_id.hpp"
 #include "rosbag2_storage/metadata_io.hpp"
 
 #include "rosbag2_test_common/temporary_directory_fixture.hpp"
+#include "rosbag2_test_common/tested_storage_ids.hpp"
 
 #include "test_msgs/msg/basic_types.hpp"
 
 using namespace ::testing;  // NOLINT
-using rosbag2_test_common::TemporaryDirectoryFixture;
+using rosbag2_test_common::ParametrizedTemporaryDirectoryFixture;
 
-TEST_F(TemporaryDirectoryFixture, read_metadata_supports_version_2) {
-  const auto expected_storage_id = rosbag2_storage::get_default_storage_id();
+TEST_P(ParametrizedTemporaryDirectoryFixture, read_metadata_supports_version_2) {
+  const auto expected_storage_id = GetParam();
   const std::string bagfile = "rosbag2_bagfile_information:\n"
     "  version: 2\n"
     "  storage_identifier: " + expected_storage_id + "\n"
@@ -102,10 +102,9 @@ TEST_F(TemporaryDirectoryFixture, read_metadata_supports_version_2) {
   }
 }
 
-TEST_F(TemporaryDirectoryFixture, read_metadata_supports_version_6) {
-  const auto expected_storage_id = rosbag2_storage::get_default_storage_id();
+TEST_P(ParametrizedTemporaryDirectoryFixture, read_metadata_supports_version_6) {
+  const auto expected_storage_id = GetParam();
   const auto expected_storage_file = "test.testbag";
-
   const std::string bagfile = "rosbag2_bagfile_information:\n"
     "  version: 6\n"
     "  storage_identifier: " + expected_storage_id + "\n"
@@ -203,8 +202,10 @@ TEST_F(TemporaryDirectoryFixture, read_metadata_supports_version_6) {
   }
 }
 
-TEST_F(TemporaryDirectoryFixture, read_metadata_makes_appropriate_call_to_metadata_io_method) {
-  const auto expected_storage_id = rosbag2_storage::get_default_storage_id();
+TEST_P(
+  ParametrizedTemporaryDirectoryFixture,
+  read_metadata_makes_appropriate_call_to_metadata_io_method) {
+  const auto expected_storage_id = GetParam();
   std::string bagfile(
     "rosbag2_bagfile_information:\n"
     "  version: 3\n"
@@ -282,12 +283,16 @@ TEST_F(TemporaryDirectoryFixture, read_metadata_makes_appropriate_call_to_metada
   EXPECT_EQ(read_metadata.compression_mode, "FILE");
 }
 
-TEST_F(TemporaryDirectoryFixture, info_for_standalone_bagfile) {
+TEST_P(ParametrizedTemporaryDirectoryFixture, info_for_standalone_bagfile) {
+  const auto storage_id = GetParam();
   const auto bag_path = rcpputils::fs::path(temporary_dir_path_) / "bag";
   {
     // Create an empty bag with default storage
     rosbag2_cpp::Writer writer;
-    writer.open(bag_path.string());
+    rosbag2_storage::StorageOptions storage_options;
+    storage_options.storage_id = storage_id;
+    storage_options.uri = bag_path.string();
+    writer.open(storage_options);
     test_msgs::msg::BasicTypes msg;
     writer.write(msg, "testtopic", rclcpp::Time{});
   }
@@ -305,3 +310,9 @@ TEST_F(TemporaryDirectoryFixture, info_for_standalone_bagfile) {
   );
   EXPECT_THAT(metadata.topics_with_message_count, SizeIs(1));
 }
+
+INSTANTIATE_TEST_SUITE_P(
+  RosbagInfoTests,
+  ParametrizedTemporaryDirectoryFixture,
+  ValuesIn(rosbag2_test_common::kTestedStorageIDs)
+);

rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp

@@ -26,11 +26,11 @@
 #include "rosbag2_cpp/writer.hpp"
 
 #include "rosbag2_storage/bag_metadata.hpp"
-#include "rosbag2_storage/default_storage_id.hpp"
 #include "rosbag2_storage/metadata_io.hpp"
 #include "rosbag2_storage/ros_helper.hpp"
 #include "rosbag2_storage/topic_metadata.hpp"
 
+#include "rosbag2_test_common/tested_storage_ids.hpp"
 #include "rosbag2_test_common/temporary_directory_fixture.hpp"
 
 #include "test_msgs/msg/basic_types.hpp"
@@ -43,7 +43,7 @@
 #include "mock_storage_factory.hpp"
 
 using namespace testing;  // NOLINT
-using rosbag2_test_common::TemporaryDirectoryFixture;
+using rosbag2_test_common::ParametrizedTemporaryDirectoryFixture;
 
 class SequentialReaderTest : public Test
 {
@@ -221,13 +221,17 @@ TEST_F(SequentialReaderTest, next_file_calls_callback) {
   EXPECT_EQ(opened_file, bag_file_2_path_.string());
 }
 
-TEST_F(TemporaryDirectoryFixture, reader_accepts_bare_file) {
+TEST_P(ParametrizedTemporaryDirectoryFixture, reader_accepts_bare_file) {
   const auto bag_path = rcpputils::fs::path(temporary_dir_path_) / "bag";
+  const auto storage_id = GetParam();
 
   {
     // Create an empty bag with default storage
     rosbag2_cpp::Writer writer;
-    writer.open(bag_path.string());
+    rosbag2_storage::StorageOptions options;
+    options.uri = bag_path.string();
+    options.storage_id = storage_id;
+    writer.open(options);
     test_msgs::msg::BasicTypes msg;
     writer.write(msg, "testtopic", rclcpp::Time{});
   }
@@ -241,14 +245,20 @@ TEST_F(TemporaryDirectoryFixture, reader_accepts_bare_file) {
   EXPECT_THAT(reader.get_metadata().topics_with_message_count, SizeIs(1));
 }
 
+INSTANTIATE_TEST_SUITE_P(
+  BareFileTests,
+  ParametrizedTemporaryDirectoryFixture,
+  ValuesIn(rosbag2_test_common::kTestedStorageIDs)
+);
 
-class ReadOrderTest : public TemporaryDirectoryFixture
+
+class ReadOrderTest : public ParametrizedTemporaryDirectoryFixture
 {
 public:
   ReadOrderTest()
   {
     storage_options.uri = (rcpputils::fs::path(temporary_dir_path_) / "ordertest").string();
-    storage_options.storage_id = rosbag2_storage::get_default_storage_id();
+    storage_options.storage_id = GetParam();
     write_sample_split_bag(storage_options, fake_messages, split_every);
   }
 
@@ -328,7 +338,7 @@ class ReadOrderTest : public TemporaryDirectoryFixture
   rosbag2_storage::StorageOptions storage_options{};
 };
 
-TEST_F(ReadOrderTest, received_timestamp_order) {
+TEST_P(ReadOrderTest, received_timestamp_order) {
   rosbag2_storage::ReadOrder order(rosbag2_storage::ReadOrder::ReceivedTimestamp, false);
   sort_expected(order);
 
@@ -340,7 +350,7 @@ TEST_F(ReadOrderTest, received_timestamp_order) {
   }
 }
 
-TEST_F(ReadOrderTest, reverse_received_timestamp_order) {
+TEST_P(ReadOrderTest, reverse_received_timestamp_order) {
   rosbag2_storage::ReadOrder order(rosbag2_storage::ReadOrder::ReceivedTimestamp, true);
   sort_expected(order);
   reader.open(storage_options, rosbag2_cpp::ConverterOptions{});
@@ -358,21 +368,21 @@ TEST_F(ReadOrderTest, reverse_received_timestamp_order) {
   }
 }
 
-TEST_F(ReadOrderTest, file_order) {
-  reader.open(storage_options, rosbag2_cpp::ConverterOptions{});
-  EXPECT_FALSE(
-    reader.set_read_order(rosbag2_storage::ReadOrder(rosbag2_storage::ReadOrder::File, false)));
-}
-
-TEST_F(ReadOrderTest, reverse_file_order) {
+TEST_P(ReadOrderTest, reverse_file_order) {
   reader.open(storage_options, rosbag2_cpp::ConverterOptions{});
   EXPECT_FALSE(
     reader.set_read_order(rosbag2_storage::ReadOrder(rosbag2_storage::ReadOrder::File, true)));
 }
 
-TEST_F(ReadOrderTest, published_timestamp_order) {
+TEST_P(ReadOrderTest, published_timestamp_order) {
   reader.open(storage_options, rosbag2_cpp::ConverterOptions{});
   EXPECT_FALSE(
     reader.set_read_order(
       rosbag2_storage::ReadOrder(rosbag2_storage::ReadOrder::PublishedTimestamp, false)));
 }
+
+INSTANTIATE_TEST_SUITE_P(
+  ThisReadOrderTest,
+  ReadOrderTest,
+  ValuesIn(rosbag2_test_common::kTestedStorageIDs)
+);

rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp

@@ -26,11 +26,11 @@
 #include "rosbag2_cpp/writer.hpp"
 
 #include "rosbag2_storage/bag_metadata.hpp"
-#include "rosbag2_storage/default_storage_id.hpp"
 #include "rosbag2_storage/ros_helper.hpp"
 #include "rosbag2_storage/topic_metadata.hpp"
 
 #include "rosbag2_test_common/temporary_directory_fixture.hpp"
+#include "rosbag2_test_common/tested_storage_ids.hpp"
 
 #include "fake_data.hpp"
 #include "mock_converter.hpp"
@@ -40,7 +40,7 @@
 #include "mock_storage_factory.hpp"
 
 using namespace testing;  // NOLINT
-using rosbag2_test_common::TemporaryDirectoryFixture;
+using rosbag2_test_common::ParametrizedTemporaryDirectoryFixture;
 
 class SequentialWriterTest : public Test
 {
@@ -642,8 +642,7 @@ TEST_F(SequentialWriterTest, split_event_calls_callback)
   EXPECT_EQ(opened_file, fake_storage_uri_);
 }
 
-
-TEST_F(TemporaryDirectoryFixture, split_bag_metadata_has_full_duration) {
+TEST_P(ParametrizedTemporaryDirectoryFixture, split_bag_metadata_has_full_duration) {
   const std::vector<std::pair<rcutils_time_point_value_t, uint32_t>> fake_messages {
     {100, 1},
     {300, 2},
@@ -654,7 +653,7 @@ TEST_F(TemporaryDirectoryFixture, split_bag_metadata_has_full_duration) {
   };
   rosbag2_storage::StorageOptions storage_options;
   storage_options.uri = (rcpputils::fs::path(temporary_dir_path_) / "split_duration_bag").string();
-  storage_options.storage_id = rosbag2_storage::get_default_storage_id();
+  storage_options.storage_id = GetParam();
   write_sample_split_bag(storage_options, fake_messages, 3);
 
   rosbag2_storage::MetadataIo metadata_io;
@@ -664,3 +663,9 @@ TEST_F(TemporaryDirectoryFixture, split_bag_metadata_has_full_duration) {
     std::chrono::high_resolution_clock::time_point(std::chrono::nanoseconds(100)));
   ASSERT_EQ(metadata.duration, std::chrono::nanoseconds(500));
 }
+
+INSTANTIATE_TEST_SUITE_P(
+  SplitMetadataTest,
+  ParametrizedTemporaryDirectoryFixture,
+  ValuesIn(rosbag2_test_common::kTestedStorageIDs)
+);

rosbag2_storage_mcap/CMakeLists.txt

@@ -85,7 +85,6 @@ if(BUILD_TESTING)
   find_package(ament_cmake_gmock REQUIRED)
   find_package(ament_lint_auto REQUIRED)
   find_package(rcpputils REQUIRED)
-  find_package(rosbag2_cpp REQUIRED)
   find_package(rosbag2_test_common REQUIRED)
   find_package(std_msgs REQUIRED)
 
@@ -101,7 +100,7 @@ if(BUILD_TESTING)
 
   ament_add_gmock(test_mcap_storage test/rosbag2_storage_mcap/test_mcap_storage.cpp)
   target_link_libraries(test_mcap_storage ${PROJECT_NAME})
-  ament_target_dependencies(test_mcap_storage rosbag2_storage rosbag2_cpp rosbag2_test_common std_msgs)
+  ament_target_dependencies(test_mcap_storage rosbag2_storage rosbag2_test_common std_msgs)
   target_compile_definitions(test_mcap_storage PRIVATE ${MCAP_COMPILE_DEFS})
 
   ament_add_gmock(test_message_definition_cache test/rosbag2_storage_mcap/test_message_definition_cache.cpp)

rosbag2_storage_mcap/package.xml

@@ -21,7 +21,6 @@
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
   <test_depend>rcpputils</test_depend>
-  <test_depend>rosbag2_cpp</test_depend>
   <test_depend>rosbag2_test_common</test_depend>
   <test_depend>std_msgs</test_depend>
   <test_depend>rosbag2_storage_mcap_testdata</test_depend>