Improve test suite quality and coverage #1102
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (6)
You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds multiple GoogleTest suites, refactors loggers to use deferred subscription with a dedicated writer thread and synchronization, tightens integer parsing types, and updates repository ignore rules. 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: Construction (deferred subscription)
TestRunner->>Logger: construct Logger (calls StatusChangeLogger())
Logger->>Writer: start writer_thread
Writer-->>Logger: writer ready
Logger->>Tree: subscribeToTreeChanges(tree.rootNode())
Tree-->>Logger: status change callbacks (enqueue Transition)
alt status change occurs
Logger->>Writer: enqueue Transition (notify)
Writer->>Logger: pop batch under lock
Writer->>Storage: write batch, flush (file_mutex held)
Storage-->>Writer: write complete
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]>
The FileLogger2 constructor would start the writer thread after registering status change callbacks in the base class. This created a race condition where callbacks could fire before the writer thread was ready, causing TSAN failures (double lock and data race errors). Fix by adding an atomic 'ready' flag that is: - Set to true when the writer thread starts running - Checked in callback() before accessing the queue Callbacks that arrive before the writer thread is ready are now safely ignored. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The convertFromString<int64_t> and convertFromString<uint64_t> functions were using 'long' and 'unsigned long' for parsing. On Windows, these types are 32-bit, causing conversion failures for values that exceed the 32-bit range. Changed to use int64_t/uint64_t directly for proper cross-platform 64-bit integer handling. Co-Authored-By: Claude Opus 4.5 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
from
0d99e5b to
de0d2e9
Compare
Use proper condition_variable synchronization to ensure the writer thread is fully ready before the constructor returns. This prevents race conditions where callbacks fire before the thread is initialized. Previous atomic-based approach still had TSAN warnings because the callback could start executing while the constructor was blocked waiting for the condition variable, causing a double-lock scenario. Co-Authored-By: Claude Opus 4.5 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
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.
The root cause was that StatusChangeLogger subscribes to callbacks in its constructor, but _p (the PImpl) wasn't initialized until after the base class constructor completed. This meant callbacks could fire with _p uninitialized. Solution: Use the base-from-member idiom to ensure _p is initialized BEFORE StatusChangeLogger. C++ initializes base classes in declaration order, so by inheriting from FileLogger2PImplBase before StatusChangeLogger, we guarantee _p exists when callbacks start firing. Also wait for writer thread to be ready using condition_variable synchronization before returning from the constructor. Co-Authored-By: Claude Opus 4.5 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
from
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.
- Fix TSAN race condition in FileLogger2: set ready flag AFTER acquiring mutex for the first time to establish proper synchronization - Add deleted copy operations to FileLogger2PImplBase to satisfy cppcoreguidelines-special-member-functions - Fix nodiscard warnings in tests by casting convertFromString return values to void in ASSERT_THROW statements Co-Authored-By: Claude Opus 4.5 <[email protected]>
Establish a happens-before relationship by acquiring the mutex in the main thread before starting the writer thread. This satisfies TSAN's requirement that mutex creation and first cross-thread access are properly synchronized. Co-Authored-By: Claude Opus 4.5 <[email protected]>
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.
The mutex initialization must happen in FileLogger2PImplBase constructor, BEFORE StatusChangeLogger's constructor runs. StatusChangeLogger subscribes to callbacks during construction, which may fire and access the mutex. Having the mutex "used" by the main thread before any other thread accesses it establishes the happens-before relationship that TSAN requires. Co-Authored-By: Claude Opus 4.5 <[email protected]>
TSAN doesn't properly track happens-before relationship between mutex creation in make_unique and first cross-thread access, even though std::thread::thread provides the synchronization guarantee. The mutex is properly protected: base class constructor acquires/releases it before StatusChangeLogger subscribes to callbacks, and writer thread acquires it before setting the ready flag. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The flush() method was accessing file_stream without synchronization while the writer thread was also accessing it. This adds a file_mutex to protect all file_stream operations in both flush() and writerLoop(). Co-Authored-By: Claude Opus 4.5 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
from
d85d80c to
5057dd7
Compare
…fety Add support for deferred subscription in StatusChangeLogger to fix race conditions in loggers with worker threads (FileLogger2, SqliteLogger). Changes to StatusChangeLogger: - Add protected default constructor for deferred subscription mode - Add protected subscribeToTreeChanges() method - Derived classes can now complete initialization before receiving callbacks Changes to FileLogger2: - Remove FileLogger2PImplBase base-from-member idiom hack - Use simple Pimpl pattern with deferred subscription - Subscribe to tree changes AFTER writer thread is started - Remove ready flag and related synchronization complexity Changes to SqliteLogger: - Use deferred subscription to fix same race condition Remove all FileLogger2 TSAN suppressions - no longer needed. Co-Authored-By: Claude Opus 4.5 <[email protected]>
facontidavide
force-pushed
the
improve-test-suite-coverage
branch
from
5057dd7 to
cadda8f
Compare
Remove atomic flag approach to avoid ABI break. The deferred subscription pattern (StatusChangeLogger() + subscribeToTreeChanges()) achieves the same thread safety without adding new members to the base class. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Prevent recursive mutex locking when multiple nodes change status during a single tree tick. The callback lambda now copies state under the lock, releases it, then calls user code. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Focus CI resources on debugging TSAN issues. Co-Authored-By: Claude Opus 4.5 <[email protected]>
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