Skip to content

Commit 1691c1d

Browse files
facontidavideclaude
facontidavide
and
claude
committed
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>
1 parent e0684b0 commit 1691c1d

2 files changed

Lines changed: 286 additions & 21 deletions

File tree

‎src/bt_factory.cpp‎

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

427427
std::unordered_map<std::string, TestNodeConfig> configs;
428428

429-
auto test_configs = json.at("TestNodeConfigs");
430-
for(auto const& [name, test_config] : test_configs.items())
429+
// TestNodeConfigs is optional: users may only have string-based
430+
// substitution rules that map to already-registered node types.
431+
if(json.contains("TestNodeConfigs"))
431432
{
432-
auto& config = configs[name];
433-
434-
auto return_status = test_config.at("return_status").get<std::string>();
435-
config.return_status = convertFromString<NodeStatus>(return_status);
436-
if(test_config.contains("async_delay"))
437-
{
438-
config.async_delay =
439-
std::chrono::milliseconds(test_config["async_delay"].get<int>());
440-
}
441-
if(test_config.contains("post_script"))
442-
{
443-
config.post_script = test_config["post_script"].get<std::string>();
444-
}
445-
if(test_config.contains("success_script"))
446-
{
447-
config.success_script = test_config["success_script"].get<std::string>();
448-
}
449-
if(test_config.contains("failure_script"))
433+
auto test_configs = json.at("TestNodeConfigs");
434+
for(auto const& [name, test_config] : test_configs.items())
450435
{
451-
config.failure_script = test_config["failure_script"].get<std::string>();
436+
auto& config = configs[name];
437+
438+
auto return_status = test_config.at("return_status").get<std::string>();
439+
config.return_status = convertFromString<NodeStatus>(return_status);
440+
if(test_config.contains("async_delay"))
441+
{
442+
config.async_delay =
443+
std::chrono::milliseconds(test_config["async_delay"].get<int>());
444+
}
445+
if(test_config.contains("post_script"))
446+
{
447+
config.post_script = test_config["post_script"].get<std::string>();
448+
}
449+
if(test_config.contains("success_script"))
450+
{
451+
config.success_script = test_config["success_script"].get<std::string>();
452+
}
453+
if(test_config.contains("failure_script"))
454+
{
455+
config.failure_script = test_config["failure_script"].get<std::string>();
456+
}
452457
}
453458
}
454459

‎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;
@@ -50,3 +52,261 @@ TEST(Substitution, Parser)
5052

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

0 commit comments

Comments
 (0)