Improve test suite quality and coverage #1102
Conversation
📝 WalkthroughWalkthroughAdds multiple new GTest suites, updates and standardizes test timings, refactors loggers for deferred subscription and writer-thread synchronization, tightens numeric parsing types, removes a stray debug print, updates TSAN suppressions, and ignores coverage_report in .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Runner
participant Tree as BehaviorTree::Tree
participant Logger as FileLogger2/SqliteLogger
participant Writer as WriterThread
participant Storage as File/Database
Note over Logger,Writer: Deferred subscription with writer readiness
TestRunner->>Logger: construct Logger (calls StatusChangeLogger())
Logger->>Writer: start writer thread
Writer-->>Logger: notify writer_ready (rgba(0,128,0,0.5))
Logger->>Tree: subscribeToTreeChanges(tree.rootNode())
Tree-->>Logger: status change callback (enqueue Transition)
alt status change
Logger->>Writer: enqueue Transition + notify
Writer->>Writer: pop batch under lock (rgba(0,0,255,0.5))
Writer->>Storage: write batch + flush
Storage-->>Writer: ack write
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/gtest_while_do_else.cpp`:
- Around line 139-165: The test named ConditionChanges_HaltsElse doesn't
actually exercise halt-on-condition-change because it calls tickWhileRunning()
once and the else branch completes; change the test to drive the tree with
tickOnce() across multiple ticks using an asynchronous action for TestB that
returns NodeStatus::RUNNING on the first tick and only completes later, register
ToggleCondition to flip from FAILURE to SUCCESS on the second tick, then assert
that When the condition flips the running else branch (TestB) receives a halt
and stops running and the then-branch (TestA) is executed; update references in
the test to use tickOnce(), the async TestB action (returns RUNNING then
halted), ToggleCondition, TestA/TestB and assert halt behavior via counters
and/or flags to prove haltChild() was invoked.
🧹 Nitpick comments (3)
tests/gtest_decorator.cpp (1)
59-59: Consider using a named constant for consistency.Line 59 uses a hardcoded
milliseconds(20)while the rest of the file now uses named constants likeACTION_SHORT_MS(which is also 20ms). For consistency with the timing constants pattern established in this file, consider usingACTION_SHORT_MShere.♻️ Suggested refactor
- RepeatTestAsync() : root("repeat", 3), action("action", milliseconds(20)) + RepeatTestAsync() : root("repeat", 3), action("action", ACTION_SHORT_MS)tests/gtest_basic_types.cpp (1)
186-191: Verify empty string split behavior.The test asserts that splitting an empty string returns an empty vector. While this is reasonable behavior, some implementations return a vector with one empty string. Consider adding a comment documenting this expected behavior.
📝 Suggested documentation
TEST(BasicTypes, SplitString_Empty) { auto parts = splitString("", ','); - // Empty string results in empty vector + // Empty string results in empty vector (not a single empty element) ASSERT_EQ(parts.size(), 0u); }tests/gtest_loggers.cpp (1)
40-44: TearDown may fail silently if files are still open.On some platforms,
std::filesystem::remove_allmay throw or fail silently if files are still open (e.g., if a test fails before closing a logger). Consider wrapping in a try-catch to prevent test framework issues.🛡️ Suggested defensive cleanup
void TearDown() override { // Clean up test files - std::filesystem::remove_all(test_dir); + try + { + std::filesystem::remove_all(test_dir); + } + catch(const std::filesystem::filesystem_error&) + { + // Ignore cleanup errors - may occur if files are locked + } }
| TEST_F(WhileDoElseTest, ConditionChanges_HaltsElse) | ||
| { | ||
| // When condition changes from false to true, else branch should be halted | ||
| int condition_counter = 0; | ||
| factory.registerSimpleCondition("ToggleCondition", [&condition_counter](BT::TreeNode&) { | ||
| return (condition_counter++ == 0) ? NodeStatus::FAILURE : NodeStatus::SUCCESS; | ||
| }); | ||
|
|
||
| const std::string xml_text = R"( | ||
| <root BTCPP_format="4"> | ||
| <BehaviorTree> | ||
| <WhileDoElse> | ||
| <ToggleCondition/> | ||
| <TestA/> | ||
| <TestB/> | ||
| </WhileDoElse> | ||
| </BehaviorTree> | ||
| </root>)"; | ||
|
|
||
| auto tree = factory.createTreeFromText(xml_text); | ||
|
|
||
| // First tick - condition false, executes else (TestB) | ||
| auto status = tree.tickWhileRunning(); | ||
| ASSERT_EQ(status, NodeStatus::SUCCESS); | ||
| ASSERT_EQ(counters[0], 0); // TestA not executed | ||
| ASSERT_EQ(counters[1], 1); // TestB executed | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the WhileDoElse implementation
fd -type f -name "*.cpp" -o -name "*.h" -o -name "*.hpp" | xargs rg -l "WhileDoElse" | head -20Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 446
🏁 Script executed:
# Look at the test file to understand the full context
cat tests/gtest_while_do_else.cppRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 9044
🏁 Script executed:
# Search for the WhileDoElse class implementation
fd -type f \( -name "*.cpp" -o -name "*.h" -o -name "*.hpp" \) | xargs rg -l "class WhileDoElse"Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 345
🏁 Script executed:
# Read the WhileDoElse implementation
cat include/behaviortree_cpp/controls/while_do_else_node.hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 2166
🏁 Script executed:
# Also check the cpp implementation
cat src/controls/while_do_else_node.cppRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 2476
Test name is misleading—doesn't verify the claimed halt-on-condition-change behavior.
The test name ConditionChanges_HaltsElse suggests it verifies halting of a running else branch when the condition toggles from false to true. However, tickWhileRunning() executes the tree to completion in a single call, and the condition is evaluated once per tick. Within that single tick, the condition returns FAILURE (first counter check), executes the else branch, completes successfully, and returns. The toggle condition is never actually reached because the tree terminates after the first tick.
The WhileDoElse implementation does support halting running branches when conditions change (via haltChild() calls), but this test doesn't demonstrate it. To verify this behavior, use an async action with tickOnce() calls to show a running branch being halted when the condition changes on a subsequent tick.
🤖 Prompt for AI Agents
In `@tests/gtest_while_do_else.cpp` around lines 139 - 165, The test named
ConditionChanges_HaltsElse doesn't actually exercise halt-on-condition-change
because it calls tickWhileRunning() once and the else branch completes; change
the test to drive the tree with tickOnce() across multiple ticks using an
asynchronous action for TestB that returns NodeStatus::RUNNING on the first tick
and only completes later, register ToggleCondition to flip from FAILURE to
SUCCESS on the second tick, then assert that When the condition flips the
running else branch (TestB) receives a halt and stops running and the
then-branch (TestA) is executed; update references in the test to use
tickOnce(), the async TestB action (returns RUNNING then halted),
ToggleCondition, TestA/TestB and assert halt behavior via counters and/or flags
to prove haltChild() was invoked.
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 <[email protected]>
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 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
2 times, most recently
from
75cf848 to
3da513e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/behaviortree_cpp/loggers/bt_file_logger_v2.h`:
- Around line 12-27: Add a direct `#include` <memory> to the header so
std::unique_ptr used in detail::FileLogger2PImplBase (member _p and
forward-declared FileLogger2Private) is available without relying on transitive
includes; place the include near the top of
include/behaviortree_cpp/loggers/bt_file_logger_v2.h before the
FileLogger2PImplBase declaration.
3da513e to
b41b8b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/gtest_decorator.cpp (1)
266-271:⚠️ Potential issue | 🟡 MinorComment inconsistency: delay is 100ms, not 200ms.
The comment on line 268 says "at least ~200ms" but the
delay_msecin the XML is set to 100. Should read "~100ms" to match the actual configured delay.📝 Proposed fix
// The child (AlwaysSuccess) should have been executed after the delay ASSERT_EQ(status, NodeStatus::SUCCESS); - // Verify that at least ~200ms have passed (with small tolerance for timing jitter) + // Verify that at least ~100ms have passed (with small tolerance for timing jitter) ASSERT_GE(elapsed.count(), 80);src/loggers/bt_file_logger_v2.cpp (1)
120-120:⚠️ Potential issue | 🟡 MinorIncorrect cast type: should be
uint8_t, notuint64_t.
trans.statusis declared asuint8_tin theTransitionstruct (see header line 74), but the cast here is touint64_t. While this works due to implicit narrowing, it's misleading and inconsistent. Use the correct target type.🔧 Proposed fix
- trans.status = static_cast<uint64_t>(status); + trans.status = static_cast<uint8_t>(status);
🤖 Fix all issues with AI agents
In `@include/behaviortree_cpp/loggers/bt_file_logger_v2.h`:
- Around line 20-27: The struct FileLogger2PImplBase declares a destructor and
move operations but doesn't declare copy operations, triggering
cppcoreguidelines-special-member-functions; explicitly delete the copy
constructor and copy assignment operator to convey non-copyable intent. Update
FileLogger2PImplBase to add declarations "FileLogger2PImplBase(const
FileLogger2PImplBase&) = delete;" and "FileLogger2PImplBase& operator=(const
FileLogger2PImplBase&) = delete;", keeping the existing move ctor/operator= and
destructor, since it holds a std::unique_ptr<FileLogger2Private> _p; this will
silence clang-tidy and follow the Rule of Five.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/loggers/bt_file_logger_v2.cpp (1)
158-160:⚠️ Potential issue | 🟡 MinorCondition predicate uses
&&instead of||for shutdown check.The condition
!_p->transitions_queue.empty() && _p->loopwon't immediately wake on shutdown (whenloopbecomesfalse). Compare withSqliteLogger::writerLoopwhich uses!transitions_queue_.empty() || !loop_to wake immediately on either new data OR shutdown request.Currently, shutdown relies on the 10ms timeout to wake the thread, causing a slight delay. This is functional but inconsistent with the pattern in
SqliteLogger.🐛 Proposed fix to match SqliteLogger pattern
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; });
🧹 Nitpick comments (1)
src/loggers/bt_file_logger_v2.cpp (1)
99-103: Spin-wait could be replaced with condition variable for better efficiency.The spin-wait with
yield()works correctly but burns CPU cycles until the writer thread signals readiness. Since the writer thread signals almost immediately after starting, this is acceptable for simplicity, but a condition variable would be more efficient if startup latency ever increases.♻️ Optional: Use condition variable instead of spin-wait
- // Wait for writer thread to signal it's ready - while(!_p->ready.load(std::memory_order_acquire)) - { - std::this_thread::yield(); - } + // Wait for writer thread to signal it's ready + { + std::unique_lock lock(_p->queue_mutex); + _p->queue_cv.wait(lock, [this]() { return _p->ready.load(std::memory_order_relaxed); }); + }This would require adjusting
writerLoopto notify after settingready.
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
2 times, most recently
from
5057dd7 to
cadda8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/loggers/bt_file_logger_v2.cpp`:
- Around line 85-92: Wrap the subscribeToTreeChanges(tree.rootNode()) call in a
try-catch that catches all exceptions, signals the background writer thread to
stop (use the Pimpl stop flag used by writerLoop and notify its condition
variable), join _p->writer_thread if joinable to avoid std::terminate, then
rethrow the exception; reference _p->writer_thread, Pimpl stop flag/condition
variable, writerLoop and subscribeToTreeChanges to locate the relevant members
and methods.
| _p->writer_thread = std::thread(&FileLogger2::writerLoop, this); | ||
|
|
||
| // Wait for writer thread to be ready before subscribing to callbacks | ||
| while(!_p->writer_ready.load(std::memory_order_acquire)) | ||
| { | ||
| std::this_thread::yield(); | ||
| } | ||
| subscribeToTreeChanges(tree.rootNode()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for subscribeToTreeChanges method declarations and definitions
rg -nP --type=cpp -C3 'subscribeToTreeChanges'Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 2535
🏁 Script executed:
#!/bin/bash
# Read the abstract_logger.h file around the implementation
sed -n '60,120p' include/behaviortree_cpp/loggers/abstract_logger.hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 2009
🏁 Script executed:
#!/bin/bash
# Search for FileLogger2 Pimpl definition and destructor
rg -nP --type=cpp -A 20 'struct.*Pimpl|class.*Pimpl' src/loggers/bt_file_logger_v2.cpp | head -100Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Search for FileLogger2 destructor
rg -nP --type=cpp -A 10 'FileLogger2::~FileLogger2' src/loggers/bt_file_logger_v2.cppRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 393
Guard subscribeToTreeChanges() with exception handling to prevent thread termination on exception.
If subscribeToTreeChanges() throws (it is not noexcept), the joinable writer_thread in Pimpl will trigger std::terminate() when the member is destroyed during stack unwinding. The method can throw via subscribers_.push_back() on allocation failure.
Add try-catch to properly stop the thread before re-throwing:
Exception-safe subscription pattern
_p->writer_thread = std::thread(&FileLogger2::writerLoop, this);
// Wait for writer thread to be ready before subscribing to callbacks
while(!_p->writer_ready.load(std::memory_order_acquire))
{
std::this_thread::yield();
}
- subscribeToTreeChanges(tree.rootNode());
+ try
+ {
+ subscribeToTreeChanges(tree.rootNode());
+ }
+ catch(...)
+ {
+ _p->loop = false;
+ _p->queue_cv.notify_one();
+ if(_p->writer_thread.joinable())
+ {
+ _p->writer_thread.join();
+ }
+ throw;
+ }🤖 Prompt for AI Agents
In `@src/loggers/bt_file_logger_v2.cpp` around lines 85 - 92, Wrap the
subscribeToTreeChanges(tree.rootNode()) call in a try-catch that catches all
exceptions, signals the background writer thread to stop (use the Pimpl stop
flag used by writerLoop and notify its condition variable), join
_p->writer_thread if joinable to avoid std::terminate, then rethrow the
exception; reference _p->writer_thread, Pimpl stop flag/condition variable,
writerLoop and subscribeToTreeChanges to locate the relevant members and
methods.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/loggers/bt_sqlite_logger.cpp`:
- Around line 131-138: Wrap the call to subscribeToTreeChanges(tree.rootNode())
in a try/catch that catches exceptions, signals the writer thread to stop (e.g.,
set the same shutdown flag used by writerLoop or notify queue_cv_ under
queue_mutex_), waits/join writer_thread_ if joinable, then rethrows (or
translate) the exception; this ensures writerLoop (started via writer_thread_)
is cleanly shut down and joined on error and prevents std::terminate during
stack unwinding while still propagating the original failure.
| 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()); |
There was a problem hiding this comment.
Guard subscribeToTreeChanges() with exception handling to prevent thread termination on exception.
If subscribeToTreeChanges() throws after the writer thread is started (line 131), the destructor won't be called (object not fully constructed), and the joinable writer_thread_ will trigger std::terminate() when destroyed during stack unwinding.
Exception-safe subscription pattern
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());
+ try
+ {
+ subscribeToTreeChanges(tree.rootNode());
+ }
+ catch(...)
+ {
+ loop_ = false;
+ queue_cv_.notify_one();
+ if(writer_thread_.joinable())
+ {
+ writer_thread_.join();
+ }
+ throw;
+ }🤖 Prompt for AI Agents
In `@src/loggers/bt_sqlite_logger.cpp` around lines 131 - 138, Wrap the call to
subscribeToTreeChanges(tree.rootNode()) in a try/catch that catches exceptions,
signals the writer thread to stop (e.g., set the same shutdown flag used by
writerLoop or notify queue_cv_ under queue_mutex_), waits/join writer_thread_ if
joinable, then rethrows (or translate) the exception; this ensures writerLoop
(started via writer_thread_) is cleanly shut down and joined on error and
prevents std::terminate during stack unwinding while still propagating the
original failure.
…ogger - 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 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
from
aafa964 to
3bafd20
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/gtest_decorator.cpp (1)
93-121:⚠️ Potential issue | 🟡 MinorAvoid fixed 50ms sleep to reduce flakiness.
Line 117 assumes the async action completes within a fixed window; on loaded CI/Windows timer granularity, it may still be RUNNING even though it finishes before the 100ms deadline. Prefer polling with an upper bound below
DEADLINE_MS.🛠️ Suggested tweak for DeadlineNotTriggeredTest
- std::this_thread::sleep_for(ACTION_SHORT_MS + milliseconds(20)); + auto start = std::chrono::steady_clock::now(); + while(action.status() == NodeStatus::RUNNING && + std::chrono::steady_clock::now() - start < milliseconds(DEADLINE_MS - 10)) + { + std::this_thread::sleep_for(milliseconds(1)); + }
🧹 Nitpick comments (2)
src/loggers/bt_file_logger_v2.cpp (2)
95-101: Addjoinable()check beforejoin()for defensive robustness.While unlikely in normal operation, calling
join()on a non-joinable thread throwsstd::system_error. Adding a guard improves robustness.Proposed fix
FileLogger2::~FileLogger2() { _p->loop = false; _p->queue_cv.notify_one(); - _p->writer_thread.join(); + if(_p->writer_thread.joinable()) + { + _p->writer_thread.join(); + } _p->file_stream.close(); }
109-109: Unnecessary intermediate cast touint64_t.
trans.statusisuint8_t, so castingstatustouint64_tfirst is misleading and then relies on implicit narrowing. Cast directly to the target type.Proposed fix
- trans.status = static_cast<uint64_t>(status); + trans.status = static_cast<uint8_t>(status);
|
Summary
This PR significantly improves test suite quality and increases code coverage from 69.5% to 77.1% (+7.6 percentage points).
New Test Files
Enhanced Existing Tests
Bug Fixes (discovered by new tests)
readyflag for proper synchronization.convertFromString<int64_t>andconvertFromString<uint64_t>usedlong/unsigned long(32-bit on Windows) instead of proper 64-bit types.Code Quality Fixes
Coverage Metrics
Key File Improvements
All 442 tests pass.
Summary by CodeRabbit
Tests
Improvements
Bug Fixes
Chores