ros2 · MichaelOrlov · Feb 27, 2025 · Feb 27, 2025 · Feb 27, 2025 · Feb 27, 2025
diff --git a/rosbag2_cpp/src/rosbag2_cpp/info.cpp b/rosbag2_cpp/src/rosbag2_cpp/info.cpp
@@ -17,8 +17,11 @@
 #include <filesystem>
 #include <unordered_map>
 #include <unordered_set>
+#include <vector>
+#include <memory>
 #include <stdexcept>
 #include <string>
+#include <utility>
 
 #include "rmw/rmw.h"
 #include "rosidl_typesupport_cpp/message_type_support.hpp"

diff --git a/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp b/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp
@@ -184,7 +184,8 @@ const LocalMessageDefinitionSource::MessageSpec & LocalMessageDefinitionSource::
   try {
     share_dir = ament_index_cpp::get_package_share_directory(package);
   } catch (const ament_index_cpp::PackageNotFoundError & e) {
-    ROSBAG2_CPP_LOG_WARN("'%s'", e.what());
+    ROSBAG2_CPP_LOG_WARN(
+      "get_package_share_directory(%s) failed with error: '%s'", package.c_str(), e.what());
     throw DefinitionNotFoundError(definition_identifier.topic_type());
   }
   std::string dir = definition_identifier.format() == Format::MSG ||
@@ -245,7 +246,7 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text(
       result = (delimiter(root_definition_identifier) +
         append_recursive(root_definition_identifier, max_recursion_depth));
     } catch (const TypenameNotUnderstoodError & err) {
-      ROSBAG2_CPP_LOG_ERROR(
+      ROSBAG2_CPP_LOG_WARN(
         "Message type name '%s' not understood by type definition search, "
         "definition will be left empty in bag.", err.what());
       format = Format::UNKNOWN;
@@ -280,7 +281,7 @@ rosbag2_storage::MessageDefinition LocalMessageDefinitionSource::get_full_text(
             format = Format::IDL;
           }
         } catch (const TypenameNotUnderstoodError & err) {
-          ROSBAG2_CPP_LOG_ERROR(
+          ROSBAG2_CPP_LOG_WARN(
             "Message type name '%s' not understood by type definition search, "
             "definition will be left empty in bag.", err.what());
           format = Format::UNKNOWN;

diff --git a/rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp b/rosbag2_cpp/src/rosbag2_cpp/service_utils.cpp
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include <cstring>
+#include <string>
 
 #include "rcl/service_introspection.h"
 

diff --git a/rosbag2_cpp/test/rosbag2_cpp/test_service_utils.cpp b/rosbag2_cpp/test/rosbag2_cpp/test_service_utils.cpp
@@ -16,6 +16,7 @@
 #include <string>
 #include <tuple>
 #include <vector>
+#include <utility>
 
 #include "rosbag2_cpp/service_utils.hpp"
 #include "rosbag2_test_common/memory_management.hpp"

diff --git a/rosbag2_py/src/rosbag2_py/_info.cpp b/rosbag2_py/src/rosbag2_py/_info.cpp
@@ -15,6 +15,7 @@
 #include <iostream>
 #include <memory>
 #include <string>
+#include <vector>
 #include <algorithm>
 
 #include "info_sorting_method.hpp"

diff --git a/rosbag2_py/src/rosbag2_py/format_service_info.cpp b/rosbag2_py/src/rosbag2_py/format_service_info.cpp
@@ -12,7 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <vector>
+#include <memory>
 #include <sstream>
+#include <string>
 
 #include "format_service_info.hpp"
 #include "rosbag2_cpp/service_utils.hpp"

diff --git a/rosbag2_py/test/test_transport.py b/rosbag2_py/test/test_transport.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import datetime
 from pathlib import Path
 import re
@@ -117,6 +118,8 @@ def test_record_cancel(tmp_path, storage_id):
 @pytest.mark.parametrize('storage_id', TESTED_STORAGE_IDS)
 def test_play_cancel(storage_id, capfd):
     bag_path = str(RESOURCES_PATH / storage_id / 'talker')
+    assert os.path.exists(bag_path), 'Could not find test bag file: ' + bag_path
+
     storage_options, converter_options = get_rosbag_options(bag_path, storage_id)
 
     player = rosbag2_py.Player()
@@ -131,18 +134,24 @@ def test_play_cancel(storage_id, capfd):
         daemon=True)
     player_thread.start()
 
-    def check_playback_start_output(cumulative_out, cumulative_err):
+    def check_playback_start_output(cap_streams):
         out, err = capfd.readouterr()
-        cumulative_err += err
-        cumulative_out += out
+        cap_streams["err"] += err
+        cap_streams["out"] += out
         expected_string_regex = re.compile(PLAYBACK_UNTIL_TIMESTAMP_REGEX_STRING)
-        matches = expected_string_regex.search(cumulative_err)
+        matches = expected_string_regex.search(cap_streams["err"])
         return matches is not None
 
-    captured_out = ''
-    captured_err = ''
-    assert wait_for(lambda: check_playback_start_output(captured_out, captured_err),
-                    timeout=rclpy.duration.Duration(seconds=3))
+    captured_streams = {"out": '', "err": ''}
+
+    if not wait_for(lambda: check_playback_start_output(captured_streams),
+                    timeout=rclpy.duration.Duration(seconds=5)):
+        with capfd.disabled():
+            print("\nCaptured stdout:", captured_streams["out"])
+            print("\nCaptured stderr:", captured_streams["err"])
+        player.cancel()
+        player_thread.join()
+        assert False
 
     player.cancel()
     player_thread.join(3)

diff --git a/rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp b/rosbag2_storage_sqlite3/src/rosbag2_storage_sqlite3/sqlite_storage.cpp
@@ -19,9 +19,9 @@
 #include <atomic>
 #include <chrono>
 #include <cstring>
-#include <filesystem>
-#include <fstream>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <iostream>
+#include <limits>
 #include <memory>
 #include <string>
 #include <unordered_map>

diff --git a/rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/storage_test_fixture.hpp b/rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/storage_test_fixture.hpp
@@ -20,7 +20,7 @@
 #include <cstdio>
 #include <cstring>
 #include <iostream>
-#include <filesystem>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <fstream>
 #include <memory>
 #include <string>

diff --git a/rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/test_sqlite_storage.cpp b/rosbag2_storage_sqlite3/test/rosbag2_storage_sqlite3/test_sqlite_storage.cpp
@@ -15,7 +15,7 @@
 #include <gmock/gmock.h>
 
 #include <algorithm>
-#include <filesystem>
+#include <filesystem>  // NOLINT cpplint: FP, filesystem is a C++17 system header
 #include <limits>
 #include <memory>
 #include <string>

diff --git a/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp b/rosbag2_test_common/include/rosbag2_test_common/process_execution_helpers_unix.hpp
@@ -116,9 +116,13 @@ bool wait_until_completion(
     // WNOHANG - wait for processes without causing the caller to be blocked
     wait_ret_code = waitpid(process_id, &status, WNOHANG);
   }
+  EXPECT_NE(wait_ret_code, 0) << "Testing process " << process_id << " hangout.\n";
   EXPECT_NE(wait_ret_code, -1);
   EXPECT_EQ(wait_ret_code, process_id) << "status = " << status;
-  return wait_ret_code != 0;
+  EXPECT_EQ(WIFEXITED(status), true) << "status = " << status;
+  EXPECT_EQ(WIFSIGNALED(status), false) << "Process terminated by signal: " << WTERMSIG(status);
+  EXPECT_EQ(WEXITSTATUS(status), EXIT_SUCCESS) << "status = " << status;
+  return WIFSIGNALED(status) != true && WEXITSTATUS(status) == EXIT_SUCCESS;
 }
 
 /// @brief Force to stop process with signal if it's currently running

diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_get_service_info.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_cpp_get_service_info.cpp
@@ -14,11 +14,13 @@
 
 #include <gmock/gmock.h>
 
-#include <atomic>
-#include <chrono>
 #include <filesystem>
+#include <chrono>
+#include <vector>
+#include <memory>
 #include <stdexcept>
 #include <string>
+#include <utility>
 
 #include "rosbag2_cpp/info.hpp"
 #include "rosbag2_cpp/writer.hpp"

diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_storage_api.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_storage_api.cpp
@@ -75,8 +75,8 @@ class Rosbag2StorageAPITests : public rosbag2_test_common::ParametrizedTemporary
       for (size_t msg_idx = 0; msg_idx < num_msgs_per_topic; msg_idx++) {
         auto std_string_msg = std::make_shared<std_msgs::msg::String>();
         std::stringstream ss;
-        for (int i = 0; i < 100; i++) {
-          ss << topic << "_long_long_string_message_" << msg_idx + 1 << "_repeat_";
+        for (int i = 0; i < 5; i++) {
+          ss << topic << "_long_string_message_" << msg_idx + 1 << "_repeat_";
         }
         std_string_msg->data = ss.str();
 
@@ -133,7 +133,7 @@ TEST_P(Rosbag2StorageAPITests, get_bagfile_size_read_write_interface)
     factory.open_read_write(options);
 
   std::vector<std::string> topics = {"topic1_topic1", "topic2_topic2"};
-  auto serialized_messages = prepare_serialized_messages(topics, 500);
+  auto serialized_messages = prepare_serialized_messages(topics, 500 * 20);
   create_topics(rw_storage, topics);
 
   rw_storage->write(serialized_messages);

diff --git a/rosbag2_transport/test/rosbag2_transport/test_play_next.cpp b/rosbag2_transport/test/rosbag2_transport/test_play_next.cpp
@@ -200,8 +200,8 @@ TEST_F(RosBag2PlayTestFixture, play_respect_messages_timing_after_play_next) {
   ASSERT_TRUE(player->play_next());
   ASSERT_TRUE(player->play_next());
   ASSERT_TRUE(player->is_paused());
-  player->resume();
   auto start = std::chrono::steady_clock::now();
+  player->resume();
   player->wait_for_playback_to_finish();
   auto replay_time = std::chrono::steady_clock::now() - start;
 

diff --git a/rosbag2_transport/test/rosbag2_transport/test_play_services.cpp b/rosbag2_transport/test/rosbag2_transport/test_play_services.cpp
@@ -172,7 +172,7 @@ class PlaySrvsTest : public RosBag2PlayTestFixture
   void expect_messages(bool messages_should_arrive, bool reset_message_counter = true)
   {
     // Not too worried about the exact timing in this test, give a lot of leeway
-    const auto condition_clear_time = std::chrono::milliseconds(ms_between_msgs_ * 10);
+    const auto condition_clear_time = std::chrono::milliseconds(ms_between_msgs_ * 100);
     std::unique_lock<std::mutex> lock(got_msg_mutex_);
     if (reset_message_counter) {
       message_counter_ = 0;
@@ -217,6 +217,7 @@ class PlaySrvsTest : public RosBag2PlayTestFixture
     player_->pause();  // Start playing in pause mode. Require for play_next test. For all other
     // tests we will resume playback via explicit call to start_playback().
     player_->play();
+    player_->wait_for_playback_to_start();
   }
 
   void topic_callback(std::shared_ptr<const test_msgs::msg::BasicTypes>/* msg */)

diff --git a/rosbag2_transport/test/rosbag2_transport/test_play_timing.cpp b/rosbag2_transport/test/rosbag2_transport/test_play_timing.cpp
@@ -163,10 +163,10 @@ TEST_F(PlayerTestFixture, playing_rate_negative)
 
 TEST_F(PlayerTestFixture, playing_respects_delay)
 {
-  rclcpp::Duration delay_margin(1, 0);
+  rclcpp::Duration delay_margin(0, static_cast<uint32_t>(RCUTILS_S_TO_NS(0.5)));
 
-  // Sleep 5.0 seconds before play
-  play_options_.delay = rclcpp::Duration(5, 0);
+  // Sleep 1.0 seconds before play
+  play_options_.delay = rclcpp::Duration(1, 0);
   auto lower_expected_duration = message_time_difference + play_options_.delay;
   auto upper_expected_duration = message_time_difference + play_options_.delay + delay_margin;
   auto player = std::make_shared<rosbag2_transport::Player>(