Found a bug that sometimes made paths incorrect

Shoobx · Sep 12, 2018 · df5737b · df5737b
1 parent ba59ce5
commit df5737b
Show file tree

Hide file tree

Showing 11 changed files with 231 additions and 20 deletions.
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -4,7 +4,7 @@ Changes
 2.0b4 (unreleased)
 ------------------
 
-- Nothing changed yet.
+- Fixed some edge case bugs
 
 
 2.0b3 (2018-09-11)

diff --git a/tests/test_data/complex-text-update.expected.html b/tests/test_data/complex-text-update.expected.html
@@ -1,5 +1,5 @@
 <body xmlns:diff="http://namespaces.shoobx.com/diff">
   <div id="id">
-    <p><diff:insert>Let's see. </diff:insert>This is some simple text demonstrating the features of the <b diff:delete-formatting=""><i diff:insert-formatting="">human text differ</i></b>. This <u diff:delete-formatting="">feature</u> attempts to make changelog readable for humans. The <i diff:insert-formatting="">human text differ</i> uses sentences as its first order matching. <diff:delete>Let's see.</diff:delete><invalid><diff:insert>It should handle unknown tags just fine.</diff:insert></invalid></p>
+    <p><diff:insert>Let's see. </diff:insert>This is some simple text demonstrating the features of the <b diff:delete-formatting=""><i diff:insert-formatting="">human text differ</i></b>. This <u diff:delete-formatting="">feature</u> attempts to make changelog readable for humans.<br diff:insert=""/> The <i diff:insert-formatting="">human text differ</i> uses sentences as its first order matching. <diff:delete>Let's see.</diff:delete><invalid><diff:insert>It should handle unknown tags just fine.</diff:insert></invalid></p>
   </div>
 </body>
diff --git a/tests/test_data/complex-text-update.right.html b/tests/test_data/complex-text-update.right.html
@@ -4,7 +4,7 @@
     <p>
       Let's see. This is some simple text demonstrating the features of the
       <i>human text differ</i>. This feature attempts to make changelog
-      readable for humans. The <i>human text differ</i> uses sentences as its
+      readable for humans.<br/> The <i>human text differ</i> uses sentences as its
       first order matching. <invalid>It should handle unknown tags just fine.</invalid>
     </p>
 

diff --git a/tests/test_data/sbt_template.expected.xml b/tests/test_data/sbt_template.expected.xml
@@ -0,0 +1,17 @@
+<metal:block xmlns:app="http://namespaces.shoobx.com/application" xmlns:tal="http://xml.zope.org/namespaces/tal" xmlns:metal="http://xml.zope.org/namespaces/metal" xmlns:diff="http://namespaces.shoobx.com/diff" tal:define="         filename string:advisor-agreement.pdf;         title string:Advisor Agreement;">
+  <metal:block use-macro="macros/document">
+    <metal:block fill-slot="content">
+
+  <app:section diff:insert="" allowCustom="False" diff:add-attr="allowCustom;hidden;name;title" hidden="advisor.payment_type == 'none'" name="payment" title="Payment"><tal:if diff:insert="" condition="python: advisor.payment_type == 'stock_award'" diff:add-attr="condition"><para diff:insert=""><diff:insert>A</diff:insert><i diff:insert=""><diff:insert>whole</diff:insert></i><diff:insert>load of formatted text and</diff:insert><br diff:insert=""/><diff:insert>other stuff.</diff:insert></para></tal:if><tal:if diff:insert="" condition="python: advisor.payment_type == 'cash'" diff:add-attr="condition"><para diff:insert=""><diff:insert>More text for diffing purposes</diff:insert></para></tal:if><tal:if diff:insert="" condition="python: advisor.payment_type == 'stock_award_and_cash'" diff:add-attr="condition"><para diff:insert=""><diff:insert>Lorem hipster ipso facto</diff:insert></para></tal:if></app:section><app:section name="payment" title="Payment" hidden="advisor.stock_award == 'exclude'" allowCustom="False" diff:delete="">
+    <tal:if condition="python: advisor.stock_award != 'exclude'" diff:delete="">
+      <para diff:delete="">
+        We write <b diff:delete="">stuff here</b> that has nothing in common with the new text.
+      </para>
+    </tal:if>
+  </app:section>
+
+  <!--need to add company address and advisor address to signature block-->
+
+    </metal:block>
+  </metal:block>
+</metal:block>
diff --git a/tests/test_data/sbt_template.left.xml b/tests/test_data/sbt_template.left.xml
@@ -0,0 +1,27 @@
+
+<!DOCTYPE document SYSTEM "rml.dtd">
+<metal:block
+        xmlns:app="http://namespaces.shoobx.com/application"
+        xmlns:tal="http://xml.zope.org/namespaces/tal"
+        xmlns:metal="http://xml.zope.org/namespaces/metal"
+        xmlns:preview="http://namespaces.shoobx.com/preview"
+        tal:define="
+        filename string:advisor-agreement.pdf;
+        title string:Advisor Agreement;">
+  <metal:block use-macro="macros/document">
+    <metal:block fill-slot="content">
+
+  <app:section name="payment" title="Payment"
+               hidden="advisor.stock_award == 'exclude'"  allowCustom="False">
+    <tal:if condition="python: advisor.stock_award != 'exclude'">
+      <para>
+        We write <b>stuff here</b> that has nothing in common with the new text.
+      </para>
+    </tal:if>
+  </app:section>
+
+  <!--need to add company address and advisor address to signature block-->
+
+    </metal:block>
+  </metal:block>
+</metal:block>
diff --git a/tests/test_data/sbt_template.right.xml b/tests/test_data/sbt_template.right.xml
@@ -0,0 +1,37 @@
+
+<!DOCTYPE document SYSTEM "rml.dtd">
+<metal:block
+        xmlns:app="http://namespaces.shoobx.com/application"
+        xmlns:tal="http://xml.zope.org/namespaces/tal"
+        xmlns:metal="http://xml.zope.org/namespaces/metal"
+        xmlns:preview="http://namespaces.shoobx.com/preview"
+        tal:define="
+        filename string:advisor-agreement.pdf;
+        title string:Advisor Agreement;">
+  <metal:block use-macro="macros/document">
+    <metal:block fill-slot="content">
+
+  <app:section name="payment" title="Payment"
+               hidden="advisor.payment_type == 'none'"  allowCustom="False">
+     <tal:if condition="python: advisor.payment_type == 'stock_award'">
+      <para>
+	A <i>whole</i> load of formatted text and <br/> other stuff.
+      </para>
+    </tal:if>
+    <tal:if condition="python: advisor.payment_type == 'cash'">
+      <para>
+	More text for diffing purposes
+      </para>
+    </tal:if>
+    <tal:if condition="python: advisor.payment_type == 'stock_award_and_cash'">
+      <para>
+	Lorem hipster ipso facto
+      </para>
+    </tal:if>
+  </app:section>
+
+  <!--need to add company address and advisor address to signature block-->
+
+    </metal:block>
+  </metal:block>
+</metal:block>
diff --git a/tests/test_diff.py b/tests/test_diff.py
@@ -8,6 +8,8 @@
                           DeleteNode, UpdateAttrib, InsertAttrib, RenameAttrib,
                           DeleteAttrib, UpdateTextAfter, RenameNode)
 
+from .testing import compare_elements
+
 
 class APITests(unittest.TestCase):
     left = u"<document><p>Text</p><p>More</p></document>"
@@ -828,6 +830,7 @@ def _diff(self, left, right):
         differ = Differ()
         differ.set_trees(left_tree, right_tree)
         editscript = list(differ.diff())
+        compare_elements(differ.left, differ.right)
         return editscript
 
     def test_process(self):
@@ -867,8 +870,8 @@ def test_process(self):
                 InsertAttrib('/document/story/section[2]', 'single-ref', '4'),
                 MoveNode('/document/story/section[1]/para[3]',
                          '/document/story/section[2]', 0),
-                InsertNode('/document/story/section[2]', 'para', 0),
-                UpdateTextIn('/document/story/section[2]/para[1]',
+                InsertNode('/document/story/section[2]', 'para', 1),
+                UpdateTextIn('/document/story/section[2]/para[2]',
                              'Fourth paragraph'),
                 DeleteNode('/document/story/deleteme/para[1]'),
                 DeleteNode('/document/story/deleteme[1]'),
@@ -895,7 +898,7 @@ def test_no_root_match(self):
         self.assertEqual(
             result,
             [
-                DeleteAttrib(node='/root[1]', name='attr'),
+                DeleteAttrib('/root[1]', 'attr'),
                 MoveNode('/root/n[1]', '/root[1]', 1),
                 MoveNode('/root/n[2]/p[2]', '/root/n[1]', 0),
             ]
@@ -1040,6 +1043,115 @@ def test_rmldoc(self):
             ]
         )
 
+    def test_sbt_template(self):
+        here = os.path.split(__file__)[0]
+        lfile = os.path.join(here, 'test_data', 'sbt_template.left.xml')
+        rfile = os.path.join(here, 'test_data', 'sbt_template.right.xml')
+        with open(lfile, 'rt', encoding='utf8') as infile:
+            left = infile.read()
+        with open(rfile, 'rt', encoding='utf8') as infile:
+            right = infile.read()
+
+        result = self._diff(left, right)
+
+        # Most lines get too long and flake8 complains because of this part:
+        bm_bm_bm = '/metal:block/metal:block/metal:block'
+        self.assertEqual(
+            result,
+            [
+                InsertNode(
+                    bm_bm_bm + '[1]',
+                    '{http://namespaces.shoobx.com/application}section',
+                    0),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]',
+                    'allowCustom',
+                    'False'),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]',
+                    'hidden',
+                    "advisor.payment_type == 'none'"),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]',
+                    'name',
+                    'payment'),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]',
+                    'title',
+                    'Payment'),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]',
+                    '{http://xml.zope.org/namespaces/tal}if',
+                    0),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]',
+                    'condition',
+                    "python: advisor.payment_type == 'stock_award'"),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]',
+                    '{http://xml.zope.org/namespaces/tal}if',
+                    1),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]/tal:if[2]',
+                    'condition',
+                    "python: advisor.payment_type == 'cash'"),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]',
+                    '{http://xml.zope.org/namespaces/tal}if',
+                    2),
+                InsertAttrib(
+                    bm_bm_bm + '/app:section[1]/tal:if[3]',
+                    'condition',
+                    "python: advisor.payment_type == 'stock_award_and_cash'"),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]',
+                    'para',
+                    0),
+                UpdateTextIn(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para[1]',
+                    '\n\tA '),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para[1]',
+                    'i',
+                    0),
+                UpdateTextIn(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para/i[1]',
+                    'whole'),
+                UpdateTextAfter(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para/i[1]',
+                    ' load of formatted text and '),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para[1]',
+                    'br',
+                    1),
+                UpdateTextAfter(
+                    bm_bm_bm + '/app:section[1]/tal:if[1]/para/br[1]',
+                    ' other stuff.\n      '),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]/tal:if[2]',
+                    'para',
+                    0),
+                UpdateTextIn(
+                    bm_bm_bm + '/app:section[1]/tal:if[2]/para[1]',
+                    '\n\tMore text for diffing purposes\n      '),
+                InsertNode(
+                    bm_bm_bm + '/app:section[1]/tal:if[3]',
+                    'para',
+                    0),
+                UpdateTextIn(
+                    bm_bm_bm + '/app:section[1]/tal:if[3]/para[1]',
+                    '\n\tLorem hipster ipso facto\n      '),
+                DeleteNode(
+                    bm_bm_bm + '/app:section[2]/tal:if/para/b[1]'),
+                DeleteNode(
+                    bm_bm_bm + '/app:section[2]/tal:if/para[1]'),
+                DeleteNode(
+                    bm_bm_bm + '/app:section[2]/tal:if[1]'),
+                DeleteNode(
+                    bm_bm_bm + '/app:section[2]')
+            ]
+        )
+
     def test_namespace(self):
         # Test changing nodes and attributes with namespaces
         left = u"""<document xmlns:app="someuri">
@@ -1094,8 +1206,9 @@ def test_namespace(self):
                 RenameNode(
                     '/document/story/app:section/foo:para[1]',
                     '{someuri}para'),
-                InsertAttrib('/document/story/app:section/app:para[3]',
-                             '{someuri}attrib', 'value'),
+                InsertAttrib(
+                    '/document/story/app:section/app:para[3]',
+                    '{someuri}attrib', 'value'),
             ]
         )
 
@@ -1120,10 +1233,10 @@ def test_multiple_tag_deletes(self):
         result = self._diff(left, right)
         self.assertEqual(
             result,
-            [UpdateTextIn(node='/document/story[1]', text='\n    '),
-             DeleteNode(node='/document/story/ul/li[3]'),
-             DeleteNode(node='/document/story/ul/li[2]'),
-             DeleteNode(node='/document/story/ul/li[1]'),
-             DeleteNode(node='/document/story/ul[1]'),
+            [UpdateTextIn('/document/story[1]', '\n    '),
+             DeleteNode('/document/story/ul/li[3]'),
+             DeleteNode('/document/story/ul/li[2]'),
+             DeleteNode('/document/story/ul/li[1]'),
+             DeleteNode('/document/story/ul[1]'),
              ]
         )
diff --git a/tests/test_formatting.py b/tests/test_formatting.py
@@ -385,6 +385,8 @@ class XMLFormatterFileTests(FormatterFileTests):
 class HTMLFormatterFileTests(FormatterFileTests):
 
     # We use the HTMLFormatter for the placeholder tests
+    # <br/> is missing from this intentionally, to test an edge case
+    # with empty non-formatting tags in text.
     formatter = formatting.XMLFormatter(
         normalize=formatting.WS_BOTH,
         pretty_print=True,

diff --git a/tests/testing.py b/tests/testing.py
@@ -35,3 +35,14 @@ def generate_filebased_cases(data_dir, test_class, suffix='xml', ignore=()):
         function_name = os.path.split(left_filename)[-1].replace('.', '-')
         test_name = 'test_' + function_name
         setattr(test_class, test_name, test_function)
+
+
+def compare_elements(left, right):
+    path = left.getroottree().getpath(left)
+    assert left.text == right.text, "Texts differ: %s" % path
+    assert left.tail == right.tail, "Tails differ: %s" % path
+    assert left.attrib == right.attrib, "Attributes differ: %s" % path
+    # We intentionally do NOT compare namespaces, they are allowed to differ
+    assert len(left) == len(right), "Children differ" % path
+    for l, r in zip(left.getchildren(), right.getchildren()):
+        compare_elements(l, r)
diff --git a/xmldiff/diff.py b/xmldiff/diff.py
@@ -200,7 +200,6 @@ def update_node_tag(self, left, right):
 
     def update_node_attr(self, left, right):
         left_xpath = utils.getpath(left)
-        right_xpath = utils.getpath(right)
 
         # Update: Look for differences in attributes
 
@@ -238,7 +237,7 @@ def update_node_attr(self, left, right):
 
         # Insert: Find new attributes
         for key in sorted(new_keys):
-            yield InsertAttrib(right_xpath, key, right.attrib[key])
+            yield InsertAttrib(left_xpath, key, right.attrib[key])
             left.attrib[key] = right.attrib[key]
 
         # Delete: remove removed attributes
@@ -270,13 +269,13 @@ def find_pos(self, child):
         # deals with the case of no child being in order.
 
         # Find the last sibling before the child that is in order
-        i = parent.index(child) - 1
-        while i >= 0:
+        i = parent.index(child)
+        while i >= 1:
+            i -= 1
             sibling = parent[i]
             if sibling in self._inorder:
                 # That's it
                 break
-            i -= 1
         else:
             # No previous sibling in order.
             return 0
@@ -387,6 +386,8 @@ def diff(self, left=None, right=None):
                     # Move the node from current parent to target
                     lparent.remove(lnode)
                     ltarget.insert(pos, lnode)
+                    self._inorder.add(lnode)
+                    self._inorder.add(rnode)
 
             # (d) Align
             for action in self.align_children(lnode, rnode):

diff --git a/xmldiff/formatting.py b/xmldiff/formatting.py
@@ -228,9 +228,12 @@ def mark_diff(self, ph, action):
             # Formatting element, add a diff attribute
             action += '-formatting'
             elem.attrib['{%s}%s' % (DIFF_NS, action)] = ''
-        else:
-            # Not formatting, wrap content
+        elif elem.text:
+            # Not formatting, but it has content, wrap content
             elem.text = self.wrap_diff(elem.text, action)
+        else:
+            # No content, not formatting
+            elem.attrib['{%s}%s' % (DIFF_NS, action)] = ''
 
         # And make a new placeholder for this new entry:
         return self.get_placeholder(elem, entry.ttype, entry.close_ph)