diff --git a/package.xml b/package.xml index 1d245b625..e1d88e8b1 100644 --- a/package.xml +++ b/package.xml @@ -24,6 +24,7 @@ libzmq3-dev ament_cmake_gtest + ament_cmake_gmock ament_cmake diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index cf45c568b..5e38910d9 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -429,119 +429,76 @@ void VerifyXML(const std::string& xml_text, const std::string ID = node->Attribute("ID") ? node->Attribute("ID") : ""; const int line_number = node->GetLineNum(); - if(name == "Decorator") + // Precondition: built-in XML element types must define attribute [ID] + const bool is_builtin = + (name == "Decorator" || name == "Action" || name == "Condition" || + name == "Control" || name == "SubTree"); + if(is_builtin && ID.empty()) { - if(children_count != 1) - { - ThrowError(line_number, "The tag must have exactly 1 " - "child"); - } - if(ID.empty()) - { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); - } - } - else if(name == "Action") - { - if(children_count != 0) - { - ThrowError(line_number, "The tag must not have any " - "child"); - } - if(ID.empty()) - { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); - } + ThrowError(line_number, + std::string("The tag <") + name + "> must have the attribute [ID]"); } - else if(name == "Condition") + + if(name == "BehaviorTree") { - if(children_count != 0) - { - ThrowError(line_number, "The tag must not have any " - "child"); - } - if(ID.empty()) + if(ID.empty() && behavior_tree_count > 1) { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); + ThrowError(line_number, "The tag must have the attribute [ID]"); } - } - else if(name == "Control") - { - if(children_count == 0) + if(registered_nodes.count(ID) != 0) { - ThrowError(line_number, "The tag must have at least 1 " - "child"); + ThrowError(line_number, "The attribute [ID] of tag must not use " + "the name of a registered Node"); } - if(ID.empty()) + if(children_count != 1) { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); + ThrowError(line_number, "The tag with ID '" + ID + + "' must have exactly 1 child"); } } else if(name == "SubTree") { if(children_count != 0) { - ThrowError(line_number, " should not have any child"); - } - if(ID.empty()) - { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); + ThrowError(line_number, + " with ID '" + ID + "' should not have any child"); } if(registered_nodes.count(ID) != 0) { - ThrowError(line_number, "The attribute [ID] of tag must " - "not use the name of a registered Node"); - } - } - else if(name == "BehaviorTree") - { - if(ID.empty() && behavior_tree_count > 1) - { - ThrowError(line_number, "The tag must have the " - "attribute [ID]"); - } - if(children_count != 1) - { - ThrowError(line_number, "The tag must have exactly 1 " - "child"); - } - if(registered_nodes.count(ID) != 0) - { - ThrowError(line_number, "The attribute [ID] of tag " - "must not use the name of a registered Node"); + ThrowError(line_number, "The attribute [ID] of tag must not use the " + "name of a registered Node"); } } else { - // search in the factory and the list of subtrees - const auto search = registered_nodes.find(name); + // use ID for builtin node types, otherwise use the element name + const auto lookup_name = is_builtin ? ID : name; + const auto search = registered_nodes.find(lookup_name); bool found = (search != registered_nodes.end()); if(!found) { - ThrowError(line_number, std::string("Node not recognized: ") + name); + ThrowError(line_number, std::string("Node not recognized: ") + lookup_name); } - if(search->second == NodeType::DECORATOR) + const auto node_type = search->second; + const std::string& registered_name = search->first; + + if(node_type == NodeType::DECORATOR) { if(children_count != 1) { - ThrowError(line_number, - std::string("The node <") + name + "> must have exactly 1 child"); + ThrowError(line_number, std::string("The node '") + registered_name + + "' must have exactly 1 child"); } } - else if(search->second == NodeType::CONTROL) + else if(node_type == NodeType::CONTROL) { if(children_count == 0) { - ThrowError(line_number, - std::string("The node <") + name + "> must have 1 or more children"); + ThrowError(line_number, std::string("The node '") + registered_name + + "' must have 1 or more children"); } - if(name == "ReactiveSequence") + if(registered_name == "ReactiveSequence") { size_t async_count = 0; for(auto child = node->FirstChildElement(); child != nullptr; @@ -563,13 +520,29 @@ void VerifyXML(const std::string& xml_text, ++async_count; if(async_count > 1) { - ThrowError(line_number, std::string("A ReactiveSequence cannot have more " - "than one async child.")); + ThrowError(line_number, std::string("A ReactiveSequence cannot have " + "more than one async child.")); } } } } } + else if(node_type == NodeType::ACTION) + { + if(children_count != 0) + { + ThrowError(line_number, std::string("The node '") + registered_name + + "' must not have any child"); + } + } + else if(node_type == NodeType::CONDITION) + { + if(children_count != 0) + { + ThrowError(line_number, std::string("The node '") + registered_name + + "' must not have any child"); + } + } } //recursion for(auto child = node->FirstChildElement(); child != nullptr; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9526b0951..c00c5987b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -37,8 +37,9 @@ set(BT_TESTS if(ament_cmake_FOUND) find_package(ament_cmake_gtest REQUIRED) + find_package(ament_cmake_gmock REQUIRED) - ament_add_gtest(${BTCPP_LIBRARY}_test ${BT_TESTS}) + ament_add_gmock(${BTCPP_LIBRARY}_test ${BT_TESTS}) target_link_libraries(${BTCPP_LIBRARY}_test ${TEST_DEPENDECIES} ${ament_LIBRARIES}) @@ -53,7 +54,9 @@ else() target_link_libraries(behaviortree_cpp_test GTest::gtest - GTest::gtest_main) + GTest::gtest_main + GTest::gmock + ) endif() diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index b5679d729..c4833f6e0 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -83,7 +84,7 @@ static const char* xml_text_subtree_part1 = R"( - + )"; @@ -94,11 +95,10 @@ static const char* xml_text_subtree_part2 = R"( - + - )"; @@ -259,7 +259,7 @@ TEST(BehaviorTreeFactory, SubtreeParsingError) catch(const BT::RuntimeError& e) { std::string error_msg = e.what(); - EXPECT_TRUE(error_msg.find("line 36") != std::string::npos); + EXPECT_THAT(error_msg, ::testing::HasSubstr("line 11")); } try { @@ -269,7 +269,7 @@ TEST(BehaviorTreeFactory, SubtreeParsingError) catch(const BT::RuntimeError& e) { std::string error_msg = e.what(); - EXPECT_TRUE(error_msg.find("line 7") != std::string::npos); + EXPECT_THAT(error_msg, ::testing::HasSubstr("line 5")); } }