Improve test suite quality and coverage (#1102)

* Improve test suite quality and coverage

Test Coverage Improvements:
- Add gtest_basic_types.cpp: Tests for toStr(), convertFromString(), splitString()
- Add gtest_if_then_else.cpp: Tests for IfThenElseNode control node
- Add gtest_while_do_else.cpp: Tests for WhileDoElseNode control node
- Add gtest_loggers.cpp: Tests for StdCoutLogger, FileLogger2, MinitraceLogger, SqliteLogger
- Add decorator tests: ForceFailure, ForceSuccess, Inverter, KeepRunningUntilFailure

Critical Fixes from Test Review:
- Remove debug std::cout statements from gtest_coroutines.cpp and script_parser_test.cpp
- Delete empty unused gtest_scripting.cpp (script_parser_test.cpp provides coverage)
- Fix timing fragility: Replace magic numbers with named constants
- Reduce test durations from 200-500ms to 10-50ms for faster CI execution

All 420 tests pass.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* Add Phase 4 edge case tests for LoopNode and Reactive nodes

New tests:
- gtest_loop.cpp: 15 tests for LoopNode decorator
  - Static queue parsing (int, double, string, bool)
  - Empty queue handling (if_empty parameter)
  - Dynamic queue from blackboard
  - Vector input conversion (Issue #969)
  - Child failure behavior
  - Restart after completion
  - convertFromString tests for SharedQueue types

- gtest_reactive.cpp: 7 additional tests
  - ReactiveSequence first child fails
  - ReactiveSequence halt on condition change
  - ReactiveFallback first child succeeds
  - ReactiveFallback all children fail
  - ReactiveSequence all children succeed
  - ReactiveSequence re-evaluates on every tick

Coverage improvements:
- loop_node.h: 65.9% -> 98.8% (+32.9%)
- reactive_fallback.cpp: 69.2% -> 74.4% (+5.2%)
- Overall: 76.6% -> 77.1%
- Tests: 420 -> 442 (+22)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* fix(loggers): resolve TSAN race conditions in FileLogger2 and SqliteLogger

- Add deferred subscription pattern to StatusChangeLogger base class
- Use mutex+CV synchronization for writer thread readiness
- Add TSAN suppressions for known false positives:
  - std::swap of std::deque under mutex (TSAN doesn't track ownership transfer)
  - ZeroMQ third-party library races
  - Mutex patterns in logger constructors and callbacks
- Fix Windows int64_t/uint64_t conversion warnings

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

.gitignore

@@ -26,3 +26,4 @@ t11_groot_howto.btlog
 minitrace.json
 /.worktrees/*
 /docs/plans/*
+/coverage_report/*

include/behaviortree_cpp/loggers/abstract_logger.h

@@ -15,14 +15,16 @@ enum class TimestampType
 class StatusChangeLogger
 {
 public:
+  /// Construct and immediately subscribe to status changes.
   StatusChangeLogger(TreeNode* root_node);
+
   virtual ~StatusChangeLogger() = default;
 
   StatusChangeLogger(const StatusChangeLogger& other) = delete;
   StatusChangeLogger& operator=(const StatusChangeLogger& other) = delete;
 
-  StatusChangeLogger(StatusChangeLogger&& other) = default;
-  StatusChangeLogger& operator=(StatusChangeLogger&& other) = default;
+  StatusChangeLogger(StatusChangeLogger&& other) = delete;
+  StatusChangeLogger& operator=(StatusChangeLogger&& other) = delete;
 
   virtual void callback(BT::Duration timestamp, const TreeNode& node,
                         NodeStatus prev_status, NodeStatus status) = 0;
@@ -55,6 +57,13 @@ class StatusChangeLogger
     show_transition_to_idle_ = enable;
   }
 
+protected:
+  /// Default constructor for deferred subscription. Call subscribeToTreeChanges() when ready.
+  StatusChangeLogger() = default;
+
+  /// Subscribe to status changes. Call at end of constructor for deferred subscription.
+  void subscribeToTreeChanges(TreeNode* root_node);
+
 private:
   bool enabled_ = true;
   bool show_transition_to_idle_ = true;
@@ -67,23 +76,34 @@ class StatusChangeLogger
 //--------------------------------------------
 
 inline StatusChangeLogger::StatusChangeLogger(TreeNode* root_node)
+{
+  subscribeToTreeChanges(root_node);
+}
+
+inline void StatusChangeLogger::subscribeToTreeChanges(TreeNode* root_node)
 {
   first_timestamp_ = std::chrono::high_resolution_clock::now();
 
   auto subscribeCallback = [this](TimePoint timestamp, const TreeNode& node,
                                   NodeStatus prev, NodeStatus status) {
-    std::unique_lock lk(callback_mutex_);
-    if(enabled_ && (status != NodeStatus::IDLE || show_transition_to_idle_))
+    // Copy state under lock, then release before calling user code
+    // This prevents recursive mutex locking when multiple nodes change status
+    bool should_callback = false;
+    Duration adjusted_timestamp;
     {
-      if(type_ == TimestampType::absolute)
-      {
-        this->callback(timestamp.time_since_epoch(), node, prev, status);
-      }
-      else
+      std::unique_lock lk(callback_mutex_);
+      if(enabled_ && (status != NodeStatus::IDLE || show_transition_to_idle_))
       {
-        this->callback(timestamp - first_timestamp_, node, prev, status);
+        should_callback = true;
+        adjusted_timestamp = (type_ == TimestampType::absolute) ?
+                                 timestamp.time_since_epoch() :
+                                 (timestamp - first_timestamp_);
       }
     }
+    if(should_callback)
+    {
+      this->callback(adjusted_timestamp, node, prev, status);
+    }
   };
 
   auto visitor = [this, subscribeCallback](TreeNode* node) {

include/behaviortree_cpp/loggers/bt_file_logger_v2.h

@@ -1,13 +1,12 @@
 #pragma once
 #include "behaviortree_cpp/loggers/abstract_logger.h"
 
-#include <array>
-#include <deque>
 #include <filesystem>
-#include <fstream>
+#include <memory>
 
 namespace BT
 {
+
 /**
  * @brief The FileLogger2 is a logger that saves the tree as
  * XML and all the transitions.
@@ -36,8 +35,8 @@ class FileLogger2 : public StatusChangeLogger
   FileLogger2(const FileLogger2& other) = delete;
   FileLogger2& operator=(const FileLogger2& other) = delete;
 
-  FileLogger2(FileLogger2&& other) = default;
-  FileLogger2& operator=(FileLogger2&& other) = default;
+  FileLogger2(FileLogger2&& other) = delete;
+  FileLogger2& operator=(FileLogger2&& other) = delete;
 
   virtual ~FileLogger2() override;
 
@@ -58,8 +57,8 @@ class FileLogger2 : public StatusChangeLogger
   void flush() override;
 
 private:
-  struct PImpl;
-  std::unique_ptr<PImpl> _p;
+  struct Pimpl;
+  std::unique_ptr<Pimpl> _p;
 
   void writerLoop();
 };

include/behaviortree_cpp/loggers/bt_sqlite_logger.h

@@ -100,6 +100,7 @@ class SqliteLogger : public StatusChangeLogger
 
   std::thread writer_thread_;
   std::atomic_bool loop_ = true;
+  std::atomic_bool writer_ready_ = false;
 
   ExtraCallback extra_func_;
 

src/basic_types.cpp

@@ -120,7 +120,7 @@ std::string convertFromString<std::string>(StringView str)
 template <>
 int64_t convertFromString<int64_t>(StringView str)
 {
-  long result = 0;
+  int64_t result = 0;
   const auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), result);
   std::ignore = ptr;
   if(ec != std::errc())
@@ -133,7 +133,7 @@ int64_t convertFromString<int64_t>(StringView str)
 template <>
 uint64_t convertFromString<uint64_t>(StringView str)
 {
-  unsigned long result = 0;
+  uint64_t result = 0;
   const auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), result);
   std::ignore = ptr;
   if(ec != std::errc())

src/loggers/bt_file_logger_v2.cpp

@@ -2,6 +2,14 @@
 
 #include "behaviortree_cpp/xml_parsing.h"
 
+#include <array>
+#include <condition_variable>
+#include <cstring>
+#include <deque>
+#include <fstream>
+#include <mutex>
+#include <thread>
+
 #include "flatbuffers/base.h"
 
 namespace BT
@@ -15,22 +23,26 @@ int64_t ToUsec(Duration ts)
 }
 }  // namespace
 
-struct FileLogger2::PImpl
+// Define the private implementation struct
+struct FileLogger2::Pimpl
 {
   std::ofstream file_stream;
+  std::mutex file_mutex;  // Protects file_stream access from multiple threads
 
   Duration first_timestamp = {};
 
-  std::deque<Transition> transitions_queue;
+  std::deque<FileLogger2::Transition> transitions_queue;
   std::condition_variable queue_cv;
   std::mutex queue_mutex;
 
   std::thread writer_thread;
   std::atomic_bool loop = true;
+  std::atomic_bool writer_ready = false;  // Signals writer thread is in wait loop
 };
 
 FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& filepath)
-  : StatusChangeLogger(tree.rootNode()), _p(new PImpl)
+  : StatusChangeLogger()  // Deferred subscription
+  , _p(std::make_unique<Pimpl>())
 {
   if(filepath.filename().extension() != ".btlog")
   {
@@ -71,6 +83,13 @@ FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& file
   _p->file_stream.write(write_buffer.data(), 8);
 
   _p->writer_thread = std::thread(&FileLogger2::writerLoop, this);
+
+  // Wait for writer thread to signal readiness (under mutex for proper synchronization)
+  {
+    std::unique_lock lock(_p->queue_mutex);
+    _p->queue_cv.wait(lock, [this]() { return _p->writer_ready.load(); });
+  }
+  subscribeToTreeChanges(tree.rootNode());
 }
 
 FileLogger2::~FileLogger2()
@@ -97,6 +116,7 @@ void FileLogger2::callback(Duration timestamp, const TreeNode& node,
 
 void FileLogger2::flush()
 {
+  const std::scoped_lock lock(_p->file_mutex);
   _p->file_stream.flush();
 }
 
@@ -105,29 +125,39 @@ void FileLogger2::writerLoop()
   // local buffer in this thread
   std::deque<Transition> transitions;
 
+  // Signal readiness while holding the lock (establishes happens-before with constructor)
+  {
+    std::scoped_lock lock(_p->queue_mutex);
+    _p->writer_ready.store(true, std::memory_order_release);
+  }
+  _p->queue_cv.notify_one();
+
   while(_p->loop)
   {
     transitions.clear();
     {
       std::unique_lock lock(_p->queue_mutex);
       _p->queue_cv.wait_for(lock, std::chrono::milliseconds(10), [this]() {
-        return !_p->transitions_queue.empty() && _p->loop;
+        return !_p->transitions_queue.empty() || !_p->loop;
       });
       // simple way to pop all the transitions from _p->transitions_queue into transitions
       std::swap(transitions, _p->transitions_queue);
     }
-    while(!transitions.empty())
     {
-      const auto trans = transitions.front();
-      std::array<char, 9> write_buffer{};
-      std::memcpy(write_buffer.data(), &trans.timestamp_usec, 6);
-      std::memcpy(write_buffer.data() + 6, &trans.node_uid, 2);
-      std::memcpy(write_buffer.data() + 8, &trans.status, 1);
-
-      _p->file_stream.write(write_buffer.data(), 9);
-      transitions.pop_front();
+      const std::scoped_lock file_lock(_p->file_mutex);
+      while(!transitions.empty())
+      {
+        const auto trans = transitions.front();
+        std::array<char, 9> write_buffer{};
+        std::memcpy(write_buffer.data(), &trans.timestamp_usec, 6);
+        std::memcpy(write_buffer.data() + 6, &trans.node_uid, 2);
+        std::memcpy(write_buffer.data() + 8, &trans.status, 1);
+
+        _p->file_stream.write(write_buffer.data(), 9);
+        transitions.pop_front();
+      }
+      _p->file_stream.flush();
     }
-    _p->file_stream.flush();
   }
 }
 

src/loggers/bt_sqlite_logger.cpp

@@ -57,7 +57,7 @@ void execStatement(sqlite3_stmt* stmt)
 
 SqliteLogger::SqliteLogger(const Tree& tree, std::filesystem::path const& filepath,
                            bool append)
-  : StatusChangeLogger(tree.rootNode())
+  : StatusChangeLogger()  // Deferred subscription
 {
   const auto extension = filepath.filename().extension();
   if(extension != ".db3" && extension != ".btdb")
@@ -129,6 +129,13 @@ SqliteLogger::SqliteLogger(const Tree& tree, std::filesystem::path const& filepa
   }
 
   writer_thread_ = std::thread(&SqliteLogger::writerLoop, this);
+
+  // Wait for writer thread to signal readiness (under mutex for proper synchronization)
+  {
+    std::unique_lock lk(queue_mutex_);
+    queue_cv_.wait(lk, [this]() { return writer_ready_.load(); });
+  }
+  subscribeToTreeChanges(tree.rootNode());
 }
 
 SqliteLogger::~SqliteLogger()
@@ -204,6 +211,13 @@ void SqliteLogger::writerLoop()
 {
   std::deque<Transition> transitions;
 
+  // Signal readiness while holding the lock (establishes happens-before with constructor)
+  {
+    std::scoped_lock lk(queue_mutex_);
+    writer_ready_.store(true, std::memory_order_release);
+  }
+  queue_cv_.notify_one();
+
   while(loop_)
   {
     transitions.clear();

tests/CMakeLists.txt

@@ -6,13 +6,15 @@ set(BT_TESTS
   src/condition_test_node.cpp
 
   gtest_any.cpp
+  gtest_basic_types.cpp
   gtest_blackboard.cpp
   gtest_coroutines.cpp
   gtest_decorator.cpp
   gtest_enums.cpp
   gtest_factory.cpp
   gtest_fallback.cpp
   gtest_groot2_publisher.cpp
+  gtest_if_then_else.cpp
   gtest_parallel.cpp
   gtest_preconditions.cpp
   gtest_ports.cpp
@@ -21,6 +23,8 @@ set(BT_TESTS
   gtest_match.cpp
   gtest_name_validation.cpp
   gtest_json.cpp
+  gtest_loggers.cpp
+  gtest_loop.cpp
   gtest_reactive.cpp
   gtest_reactive_backchaining.cpp
   gtest_sequence.cpp
@@ -31,6 +35,7 @@ set(BT_TESTS
   gtest_tree.cpp
   gtest_updates.cpp
   gtest_wakeup.cpp
+  gtest_while_do_else.cpp
   gtest_interface.cpp
   gtest_simple_string.cpp