From a64af18b09ab033724642005361ff098601aab49 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 21 May 2024 14:07:58 -0700 Subject: [PATCH 1/2] Fix for "open_succeeds_twice" test failure on second run - Use std::filesystem for temp files and folders operation. For some reason rcpputils::fs::delete_all(folder_name) wasn't able to delete temp folder with subfolders. Signed-off-by: Michael Orlov --- .../test_sequential_compression_writer.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp b/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp index 4b5c3589b..8efd6aba3 100644 --- a/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp +++ b/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp @@ -14,6 +14,7 @@ #include +#include #include #include #include @@ -21,7 +22,6 @@ #include #include "rcpputils/asserts.hpp" -#include "rcpputils/filesystem_helper.hpp" #include "rosbag2_compression/compression_options.hpp" #include "rosbag2_compression/sequential_compression_writer.hpp" @@ -39,6 +39,8 @@ using namespace testing; // NOLINT +namespace fs = std::filesystem; + static constexpr const char * DefaultTestCompressor = "fake_comp"; class SequentialCompressionWriterTest : public TestWithParam @@ -49,12 +51,12 @@ class SequentialCompressionWriterTest : public TestWithParam storage_{std::make_shared>()}, converter_factory_{std::make_shared>()}, metadata_io_{std::make_unique>()}, - tmp_dir_{rcpputils::fs::temp_directory_path() / "SequentialCompressionWriterTest"}, + tmp_dir_{fs::temp_directory_path() / "SequentialCompressionWriterTest"}, tmp_dir_storage_options_{}, serialization_format_{"rmw_format"} { tmp_dir_storage_options_.uri = tmp_dir_.string(); - rcpputils::fs::remove_all(tmp_dir_); + fs::remove_all(tmp_dir_); ON_CALL(*storage_factory_, open_read_write(_)).WillByDefault(Return(storage_)); EXPECT_CALL(*storage_factory_, open_read_write(_)).Times(AtLeast(0)); // intercept the metadata write so we can analyze it. @@ -71,7 +73,7 @@ class SequentialCompressionWriterTest : public TestWithParam ~SequentialCompressionWriterTest() override { - rcpputils::fs::remove_all(tmp_dir_); + fs::remove_all(tmp_dir_); } void initializeFakeFileStorage() @@ -131,7 +133,7 @@ class SequentialCompressionWriterTest : public TestWithParam std::shared_ptr> converter_factory_; std::unique_ptr metadata_io_; - rcpputils::fs::path tmp_dir_; + fs::path tmp_dir_; rosbag2_storage::StorageOptions tmp_dir_storage_options_; rosbag2_storage::BagMetadata intercepted_write_metadata_; std::vector v_intercepted_update_metadata_; @@ -199,7 +201,7 @@ TEST_F(SequentialCompressionWriterTest, open_succeeds_on_supported_compression_f kDefaultCompressionQueueSize, kDefaultCompressionQueueThreads}; initializeWriter(compression_options); - auto tmp_dir = rcpputils::fs::temp_directory_path() / "path_not_empty"; + auto tmp_dir = tmp_dir_ / "path_not_empty"; auto storage_options = rosbag2_storage::StorageOptions(); storage_options.uri = tmp_dir.string(); @@ -214,8 +216,8 @@ TEST_F(SequentialCompressionWriterTest, open_succeeds_twice) kDefaultCompressionQueueSize, kDefaultCompressionQueueThreads}; initializeWriter(compression_options); - auto tmp_dir = rcpputils::fs::temp_directory_path() / "path_not_empty"; - auto tmp_dir_next = rcpputils::fs::temp_directory_path() / "path_not_empty_next"; + auto tmp_dir = tmp_dir_ / "path_not_empty"; + auto tmp_dir_next = tmp_dir_ / "path_not_empty_next"; auto storage_options = rosbag2_storage::StorageOptions(); auto storage_options_next = rosbag2_storage::StorageOptions(); From 7081a65595528a614ca811e0ce936b1f356830d6 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 21 May 2024 14:09:20 -0700 Subject: [PATCH 2/2] Address flakiness in TestRosbag2CPPAPI::minimal_writer_example - The `serialized_msg2` is not owning the serialized data after the first call writer.write(serialized_msg2,..). i.e. need to use another message or another API in test for second call to writer.write(msg). Signed-off-by: Michael Orlov --- rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp index 982d46093..04b356215 100644 --- a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp +++ b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_api.cpp @@ -106,9 +106,7 @@ TEST_P(TestRosbag2CPPAPI, minimal_writer_example) writer.open(rosbag_directory_next.string()); // write same topic to different bag - writer.write( - serialized_msg2, "/yet/another/topic", "test_msgs/msg/BasicTypes", - rclcpp::Clock().now()); + writer.write(bag_message, "/my/other/topic", "test_msgs/msg/BasicTypes"); // close by scope } @@ -152,7 +150,7 @@ TEST_P(TestRosbag2CPPAPI, minimal_writer_example) &extracted_serialized_msg, &extracted_test_msg); EXPECT_EQ(test_msg, extracted_test_msg); - EXPECT_EQ("/yet/another/topic", topic); + EXPECT_EQ("/my/other/topic", topic); } // alternative reader