Parametrize all rosbag2_tests for both supported storage plugins (#1221)

* Start parametrizing rosbag2_tests with new framework

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* All record tests pass with parametrized

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Parametrize test_rosbag2_cpp_api

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* test_reindex paramized

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Paramize play_end_to_end

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Paramize test_info_end_to_end

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* rosbag2_tests: correct directory name for existing_dir test

Signed-off-by: James Smith <james@foxglove.dev>

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: James Smith <james@foxglove.dev>
Co-authored-by: James Smith <james@foxglove.dev>

rosbag2_cpp/src/rosbag2_cpp/reindexer.cpp

@@ -189,6 +189,7 @@ void Reindexer::aggregate_metadata(
     rosbag2_cpp::ConverterOptions blank_converter_options {};
     bag_reader->open(temp_so, blank_converter_options);
     auto temp_metadata = bag_reader->get_metadata();
+    metadata_.storage_identifier = temp_metadata.storage_identifier;
 
     if (temp_metadata.starting_time < metadata_.starting_time) {
       metadata_.starting_time = temp_metadata.starting_time;

rosbag2_tests/CMakeLists.txt

@@ -42,7 +42,8 @@ if(BUILD_TESTING)
 
   ament_add_gmock(test_rosbag2_record_end_to_end
     test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp
-    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+    TIMEOUT 120)
   if(TARGET test_rosbag2_record_end_to_end)
     ament_target_dependencies(test_rosbag2_record_end_to_end
       rclcpp
@@ -96,7 +97,8 @@ if(BUILD_TESTING)
       rosbag2_cpp
       rosbag2_storage
       rosbag2_storage_default_plugins
-      rosbag2_test_common)
+      rosbag2_test_common
+      std_msgs)
   endif()
 
 

rosbag2_tests/package.xml

@@ -24,6 +24,8 @@
   <test_depend>rosbag2_compression_zstd</test_depend>
   <test_depend>rosbag2_cpp</test_depend>
   <test_depend>rosbag2_storage_default_plugins</test_depend>
+  <!-- TODO(mcap): remove when mcap is added to default plugins -->
+  <test_depend>rosbag2_storage_mcap</test_depend>
   <test_depend>rosbag2_storage</test_depend>
   <test_depend>rosbag2_test_common</test_depend>
   <test_depend>std_msgs</test_depend>

rosbag2_tests/resources/mcap/cdr_test/metadata.yaml

@@ -0,0 +1,33 @@
+rosbag2_bagfile_information:
+  version: 6
+  storage_identifier: mcap
+  duration:
+    nanoseconds: 151137181
+  starting_time:
+    nanoseconds_since_epoch: 1586406456763032325
+  message_count: 7
+  topics_with_message_count:
+    - topic_metadata:
+        name: /array_topic
+        type: test_msgs/msg/Arrays
+        serialization_format: cdr
+        offered_qos_profiles: "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 0\n    nsec: 0\n  lifespan:\n    sec: 0\n    nsec: 0\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 0\n    nsec: 0\n  avoid_ros_namespace_conventions: false"
+      message_count: 4
+    - topic_metadata:
+        name: /test_topic
+        type: test_msgs/msg/BasicTypes
+        serialization_format: cdr
+        offered_qos_profiles: "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 0\n    nsec: 0\n  lifespan:\n    sec: 0\n    nsec: 0\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 0\n    nsec: 0\n  avoid_ros_namespace_conventions: false"
+      message_count: 3
+  compression_format: ""
+  compression_mode: ""
+  relative_file_paths:
+    - cdr_test_0.mcap
+  files:
+    - path: cdr_test_0.mcap
+      starting_time:
+        nanoseconds_since_epoch: 1586406456763032325
+      duration:
+        nanoseconds: 151137181
+      message_count: 7
+  custom_data: ~

rosbag2_tests/resources/mcap/wrong_rmw_test/metadata.yaml

@@ -0,0 +1,33 @@
+rosbag2_bagfile_information:
+  version: 6
+  storage_identifier: mcap
+  duration:
+    nanoseconds: 151137181
+  starting_time:
+    nanoseconds_since_epoch: 1586406456763032325
+  message_count: 7
+  topics_with_message_count:
+    - topic_metadata:
+        name: /array_topic
+        type: test_msgs/msg/Arrays
+        serialization_format: wrong_format
+        offered_qos_profiles: "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 0\n    nsec: 0\n  lifespan:\n    sec: 0\n    nsec: 0\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 0\n    nsec: 0\n  avoid_ros_namespace_conventions: false"
+      message_count: 4
+    - topic_metadata:
+        name: /test_topic
+        type: test_msgs/msg/BasicTypes
+        serialization_format: wrong_format
+        offered_qos_profiles: "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 0\n    nsec: 0\n  lifespan:\n    sec: 0\n    nsec: 0\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 0\n    nsec: 0\n  avoid_ros_namespace_conventions: false"
+      message_count: 3
+  compression_format: ""
+  compression_mode: ""
+  relative_file_paths:
+    - wrong_rmw_test_0.mcap
+  files:
+    - path: wrong_rmw_test_0.mcap
+      starting_time:
+        nanoseconds_since_epoch: 1586406456763032325
+      duration:
+        nanoseconds: 151137181
+      message_count: 7
+  custom_data: ~

rosbag2_tests/test/rosbag2_tests/record_fixture.hpp

@@ -32,6 +32,8 @@
 #include "rosbag2_storage/default_storage_id.hpp"
 #include "rosbag2_storage/storage_filter.hpp"
 #include "rosbag2_test_common/memory_management.hpp"
+#include "rosbag2_test_common/temporary_directory_fixture.hpp"
+#include "rosbag2_test_common/tested_storage_ids.hpp"
 
 #include "test_msgs/msg/arrays.hpp"
 #include "test_msgs/msg/basic_types.hpp"
@@ -41,26 +43,14 @@ using namespace ::testing;  // NOLINT
 using namespace std::chrono_literals;  // NOLINT
 using namespace rosbag2_test_common;  // NOLINT
 
-const std::unordered_map<std::string, std::string> storage_plugins_to_extension {
-  {"sqlite3", ".db3"}
-};
 
-class RecordFixture : public TestWithParam<std::string>
+class RecordFixture : public ParametrizedTemporaryDirectoryFixture
 {
 public:
-  RecordFixture()
-  {
-    temporary_dir_path_ = rcpputils::fs::create_temp_directory("tmp_test_dir_").string();
-  }
-
-  ~RecordFixture() override
-  {
-    rcpputils::fs::remove_all(rcpputils::fs::path(temporary_dir_path_));
-  }
-
   void SetUp() override
   {
-    root_bag_path_ = rcpputils::fs::path(temporary_dir_path_) / get_test_name();
+    auto bag_name = get_test_name() + "_" + GetParam();
+    root_bag_path_ = rcpputils::fs::path(temporary_dir_path_) / bag_name;
 
     // Clean up potentially leftover bag files.
     // There may be leftovers if the system reallocates a temp directory
@@ -83,6 +73,11 @@ class RecordFixture : public TestWithParam<std::string>
     rclcpp::shutdown();
   }
 
+  std::string get_base_record_command() const
+  {
+    return "ros2 bag record --storage " + GetParam() + " --output " + root_bag_path_.string();
+  }
+
   std::string get_test_name() const
   {
     const auto * test_info = UnitTest::GetInstance()->current_test_info();
@@ -95,7 +90,7 @@ class RecordFixture : public TestWithParam<std::string>
   std::string get_bag_file_name(int split_index) const
   {
     std::stringstream bag_file_name;
-    bag_file_name << get_test_name() << "_" << split_index;
+    bag_file_name << get_test_name() << "_" << GetParam() << "_" << split_index;
 
     return bag_file_name.str();
   }
@@ -113,8 +108,9 @@ class RecordFixture : public TestWithParam<std::string>
   rcpputils::fs::path get_relative_bag_file_path(int split_index)
   {
     const auto storage_id = GetParam();
-    const auto extension = storage_plugins_to_extension.at(storage_id);
-    return rcpputils::fs::path(get_bag_file_name(split_index) + extension);
+    return rcpputils::fs::path(
+      rosbag2_test_common::bag_filename_for_storage_id(
+        get_bag_file_name(split_index), storage_id));
   }
 
   void wait_for_metadata(std::chrono::duration<float> timeout = std::chrono::seconds(5)) const
@@ -223,7 +219,6 @@ class RecordFixture : public TestWithParam<std::string>
   #endif
   }
 
-  std::string temporary_dir_path_;
   // relative path to the root of the bag file.
   rcpputils::fs::path root_bag_path_;