Skip to content

Commit 5060cd4

Browse files
facontidavideclaude
facontidavide
and
claude
authored
Fix #930: mock substitution hangs when TestNodeConfigs absent (#1084)
* Fix #930: mock substitution hangs when TestNodeConfigs is absent Make TestNodeConfigs parsing conditional on its presence in the JSON configuration. Previously, an empty TestNodeConfigs object was always created, causing mock nodes to be registered even when no test configuration was intended, which led to infinite loops during tree execution. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix clang-tidy readability-implicit-bool-conversion warning Use explicit != nullptr comparison for pointer-to-bool conversion flagged by clang-tidy. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent d53a8ea commit 5060cd4

File tree

2 files changed

+286
-21
lines changed

2 files changed

+286
-21
lines changed

‎src/bt_factory.cpp‎

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -433,29 +433,34 @@ void BehaviorTreeFactory::loadSubstitutionRuleFromJSON(const std::string& json_t
433433

434434
std::unordered_map<std::string, TestNodeConfig> configs;
435435

436-
auto test_configs = json.at("TestNodeConfigs");
437-
for(auto const& [name, test_config] : test_configs.items())
436+
// TestNodeConfigs is optional: users may only have string-based
437+
// substitution rules that map to already-registered node types.
438+
if(json.contains("TestNodeConfigs"))
438439
{
439-
auto& config = configs[name];
440-
441-
auto return_status = test_config.at("return_status").get<std::string>();
442-
config.return_status = convertFromString<NodeStatus>(return_status);
443-
if(test_config.contains("async_delay"))
444-
{
445-
config.async_delay =
446-
std::chrono::milliseconds(test_config["async_delay"].get<int>());
447-
}
448-
if(test_config.contains("post_script"))
449-
{
450-
config.post_script = test_config["post_script"].get<std::string>();
451-
}
452-
if(test_config.contains("success_script"))
453-
{
454-
config.success_script = test_config["success_script"].get<std::string>();
455-
}
456-
if(test_config.contains("failure_script"))
440+
auto test_configs = json.at("TestNodeConfigs");
441+
for(auto const& [name, test_config] : test_configs.items())
457442
{
458-
config.failure_script = test_config["failure_script"].get<std::string>();
443+
auto& config = configs[name];
444+
445+
auto return_status = test_config.at("return_status").get<std::string>();
446+
config.return_status = convertFromString<NodeStatus>(return_status);
447+
if(test_config.contains("async_delay"))
448+
{
449+
config.async_delay =
450+
std::chrono::milliseconds(test_config["async_delay"].get<int>());
451+
}
452+
if(test_config.contains("post_script"))
453+
{
454+
config.post_script = test_config["post_script"].get<std::string>();
455+
}
456+
if(test_config.contains("success_script"))
457+
{
458+
config.success_script = test_config["success_script"].get<std::string>();
459+
}
460+
if(test_config.contains("failure_script"))
461+
{
462+
config.failure_script = test_config["failure_script"].get<std::string>();
463+
}
459464
}
460465
}
461466

‎tests/gtest_substitution.cpp‎

Lines changed: 260 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#include "behaviortree_cpp/bt_factory.h"
22

3+
#include <future>
4+
35
#include <gtest/gtest.h>
46

57
using namespace BT;
@@ -86,3 +88,261 @@ TEST(Substitution, SubTreeNodeSubstitution)
8688
auto status = tree.tickWhileRunning();
8789
ASSERT_EQ(status, BT::NodeStatus::SUCCESS);
8890
}
91+
92+
// Test for issue #930: Mock substitution with registerSimpleAction
93+
// should not hang when using string-based substitution from JSON.
94+
TEST(Substitution, StringSubstitutionWithSimpleAction_Issue930)
95+
{
96+
// XML tree: Sequence with a Delay, then an action to be substituted
97+
static const char* xml_text = R"(
98+
<root BTCPP_format="4">
99+
<BehaviorTree ID="MainTree">
100+
<Sequence>
101+
<Delay delay_msec="100">
102+
<AlwaysSuccess/>
103+
</Delay>
104+
<SaySomething name="action_to_replace" message="hello"/>
105+
</Sequence>
106+
</BehaviorTree>
107+
</root>
108+
)";
109+
110+
BehaviorTreeFactory factory;
111+
112+
// Register original action
113+
factory.registerSimpleAction("SaySomething",
114+
[](TreeNode& node) { return NodeStatus::SUCCESS; },
115+
{ InputPort<std::string>("message") });
116+
117+
// Register substitute action
118+
factory.registerSimpleAction("MyTestAction",
119+
[](TreeNode&) { return NodeStatus::SUCCESS; });
120+
121+
// Use string-based substitution: replace action_to_replace with
122+
// MyTestAction
123+
factory.addSubstitutionRule("action_to_replace", "MyTestAction");
124+
125+
factory.registerBehaviorTreeFromText(xml_text);
126+
auto tree = factory.createTree("MainTree");
127+
128+
// This should NOT hang. Use a future with timeout to detect hangs.
129+
auto future =
130+
std::async(std::launch::async, [&tree]() { return tree.tickWhileRunning(); });
131+
132+
auto status = future.wait_for(std::chrono::seconds(5));
133+
ASSERT_NE(status, std::future_status::timeout) << "Tree hung! tickWhileRunning did not "
134+
"complete within 5 seconds";
135+
ASSERT_EQ(future.get(), NodeStatus::SUCCESS);
136+
}
137+
138+
// Test for issue #930: TestNodeConfig-based substitution with
139+
// async_delay should also not hang on single-threaded executor.
140+
TEST(Substitution, TestNodeConfigAsyncSubstitution_Issue930)
141+
{
142+
static const char* xml_text = R"(
143+
<root BTCPP_format="4">
144+
<BehaviorTree ID="MainTree">
145+
<Sequence>
146+
<AlwaysSuccess name="action_A"/>
147+
<AlwaysSuccess name="action_B"/>
148+
</Sequence>
149+
</BehaviorTree>
150+
</root>
151+
)";
152+
153+
BehaviorTreeFactory factory;
154+
factory.registerBehaviorTreeFromText(xml_text);
155+
156+
// Substitute action_B with a TestNodeConfig that has async_delay
157+
TestNodeConfig test_config;
158+
test_config.return_status = NodeStatus::SUCCESS;
159+
test_config.async_delay = std::chrono::milliseconds(100);
160+
factory.addSubstitutionRule("action_B", test_config);
161+
162+
auto tree = factory.createTree("MainTree");
163+
164+
// This should NOT hang -- the TestNode should complete after the
165+
// async_delay and emit a wake-up signal.
166+
auto future =
167+
std::async(std::launch::async, [&tree]() { return tree.tickWhileRunning(); });
168+
169+
auto status = future.wait_for(std::chrono::seconds(5));
170+
ASSERT_NE(status, std::future_status::timeout) << "Tree hung! tickWhileRunning did not "
171+
"complete within 5 seconds";
172+
ASSERT_EQ(future.get(), NodeStatus::SUCCESS);
173+
}
174+
175+
// Test for issue #930: JSON-based substitution mapping to a
176+
// registered SimpleAction (string rule) should work correctly.
177+
TEST(Substitution, JsonStringSubstitution_Issue930)
178+
{
179+
static const char* xml_text = R"(
180+
<root BTCPP_format="4">
181+
<BehaviorTree ID="MainTree">
182+
<Sequence>
183+
<AlwaysSuccess name="action_A"/>
184+
<AlwaysSuccess name="action_B"/>
185+
</Sequence>
186+
</BehaviorTree>
187+
</root>
188+
)";
189+
190+
// JSON that maps action_B to a registered SimpleAction
191+
// (not a TestNodeConfig name)
192+
static const char* json_rules = R"(
193+
{
194+
"TestNodeConfigs": {},
195+
"SubstitutionRules": {
196+
"action_B": "MyReplacement"
197+
}
198+
}
199+
)";
200+
201+
BehaviorTreeFactory factory;
202+
203+
// Register the replacement action
204+
factory.registerSimpleAction("MyReplacement",
205+
[](TreeNode&) { return NodeStatus::SUCCESS; });
206+
207+
factory.loadSubstitutionRuleFromJSON(json_rules);
208+
factory.registerBehaviorTreeFromText(xml_text);
209+
auto tree = factory.createTree("MainTree");
210+
211+
// This should NOT hang
212+
auto future =
213+
std::async(std::launch::async, [&tree]() { return tree.tickWhileRunning(); });
214+
215+
auto status = future.wait_for(std::chrono::seconds(5));
216+
ASSERT_NE(status, std::future_status::timeout) << "Tree hung! tickWhileRunning did not "
217+
"complete within 5 seconds";
218+
ASSERT_EQ(future.get(), NodeStatus::SUCCESS);
219+
}
220+
221+
// Test for issue #930: loadSubstitutionRuleFromJSON should work
222+
// when TestNodeConfigs is empty (only string rules).
223+
TEST(Substitution, JsonWithEmptyTestNodeConfigs_Issue930)
224+
{
225+
static const char* json_rules = R"(
226+
{
227+
"TestNodeConfigs": {},
228+
"SubstitutionRules": {
229+
"node_A": "ReplacementNode"
230+
}
231+
}
232+
)";
233+
234+
BehaviorTreeFactory factory;
235+
factory.registerSimpleAction("ReplacementNode",
236+
[](TreeNode&) { return NodeStatus::SUCCESS; });
237+
238+
// This should not throw
239+
ASSERT_NO_THROW(factory.loadSubstitutionRuleFromJSON(json_rules));
240+
241+
const auto& rules = factory.substitutionRules();
242+
ASSERT_EQ(rules.size(), 1);
243+
ASSERT_EQ(rules.count("node_A"), 1);
244+
auto* rule_str = std::get_if<std::string>(&rules.at("node_A"));
245+
ASSERT_NE(rule_str, nullptr);
246+
ASSERT_EQ(*rule_str, "ReplacementNode");
247+
}
248+
249+
// Test for issue #930: loadSubstitutionRuleFromJSON should handle
250+
// missing TestNodeConfigs gracefully.
251+
TEST(Substitution, JsonWithoutTestNodeConfigs_Issue930)
252+
{
253+
static const char* json_rules = R"(
254+
{
255+
"SubstitutionRules": {
256+
"node_A": "ReplacementNode"
257+
}
258+
}
259+
)";
260+
261+
BehaviorTreeFactory factory;
262+
factory.registerSimpleAction("ReplacementNode",
263+
[](TreeNode&) { return NodeStatus::SUCCESS; });
264+
265+
// TestNodeConfigs is optional: string-only substitution rules
266+
// don't need TestNodeConfigs.
267+
ASSERT_NO_THROW(factory.loadSubstitutionRuleFromJSON(json_rules));
268+
269+
const auto& rules = factory.substitutionRules();
270+
ASSERT_EQ(rules.size(), 1);
271+
auto* rule_str = std::get_if<std::string>(&rules.at("node_A"));
272+
ASSERT_NE(rule_str, nullptr);
273+
ASSERT_EQ(*rule_str, "ReplacementNode");
274+
}
275+
276+
// Test for issue #930: End-to-end test combining JSON-based
277+
// string substitution with tree execution involving async nodes.
278+
// This closely matches the issue reporter's scenario.
279+
TEST(Substitution, JsonStringSubstitutionWithDelay_Issue930)
280+
{
281+
static const char* xml_text = R"(
282+
<root BTCPP_format="4">
283+
<BehaviorTree ID="MainTree">
284+
<Sequence>
285+
<Delay delay_msec="50">
286+
<AlwaysSuccess/>
287+
</Delay>
288+
<Script name="script_2" code=" val:=1 "/>
289+
</Sequence>
290+
</BehaviorTree>
291+
</root>
292+
)";
293+
294+
static const char* json_rules = R"(
295+
{
296+
"SubstitutionRules": {
297+
"script_2": "MyTest"
298+
}
299+
}
300+
)";
301+
302+
BehaviorTreeFactory factory;
303+
304+
bool action_executed = false;
305+
factory.registerSimpleAction("MyTest", [&](TreeNode&) {
306+
action_executed = true;
307+
return NodeStatus::SUCCESS;
308+
});
309+
310+
factory.loadSubstitutionRuleFromJSON(json_rules);
311+
factory.registerBehaviorTreeFromText(xml_text);
312+
auto tree = factory.createTree("MainTree");
313+
314+
auto future =
315+
std::async(std::launch::async, [&tree]() { return tree.tickWhileRunning(); });
316+
317+
auto status = future.wait_for(std::chrono::seconds(5));
318+
ASSERT_NE(status, std::future_status::timeout) << "Tree hung! tickWhileRunning did not "
319+
"complete within "
320+
"5 seconds";
321+
ASSERT_EQ(future.get(), NodeStatus::SUCCESS);
322+
ASSERT_TRUE(action_executed);
323+
}
324+
325+
// Test for issue #930: Verify that substituted node's registration
326+
// ID is preserved correctly for string-based substitution.
327+
TEST(Substitution, StringSubstitutionRegistrationID_Issue930)
328+
{
329+
static const char* xml_text = R"(
330+
<root BTCPP_format="4">
331+
<BehaviorTree ID="MainTree">
332+
<AlwaysSuccess name="target_node"/>
333+
</BehaviorTree>
334+
</root>
335+
)";
336+
337+
BehaviorTreeFactory factory;
338+
339+
factory.registerSimpleAction("MyReplacement",
340+
[](TreeNode&) { return NodeStatus::SUCCESS; });
341+
342+
factory.addSubstitutionRule("target_node", "MyReplacement");
343+
factory.registerBehaviorTreeFromText(xml_text);
344+
auto tree = factory.createTree("MainTree");
345+
346+
// The substituted node should still work correctly
347+
ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS);
348+
}

0 commit comments

Comments
 (0)