virtinst: Add full test coverage for xml*.py files

.coveragerc

@@ -0,0 +1,7 @@
+[report]
+exclude_lines =
+    # Have to re-enable the standard pragma
+    pragma: no cover
+
+    # Don't complain if tests don't hit defensive assertion code:
+    raise NotImplementedError

tests/xmlparse.py

@@ -1448,13 +1448,43 @@ class XMLParseTest(unittest.TestCase):
         except ValueError:
             pass
 
-    def testXMLRootValidate(self):
+    def testXMLCoverage(self):
         try:
+            # Ensure we validate root element
             virtinst.DeviceDisk(self.conn, parsexml="<foo/>")
             raise AssertionError("Expected parse failure")
         except RuntimeError as e:
             self.assertTrue("'foo'" in str(e))
 
+        try:
+            # Ensure we validate root element
+            virtinst.DeviceDisk(self.conn, parsexml=-1)
+            raise AssertionError("Expected parse failure")
+        except Exception as e:
+            self.assertTrue("xmlParseDoc" in str(e))
+
+        try:
+            from virtinst import xmlutil
+            xmlutil.raise_programming_error(True, "for coverage")
+            raise AssertionError("We shouldn't get here")
+        except RuntimeError:
+            pass
+
+        try:
+            virtinst.DeviceDisk.validate_generic_name("objtype", None)
+            raise AssertionError("We shouldn't get here")
+        except ValueError:
+            pass
+        try:
+            virtinst.DeviceDisk.validate_generic_name("objtype", "foo bar")
+            raise AssertionError("We shouldn't get here")
+        except ValueError:
+            pass
+
+        # Test property __repr__ for code coverage
+        assert str(virtinst.DeviceDisk.address)
+        assert str(virtinst.DeviceDisk.driver_cache)
+
     def testReplaceChildParse(self):
         buildfile = "tests/xmlparse-xml/replace-child-build.xml"
         parsefile = "tests/xmlparse-xml/replace-child-parse.xml"

virtinst/xmlapi.py

@@ -224,8 +224,7 @@ class _XMLBase(object):
         xpathobj = _XPath(fullxpath)
         parentxpath = "."
         parentnode = self._find(parentxpath)
-        if parentnode is None:
+        xmlutil.raise_programming_error(not parentnode,
-            raise RuntimeError("programming error: "
                 "Did not find XML root node for xpath=%s" % fullxpath)
 
         for xpathseg in xpathobj.segments[1:]:
@@ -287,15 +286,15 @@ class _Libxml2API(_XMLBase):
             self._ctx.xpathRegisterNs(key, val)
 
     def __del__(self):
+        if not hasattr(self, "_doc"):
+            # Incase we error when parsing the doc
+            return
         self._doc.freeDoc()
         self._doc = None
         self._ctx.xpathFreeContext()
         self._ctx = None
 
     def _sanitize_xml(self, xml):
-        # Strip starting <?...> line
-        if xml.startswith("<?"):
-            ignore, xml = xml.split("\n", 1)
         if not xml.endswith("\n") and "\n" in xml:
             xml += "\n"
         return xml

virtinst/xmlbuilder.py

@@ -192,8 +192,8 @@ class XMLProperty(_XMLPropertyBase):
         :param do_abspath: If True, run os.path.abspath on the passed value
         """
         self._xpath = xpath
-        if not self._xpath:
+        xmlutil.raise_programming_error(not self._xpath,
-            raise RuntimeError("XMLProperty: xpath must be passed.")
+                "XMLProperty: xpath must be passed.")
 
         self._is_bool = is_bool
         self._is_int = is_int
@@ -201,10 +201,11 @@ class XMLProperty(_XMLPropertyBase):
         self._is_onoff = is_onoff
         self._do_abspath = do_abspath
 
-        if sum([int(bool(i)) for i in
+        conflicts = sum([int(bool(i)) for i in
                 [self._is_bool, self._is_int,
-                 self._is_yesno, self._is_onoff]]) > 1:
+                 self._is_yesno, self._is_onoff]])
-            raise RuntimeError("Conflict property converter options.")
+        xmlutil.raise_programming_error(conflicts > 1,
+                "Conflict property converter options.")
 
         self._is_tracked = False
         if _trackprops:
@@ -405,8 +406,6 @@ class _XMLState(object):
         self._parent_xpath = xpath or ""
 
     def _join_xpath(self, x1, x2):
-        if x1.endswith("/"):
-            x1 = x1[:-1]
         if x2.startswith("."):
             x2 = x2[1:]
         return x1 + x2
@@ -478,11 +477,7 @@ class XMLBuilder(object):
         self.conn = conn
 
         if self._XML_SANITIZE:
-            if hasattr(parsexml, 'decode'):
+            parsexml = parsexml.encode("ascii", "ignore").decode("ascii")
-                parsexml = parsexml.decode("ascii", "ignore").encode("ascii")
-            else:
-                parsexml = parsexml.encode("ascii", "ignore").decode("ascii")
-
             parsexml = "".join([c for c in parsexml if c in string.printable])
 
         self._propstore = collections.OrderedDict()
@@ -502,15 +497,16 @@ class XMLBuilder(object):
         xmlprops = self._all_xml_props()
         childprops = self._all_child_props()
         for key in self._XML_PROP_ORDER:
-            if key not in xmlprops and key not in childprops:
+            xmlutil.raise_programming_error(
-                raise RuntimeError("programming error: key '%s' must be "
+                key not in xmlprops and key not in childprops,
-                                   "xml prop or child prop" % key)
+                "key '%s' must be xml prop or child prop" % key)
 
         childclasses = []
         for childprop in childprops.values():
-            if childprop.child_class in childclasses:
+            xmlutil.raise_programming_error(
-                raise RuntimeError("programming error: can't register "
+                childprop.child_class in childclasses,
-                        "duplicate child_classs=%s" % childprop.child_class)
+                "can't register duplicate child_class=%s" %
+                childprop.child_class)
             childclasses.append(childprop.child_class)
 
         setattr(self.__class__, cachekey, True)
@@ -636,14 +632,16 @@ class XMLBuilder(object):
 
     def _find_child_prop(self, child_class):
         xmlprops = self._all_child_props()
+        ret = None
         for xmlprop in list(xmlprops.values()):
             if xmlprop.is_single:
                 continue
             if child_class is xmlprop.child_class:
-                return xmlprop
+                ret = xmlprop
-        raise RuntimeError("programming error: "
+                break
-                           "Didn't find child property for child_class=%s" %
+        xmlutil.raise_programming_error(not ret,
-                           child_class)
+                "Didn't find child property for child_class=%s" % child_class)
+        return ret
 
     def _set_xpaths(self, parent_xpath, relative_object_xpath=-1):
         """

virtinst/xmlutil.py

@@ -57,3 +57,11 @@ def set_prop_path(obj, prop_path, value):
         parent = getattr(parent, piece)
 
     return setattr(parent, pieces[-1], value)
+
+
+def raise_programming_error(cond, msg):
+    """
+    Small helper to raise a consistent error message for coding issues
+    """
+    if cond:
+        raise RuntimeError("programming error: %s" % msg)