From 94274235edba8c3c1b06f68b2d71d4b5aa1c0fd9 Mon Sep 17 00:00:00 2001 From: "Kanykei.Imanalieva" Date: Mon, 20 Apr 2026 16:08:21 +0000 Subject: [PATCH 1/3] fix(xml_parsing): throw on duplicate BehaviorTree ID registration --- include/behaviortree_cpp/bt_factory.h | 31 +++- src/xml_parsing.cpp | 21 +++ tests/CMakeLists.txt | 1 + tests/gtest_duplicate_tree.cpp | 220 ++++++++++++++++++++++++++ tests/gtest_factory.cpp | 1 + tests/gtest_parallel.cpp | 2 + tests/gtest_subtree.cpp | 2 + 7 files changed, 276 insertions(+), 2 deletions(-) create mode 100644 tests/gtest_duplicate_tree.cpp diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 0ba3c86bb..85e49e061 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -316,11 +316,20 @@ class BehaviorTreeFactory * * where "tree_id" come from the XML attribute * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ void registerBehaviorTreeFromFile(const std::filesystem::path& filename); - /// Same of registerBehaviorTreeFromFile, but passing the XML text, - /// instead of the filename. + /** + * @brief Same as registerBehaviorTreeFromFile, but passing the XML text + * instead of the filename. + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. + */ void registerBehaviorTreeFromText(const std::string& xml_text); /// Returns the ID of the trees registered either with @@ -446,6 +455,10 @@ class BehaviorTreeFactory * @param text string containing the XML * @param blackboard blackboard of the root tree * @return the newly created tree + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromText( const std::string& text, Blackboard::Ptr blackboard = Blackboard::create()); @@ -460,11 +473,25 @@ class BehaviorTreeFactory * @param file_path location of the file to load * @param blackboard blackboard of the root tree * @return the newly created tree + * + * @throws RuntimeError if the XML registers a BehaviorTree ID that is + * already known to this factory. Call clearRegisteredBehaviorTrees() + * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromFile(const std::filesystem::path& file_path, Blackboard::Ptr blackboard = Blackboard::create()); + /** + * @brief createTree instantiates a tree by ID from definitions already + * present in the factory's parser. It does not load or parse XML; + * duplicate-ID errors come from registerBehaviorTreeFrom* or + * createTreeFrom*, not from createTree(). + * + * @param tree_name ID of the tree to instantiate + * @param blackboard blackboard of the root tree + * @return the newly created tree + */ [[nodiscard]] Tree createTree(const std::string& tree_name, Blackboard::Ptr blackboard = Blackboard::create()); diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index b4a82e976..c276f824a 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -48,6 +48,7 @@ #include #include +#include #ifdef USING_ROS2 #include @@ -243,10 +244,12 @@ struct XMLParser::PImpl std::list > opened_documents; std::map tree_roots; + std::unordered_map tree_sources; const BehaviorTreeFactory* factory = nullptr; std::filesystem::path current_path; + std::string current_file = ""; std::map subtree_models; int suffix_count = 0; @@ -261,6 +264,8 @@ struct XMLParser::PImpl current_path = std::filesystem::current_path(); opened_documents.clear(); tree_roots.clear(); + tree_sources.clear(); + current_file = ""; } private: @@ -294,6 +299,7 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc doc->LoadFile(filepath.string().c_str()); _p->current_path = std::filesystem::absolute(filepath.parent_path()); + _p->current_file = std::filesystem::absolute(filepath).string(); _p->loadDocImpl(doc, add_includes); } @@ -305,6 +311,8 @@ void XMLParser::loadFromText(const std::string& xml_text, bool add_includes) XMLDocument* doc = _p->opened_documents.back().get(); doc->Parse(xml_text.c_str(), xml_text.size()); + _p->current_file = ""; + _p->loadDocImpl(doc, add_includes); } @@ -418,13 +426,16 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) // change current path to the included file for handling additional relative paths const auto previous_path = current_path; + const std::string previous_file = current_file; current_path = std::filesystem::absolute(file_path.parent_path()); + current_file = std::filesystem::absolute(file_path).string(); next_doc->LoadFile(file_path.string().c_str()); loadDocImpl(next_doc, add_includes); // reset current path to the previous value current_path = previous_path; + current_file = previous_file; } // Collect the names of all nodes registered with the behavior tree factory @@ -457,7 +468,17 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) tree_name = "BehaviorTree_" + std::to_string(suffix_count++); } + const auto existing = tree_sources.find(tree_name); + if(existing != tree_sources.end()) + { + throw RuntimeError("Duplicate BehaviorTree ID '", tree_name, + "': first registered from '", existing->second, + "', re-registered from '", current_file, + "'. Call clearRegisteredBehaviorTrees() if you " + "intend to reload."); + } tree_roots[tree_name] = bt_node; + tree_sources[tree_name] = current_file; } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a009ee5ea..281062cc3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -23,6 +23,7 @@ set(BT_TESTS gtest_blackboard.cpp gtest_coroutines.cpp gtest_decorator.cpp + gtest_duplicate_tree.cpp gtest_enums.cpp gtest_factory.cpp gtest_fallback.cpp diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp new file mode 100644 index 000000000..aec8124af --- /dev/null +++ b/tests/gtest_duplicate_tree.cpp @@ -0,0 +1,220 @@ +/* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */ + +#include "behaviortree_cpp/bt_factory.h" + +#include +#include +#include +#include + +#include + +using namespace BT; + +namespace +{ +class TempSubdir +{ +public: + TempSubdir() + { + const auto stamp = + std::chrono::steady_clock::now().time_since_epoch().count(); + path = std::filesystem::temp_directory_path() / + ("bt_duplicate_tree_" + std::to_string(stamp)); + std::filesystem::create_directories(path); + } + + ~TempSubdir() + { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + + std::filesystem::path path; + + TempSubdir(const TempSubdir&) = delete; + TempSubdir& operator=(const TempSubdir&) = delete; +}; + +void writeFile(const std::filesystem::path& p, const std::string& content) +{ + std::ofstream out(p.string()); + out << content; +} + +std::string minimalTreeXml(const std::string& tree_id) +{ + return std::string(R"( + + + + + + )"); +} + +} // namespace + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFiles) +{ + TempSubdir dir; + const auto path_a = dir.path / "a.xml"; + const auto path_b = dir.path / "b.xml"; + writeFile(path_a, minimalTreeXml("DupTree")); + writeFile(path_b, minimalTreeXml("DupTree")); + + const std::string abs_a = std::filesystem::absolute(path_a).string(); + const std::string abs_b = std::filesystem::absolute(path_b).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); + + try + { + factory.registerBehaviorTreeFromFile(path_b); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_a), std::string::npos) << msg; + EXPECT_NE(msg.find(abs_b), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameText) +{ + const std::string xml = R"( + + + + +)"; + + BehaviorTreeFactory factory; + try + { + factory.registerBehaviorTreeFromText(xml); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(""), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFileAndText) +{ + TempSubdir dir; + const auto path = dir.path / "one.xml"; + writeFile(path, minimalTreeXml("shared_id")); + + const std::string abs_path = std::filesystem::absolute(path).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + + const std::string xml = minimalTreeXml("shared_id"); + try + { + factory.registerBehaviorTreeFromText(xml); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; + EXPECT_NE(msg.find(""), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, DuplicateIdSamePathTwice) +{ + TempSubdir dir; + const auto path = dir.path / "reload.xml"; + writeFile(path, minimalTreeXml("same")); + + const std::string abs_path = std::filesystem::absolute(path).string(); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + + try + { + factory.registerBehaviorTreeFromFile(path); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; + } +} + +TEST(DuplicateBehaviorTreeId, ClearAllowsReload) +{ + TempSubdir dir; + const auto path = dir.path / "clear.xml"; + writeFile(path, minimalTreeXml("cleared")); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + factory.clearRegisteredBehaviorTrees(); + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + ASSERT_NO_THROW(static_cast(factory.createTree("cleared"))); +} + +TEST(DuplicateBehaviorTreeId, DifferentIdsCoexist) +{ + TempSubdir dir; + const auto path_a = dir.path / "tree_a.xml"; + const auto path_b = dir.path / "tree_b.xml"; + writeFile(path_a, minimalTreeXml("TreeA")); + writeFile(path_b, minimalTreeXml("TreeB")); + + BehaviorTreeFactory factory; + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); + ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_b)); + + const auto ids = factory.registeredBehaviorTrees(); + ASSERT_EQ(ids.size(), 2); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeA"))); + ASSERT_NO_THROW(static_cast(factory.createTree("TreeB"))); +} + +TEST(DuplicateBehaviorTreeId, IncludeDuplicateThrows) +{ + TempSubdir dir; + const auto inner_path = dir.path / "inner.xml"; + const auto outer_path = dir.path / "outer.xml"; + + writeFile(inner_path, minimalTreeXml("foo")); + + const std::string outer_xml = R"( + + + + + + +)"; + writeFile(outer_path, outer_xml); + + const std::string inner_abs = + std::filesystem::absolute(inner_path).string(); + + BehaviorTreeFactory factory; + try + { + factory.registerBehaviorTreeFromFile( + std::filesystem::absolute(outer_path)); + FAIL() << "expected RuntimeError"; + } + catch(const RuntimeError& e) + { + const std::string msg = e.what(); + EXPECT_NE(msg.find(inner_abs), std::string::npos) << msg; + } +} diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 01718cbce..a186a54a8 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -400,6 +400,7 @@ TEST(BehaviorTreeReload, ReloadSameTree) ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning()); } + factory.clearRegisteredBehaviorTrees(); factory.registerBehaviorTreeFromText(xmlB); { auto tree = factory.createTree("MainTree"); diff --git a/tests/gtest_parallel.cpp b/tests/gtest_parallel.cpp index 81279921a..3f0b88350 100644 --- a/tests/gtest_parallel.cpp +++ b/tests/gtest_parallel.cpp @@ -486,6 +486,8 @@ TEST(Parallel, ParallelAll) ASSERT_EQ(1, observer.getStatistics("third").success_count); } + factory.clearRegisteredBehaviorTrees(); + { const char* xml_text = R"( diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index cda5d68a8..78bf9e172 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -126,6 +126,8 @@ TEST(SubTree, BadRemapping) Tree tree_bad_in = factory.createTree("MainTree"); EXPECT_ANY_THROW(tree_bad_in.tickWhileRunning()); + factory.clearRegisteredBehaviorTrees(); + static const char* xml_text_bad_out = R"( From 5a03b3e2258365b2505bdacc8b46f9cf43a3fe0a Mon Sep 17 00:00:00 2001 From: "Kanykei.Imanalieva" Date: Mon, 20 Apr 2026 16:51:34 +0000 Subject: [PATCH 2/3] format --- tests/gtest_duplicate_tree.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp index aec8124af..5f7bb2f4b 100644 --- a/tests/gtest_duplicate_tree.cpp +++ b/tests/gtest_duplicate_tree.cpp @@ -18,8 +18,7 @@ class TempSubdir public: TempSubdir() { - const auto stamp = - std::chrono::steady_clock::now().time_since_epoch().count(); + const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count(); path = std::filesystem::temp_directory_path() / ("bt_duplicate_tree_" + std::to_string(stamp)); std::filesystem::create_directories(path); @@ -47,7 +46,8 @@ std::string minimalTreeXml(const std::string& tree_id) { return std::string(R"( - + @@ -202,14 +202,12 @@ TEST(DuplicateBehaviorTreeId, IncludeDuplicateThrows) )"; writeFile(outer_path, outer_xml); - const std::string inner_abs = - std::filesystem::absolute(inner_path).string(); + const std::string inner_abs = std::filesystem::absolute(inner_path).string(); BehaviorTreeFactory factory; try { - factory.registerBehaviorTreeFromFile( - std::filesystem::absolute(outer_path)); + factory.registerBehaviorTreeFromFile(std::filesystem::absolute(outer_path)); FAIL() << "expected RuntimeError"; } catch(const RuntimeError& e) From 0bd6dea91765baeaa629da3895d724e454003ebc Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 24 Oct 2026 16:06:40 +0200 Subject: [PATCH 3/3] warn about duplicate BehaviorTree IDs instead of throwing Registering a BehaviorTree ID that already exists replaced the previous definition silently. Making it an error breaks legitimate uses of the deliberate last-wins behavior (a851c79c): registering the same file twice, calling createTreeFromText repeatedly on one factory, diamond includes, and replacing a subtree with a test double. Keep last-wins and print a warning instead, only when the previous definition came from a different source or from the same document; reloading a file, including it from two places, or reusing the same inline text stays silent. Take the source from the include stack added by #1250 instead of a second current_file variable, document the behavior in bt_factory.h, revert the clearRegisteredBehaviorTrees() calls added to existing tests, and turn the new tests into warning/last-wins checks, plus diamond-include and repeated-inline-text cases. Co-Authored-By: Claude Opus 5.5 --- include/behaviortree_cpp/bt_factory.h | 35 +---- src/xml_parsing.cpp | 29 ++-- tests/gtest_duplicate_tree.cpp | 199 +++++++++++++++----------- tests/gtest_factory.cpp | 1 - tests/gtest_parallel.cpp | 2 - tests/gtest_subtree.cpp | 2 - 6 files changed, 137 insertions(+), 131 deletions(-) diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index f48ed027e..3ac77ea11 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -320,20 +320,15 @@ class BehaviorTreeFactory * * where "tree_id" come from the XML attribute * - * @throws RuntimeError if the XML registers a BehaviorTree ID that is - * already known to this factory. Call clearRegisteredBehaviorTrees() - * first to replace previous definitions intentionally. + * If a tree with the same ID is already registered, the new definition + * replaces it. A warning is printed when the previous definition came from + * a different file (or from the same document); reloading the same file + * is silent. */ void registerBehaviorTreeFromFile(const std::filesystem::path& filename); - /** - * @brief Same as registerBehaviorTreeFromFile, but passing the XML text - * instead of the filename. - * - * @throws RuntimeError if the XML registers a BehaviorTree ID that is - * already known to this factory. Call clearRegisteredBehaviorTrees() - * first to replace previous definitions intentionally. - */ + /// Same of registerBehaviorTreeFromFile, but passing the XML text, + /// instead of the filename. void registerBehaviorTreeFromText(const std::string& xml_text); /// Returns the ID of the trees registered either with @@ -459,10 +454,6 @@ class BehaviorTreeFactory * @param text string containing the XML * @param blackboard blackboard of the root tree * @return the newly created tree - * - * @throws RuntimeError if the XML registers a BehaviorTree ID that is - * already known to this factory. Call clearRegisteredBehaviorTrees() - * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromText( const std::string& text, Blackboard::Ptr blackboard = Blackboard::create()); @@ -477,25 +468,11 @@ class BehaviorTreeFactory * @param file_path location of the file to load * @param blackboard blackboard of the root tree * @return the newly created tree - * - * @throws RuntimeError if the XML registers a BehaviorTree ID that is - * already known to this factory. Call clearRegisteredBehaviorTrees() - * first to replace previous definitions intentionally. */ [[nodiscard]] Tree createTreeFromFile(const std::filesystem::path& file_path, Blackboard::Ptr blackboard = Blackboard::create()); - /** - * @brief createTree instantiates a tree by ID from definitions already - * present in the factory's parser. It does not load or parse XML; - * duplicate-ID errors come from registerBehaviorTreeFrom* or - * createTreeFrom*, not from createTree(). - * - * @param tree_name ID of the tree to instantiate - * @param blackboard blackboard of the root tree - * @return the newly created tree - */ [[nodiscard]] Tree createTree(const std::string& tree_name, Blackboard::Ptr blackboard = Blackboard::create()); diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 292d99111..d3049a12a 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -254,7 +254,6 @@ struct XMLParser::PImpl const BehaviorTreeFactory* factory = nullptr; std::filesystem::path current_path; - std::string current_file = ""; // files currently being loaded through , used to detect cycles std::vector include_stack; std::map subtree_models; @@ -272,7 +271,6 @@ struct XMLParser::PImpl opened_documents.clear(); tree_roots.clear(); tree_sources.clear(); - current_file = ""; } private: @@ -306,7 +304,6 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc doc->LoadFile(filepath.string().c_str()); _p->current_path = std::filesystem::absolute(filepath.parent_path()); - _p->current_file = std::filesystem::absolute(filepath).string(); _p->include_stack = { IncludeKey(std::filesystem::absolute(filepath)) }; _p->loadDocImpl(doc, add_includes); @@ -319,7 +316,6 @@ void XMLParser::loadFromText(const std::string& xml_text, bool add_includes) XMLDocument* doc = _p->opened_documents.back().get(); doc->Parse(xml_text.c_str(), xml_text.size()); - _p->current_file = ""; _p->include_stack.clear(); _p->loadDocImpl(doc, add_includes); } @@ -450,9 +446,7 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) // change current path to the included file for handling additional relative paths const auto previous_path = current_path; - const std::string previous_file = current_file; current_path = std::filesystem::absolute(file_path.parent_path()); - current_file = std::filesystem::absolute(file_path).string(); include_stack.push_back(std::move(include_key)); next_doc->LoadFile(file_path.string().c_str()); @@ -461,7 +455,6 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) // reset current path to the previous value current_path = previous_path; - current_file = previous_file; } // Collect the names of all nodes registered with the behavior tree factory @@ -481,6 +474,9 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) loadSubtreeModel(xml_root); // Register each BehaviorTree within the XML + const std::string source = + include_stack.empty() ? "" : include_stack.back().string(); + std::unordered_set ids_in_document; for(auto bt_node = xml_root->FirstChildElement("BehaviorTree"); bt_node != nullptr; bt_node = bt_node->NextSiblingElement("BehaviorTree")) { @@ -494,17 +490,20 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) tree_name = "BehaviorTree_" + std::to_string(suffix_count++); } - const auto existing = tree_sources.find(tree_name); - if(existing != tree_sources.end()) + // An ID that is already registered is replaced by the new definition. + // Warn when the previous one came from another source, or from this same + // document; reloading a file, or including it twice, stays silent. + const auto previous = tree_sources.find(tree_name); + const bool repeated_in_document = !ids_in_document.insert(tree_name).second; + if(repeated_in_document || + (previous != tree_sources.end() && previous->second != source)) { - throw RuntimeError("Duplicate BehaviorTree ID '", tree_name, - "': first registered from '", existing->second, - "', re-registered from '", current_file, - "'. Call clearRegisteredBehaviorTrees() if you " - "intend to reload."); + std::cout << "Warning: BehaviorTree ID [" << tree_name << "] in [" << source + << "] replaces the one previously registered from [" + << (repeated_in_document ? source : previous->second) << "]" << std::endl; } tree_roots[tree_name] = bt_node; - tree_sources[tree_name] = current_file; + tree_sources[tree_name] = source; } } diff --git a/tests/gtest_duplicate_tree.cpp b/tests/gtest_duplicate_tree.cpp index 5f7bb2f4b..591d6a3dc 100644 --- a/tests/gtest_duplicate_tree.cpp +++ b/tests/gtest_duplicate_tree.cpp @@ -42,48 +42,69 @@ void writeFile(const std::filesystem::path& p, const std::string& content) out << content; } -std::string minimalTreeXml(const std::string& tree_id) +std::string minimalTreeXml(const std::string& tree_id, const std::string& node = "AlwaysS" + "uccess") { - return std::string(R"( + return R"( - + tree_id + R"("> + <)" + + node + + R"(/> - )"); + )"; +} + +// The parser reports included files by their canonical path (symlinks +// resolved, e.g. /var -> /private/var on macOS). +std::string canonicalPath(const std::filesystem::path& p) +{ + return std::filesystem::weakly_canonical(p).string(); +} + +// Runs the function and returns what it printed on stdout. +template +std::string captureStdout(Func&& func) +{ + testing::internal::CaptureStdout(); + func(); + return testing::internal::GetCapturedStdout(); +} + +bool hasWarning(const std::string& output) +{ + return output.find("Warning: BehaviorTree ID") != std::string::npos; } } // namespace -TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFiles) +// A BehaviorTree ID registered again replaces the previous definition (last +// wins), with a warning when the two come from different sources. + +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFilesWarnsAndLastWins) { TempSubdir dir; const auto path_a = dir.path / "a.xml"; const auto path_b = dir.path / "b.xml"; writeFile(path_a, minimalTreeXml("DupTree")); - writeFile(path_b, minimalTreeXml("DupTree")); - - const std::string abs_a = std::filesystem::absolute(path_a).string(); - const std::string abs_b = std::filesystem::absolute(path_b).string(); + writeFile(path_b, minimalTreeXml("DupTree", "AlwaysFailure")); BehaviorTreeFactory factory; - ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); - - try - { - factory.registerBehaviorTreeFromFile(path_b); - FAIL() << "expected RuntimeError"; - } - catch(const RuntimeError& e) - { - const std::string msg = e.what(); - EXPECT_NE(msg.find(abs_a), std::string::npos) << msg; - EXPECT_NE(msg.find(abs_b), std::string::npos) << msg; - } + ASSERT_FALSE( + hasWarning(captureStdout([&] { factory.registerBehaviorTreeFromFile(path_a); }))); + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(path_b); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(path_a)), std::string::npos) << output; + EXPECT_NE(output.find(canonicalPath(path_b)), std::string::npos) << output; + + auto tree = factory.createTree("DupTree"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); } -TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameText) +TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameTextWarns) { const std::string xml = R"( @@ -93,64 +114,81 @@ TEST(DuplicateBehaviorTreeId, DuplicateIdWithinSameText) )"; BehaviorTreeFactory factory; - try - { - factory.registerBehaviorTreeFromText(xml); - FAIL() << "expected RuntimeError"; - } - catch(const RuntimeError& e) - { - const std::string msg = e.what(); - EXPECT_NE(msg.find(""), std::string::npos) << msg; - } + const auto output = captureStdout([&] { factory.registerBehaviorTreeFromText(xml); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(""), std::string::npos) << output; + + auto tree = factory.createTree("foo"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); } -TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFileAndText) +TEST(DuplicateBehaviorTreeId, DuplicateIdAcrossFileAndTextWarns) { TempSubdir dir; const auto path = dir.path / "one.xml"; writeFile(path, minimalTreeXml("shared_id")); - const std::string abs_path = std::filesystem::absolute(path).string(); - BehaviorTreeFactory factory; - ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); - - const std::string xml = minimalTreeXml("shared_id"); - try - { - factory.registerBehaviorTreeFromText(xml); - FAIL() << "expected RuntimeError"; - } - catch(const RuntimeError& e) - { - const std::string msg = e.what(); - EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; - EXPECT_NE(msg.find(""), std::string::npos) << msg; - } + factory.registerBehaviorTreeFromFile(path); + const auto output = captureStdout( + [&] { factory.registerBehaviorTreeFromText(minimalTreeXml("shared_id")); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(path)), std::string::npos) << output; + EXPECT_NE(output.find(""), std::string::npos) << output; } -TEST(DuplicateBehaviorTreeId, DuplicateIdSamePathTwice) +TEST(DuplicateBehaviorTreeId, ReloadingSameFileIsSilent) { TempSubdir dir; const auto path = dir.path / "reload.xml"; writeFile(path, minimalTreeXml("same")); - const std::string abs_path = std::filesystem::absolute(path).string(); + BehaviorTreeFactory factory; + factory.registerBehaviorTreeFromFile(path); + const auto output = captureStdout([&] { factory.registerBehaviorTreeFromFile(path); }); + EXPECT_FALSE(hasWarning(output)) << output; + ASSERT_NO_THROW(static_cast(factory.createTree("same"))); +} +TEST(DuplicateBehaviorTreeId, RepeatedInlineTextIsSilent) +{ BehaviorTreeFactory factory; - ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path)); + const std::string xml = minimalTreeXml("MainTree"); + const auto output = captureStdout([&] { + factory.createTreeFromText(xml); + factory.createTreeFromText(xml); + }); + EXPECT_FALSE(hasWarning(output)) << output; +} - try - { - factory.registerBehaviorTreeFromFile(path); - FAIL() << "expected RuntimeError"; - } - catch(const RuntimeError& e) +TEST(DuplicateBehaviorTreeId, DiamondIncludeIsSilent) +{ + // top includes left and right, which both include the same shared file + TempSubdir dir; + writeFile(dir.path / "shared.xml", minimalTreeXml("Shared")); + for(const char* side : { "left", "right" }) { - const std::string msg = e.what(); - EXPECT_NE(msg.find(abs_path), std::string::npos) << msg; + writeFile(dir.path / (std::string(side) + ".xml"), std::string(R"( + + + +)"); } + writeFile(dir.path / "top.xml", R"( + + + + + + +)"); + + BehaviorTreeFactory factory; + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(dir.path / "top.xml"); }); + EXPECT_FALSE(hasWarning(output)) << output; + auto tree = factory.createTree("Top"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS); } TEST(DuplicateBehaviorTreeId, ClearAllowsReload) @@ -175,8 +213,11 @@ TEST(DuplicateBehaviorTreeId, DifferentIdsCoexist) writeFile(path_b, minimalTreeXml("TreeB")); BehaviorTreeFactory factory; - ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_a)); - ASSERT_NO_THROW(factory.registerBehaviorTreeFromFile(path_b)); + const auto output = captureStdout([&] { + factory.registerBehaviorTreeFromFile(path_a); + factory.registerBehaviorTreeFromFile(path_b); + }); + EXPECT_FALSE(hasWarning(output)) << output; const auto ids = factory.registeredBehaviorTrees(); ASSERT_EQ(ids.size(), 2); @@ -184,35 +225,29 @@ TEST(DuplicateBehaviorTreeId, DifferentIdsCoexist) ASSERT_NO_THROW(static_cast(factory.createTree("TreeB"))); } -TEST(DuplicateBehaviorTreeId, IncludeDuplicateThrows) +TEST(DuplicateBehaviorTreeId, IncludeDuplicateWarns) { TempSubdir dir; const auto inner_path = dir.path / "inner.xml"; const auto outer_path = dir.path / "outer.xml"; writeFile(inner_path, minimalTreeXml("foo")); - - const std::string outer_xml = R"( + writeFile(outer_path, R"( - + -)"; - writeFile(outer_path, outer_xml); - - const std::string inner_abs = std::filesystem::absolute(inner_path).string(); +)"); BehaviorTreeFactory factory; - try - { - factory.registerBehaviorTreeFromFile(std::filesystem::absolute(outer_path)); - FAIL() << "expected RuntimeError"; - } - catch(const RuntimeError& e) - { - const std::string msg = e.what(); - EXPECT_NE(msg.find(inner_abs), std::string::npos) << msg; - } + const auto output = + captureStdout([&] { factory.registerBehaviorTreeFromFile(outer_path); }); + EXPECT_TRUE(hasWarning(output)) << output; + EXPECT_NE(output.find(canonicalPath(inner_path)), std::string::npos) << output; + + // the including file is registered after its includes, so it wins + auto tree = factory.createTree("foo"); + EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::FAILURE); } diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index be0f3f546..1fe46a7a3 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -419,7 +419,6 @@ TEST(BehaviorTreeReload, ReloadSameTree) ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning()); } - factory.clearRegisteredBehaviorTrees(); factory.registerBehaviorTreeFromText(xmlB); { auto tree = factory.createTree("MainTree"); diff --git a/tests/gtest_parallel.cpp b/tests/gtest_parallel.cpp index 3f0b88350..81279921a 100644 --- a/tests/gtest_parallel.cpp +++ b/tests/gtest_parallel.cpp @@ -486,8 +486,6 @@ TEST(Parallel, ParallelAll) ASSERT_EQ(1, observer.getStatistics("third").success_count); } - factory.clearRegisteredBehaviorTrees(); - { const char* xml_text = R"( diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 42d871bdf..0709a316f 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -126,8 +126,6 @@ TEST(SubTree, BadRemapping) Tree tree_bad_in = factory.createTree("MainTree"); EXPECT_ANY_THROW(tree_bad_in.tickWhileRunning()); - factory.clearRegisteredBehaviorTrees(); - static const char* xml_text_bad_out = R"(