Merge pull request #16 from PickNikRobotics/add-id-to-validation

Refactor VerifyXML to clarify logic

Author: EzraBrooks

package.xml

@@ -24,6 +24,7 @@
   <depend>libzmq3-dev</depend>
 
   <test_depend >ament_cmake_gtest</test_depend>
+  <test_depend >ament_cmake_gmock</test_depend>
 
   <export>
       <build_type >ament_cmake</build_type>

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 <Decorator> must have exactly 1 "
-                                "child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <Decorator> must have the "
-                                "attribute [ID]");
-      }
-    }
-    else if(name == "Action")
-    {
-      if(children_count != 0)
-      {
-        ThrowError(line_number, "The tag <Action> must not have any "
-                                "child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <Action> 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 <Condition> must not have any "
-                                "child");
-      }
-      if(ID.empty())
+      if(ID.empty() && behavior_tree_count > 1)
       {
-        ThrowError(line_number, "The tag <Condition> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number, "The tag <BehaviorTree> must have the attribute [ID]");
       }
-    }
-    else if(name == "Control")
-    {
-      if(children_count == 0)
+      if(registered_nodes.count(ID) != 0)
       {
-        ThrowError(line_number, "The tag <Control> must have at least 1 "
-                                "child");
+        ThrowError(line_number, "The attribute [ID] of tag <BehaviorTree> must not use "
+                                "the name of a registered Node");
       }
-      if(ID.empty())
+      if(children_count != 1)
       {
-        ThrowError(line_number, "The tag <Control> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number, "The tag <BehaviorTree> with ID '" + ID +
+                                    "' must have exactly 1 child");
       }
     }
     else if(name == "SubTree")
     {
       if(children_count != 0)
       {
-        ThrowError(line_number, "<SubTree> should not have any child");
-      }
-      if(ID.empty())
-      {
-        ThrowError(line_number, "The tag <SubTree> must have the "
-                                "attribute [ID]");
+        ThrowError(line_number,
+                   "<SubTree> with ID '" + ID + "' should not have any child");
       }
       if(registered_nodes.count(ID) != 0)
       {
-        ThrowError(line_number, "The attribute [ID] of tag <SubTree> 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 <BehaviorTree> must have the "
-                                "attribute [ID]");
-      }
-      if(children_count != 1)
-      {
-        ThrowError(line_number, "The tag <BehaviorTree> must have exactly 1 "
-                                "child");
-      }
-      if(registered_nodes.count(ID) != 0)
-      {
-        ThrowError(line_number, "The attribute [ID] of tag <BehaviorTree> "
-                                "must not use the name of a registered Node");
+        ThrowError(line_number, "The attribute [ID] of tag <SubTree> 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;

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()
 

tests/gtest_factory.cpp

@@ -1,4 +1,5 @@
 #include <gtest/gtest.h>
+#include <gmock/gmock.h>
 #include <filesystem>
 #include <string>
 #include <utility>
@@ -83,7 +84,7 @@ static const char* xml_text_subtree_part1 = R"(
   <BehaviorTree ID="MainTree">
     <Fallback name="root_selector">
       <SubTree ID="DoorClosedSubtree" />
-      <Action ID="PassThroughWindow" />
+      <Action ID="PassThroughDoor" />
     </Fallback>
   </BehaviorTree>
 </root>  )";
@@ -94,11 +95,10 @@ static const char* xml_text_subtree_part2 = R"(
   <BehaviorTree ID="DoorClosedSubtree">
     <Sequence name="door_sequence">
       <Decorator ID="Inverter">
-        <Action ID="IsDoorLocked" />
+        <Action ID="IsDoorClosed" />
       </Decorator>
       <Action ID="OpenDoor" />
       <Action ID="PassThroughDoor" />
-      <Action ID="CloseDoor" />
     </Sequence>
   </BehaviorTree>
 </root>  )";
@@ -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"));
   }
 }