Noissue - Make comparable_xml safe against ambiguous tags

tests/test_packagerWebExt.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import difflib
 import json
 import os
 import re
 import shutil
 import zipfile
@@ skipping 222 matching lines @@
     files['ext/c.js'] = 'var this_is_c;'
 
     tmpdir.mkdir('lib').join('b.js').write(files['lib/b.js'])
     ext_dir = tmpdir.mkdir('ext')
     ext_dir.join('a.js').write(files['ext/a.js'])
     ext_dir.join('c.js').write(files['ext/c.js'])
 
     return files
 
 
-def comparable_json(json_string):
-    """Create a nonambiguous representation of a given JSON string."""
+def comparable_json(json_data):
+    """Create a nonambiguous representation of the given JSON data."""
+    if isinstance(json_data, basestring):
+        json_data = json.loads(json_data)
     return json.dumps(
-        json.loads(json_string), sort_keys=True, indent=0
+        json_data, sort_keys=True, indent=0
     ).split('\n')
 
 
 def comparable_xml(xml):
     """Create a nonambiguous representation of a given XML tree."""
     def strip(s):
         if s is None:
             return ''
         return s.strip()
 
     def transform(elt):
         subs = sorted(transform(s) for s in elt)
-        attrs = tuple('{}={}'.format(k, v) for k, v in elt.attrib.items())

[Vasily Kuznetsov, 2017/10/23 12:48:20]

Since you're doing a `comparable_json` later, we can just use `elt.attrib`
instead of this tuple comprehension thingy and `comparable_json` will handle
those dicts just fine, right?

[tlucas, 2017/10/23 13:21:36]

Good point - Done.

-        return (elt.tag, strip(elt.tail), strip(elt.text), attrs, subs)
-
-    return comparable_json(json.dumps(transform(ElementTree.fromstring(xml))))

[Vasily Kuznetsov, 2017/10/23 12:48:20]

Its kind of a pity that we're doing `json.dumps` here just to immediately
`json.loads` it in `comparable_json` but I guess it's just a few extra CPU
cycles so who cares.

[tlucas, 2017/10/23 13:21:36]

No, you are right - dumps(loads(dumps())) is definitely kind of ugly - Done.

+        return (elt.tag, strip(elt.tail), strip(elt.text), elt.attrib, subs)
+
+    return comparable_json(transform(ElementTree.fromstring(xml)))
 
 
 def assert_manifest_content(manifest, expected_path):
     extension = os.path.basename(expected_path).split('.')[-1]
 
     with open(expected_path, 'r') as fp:
         if extension == 'xml':
             generated = comparable_xml(manifest)
             expected = comparable_xml(fp.read())
         else:
@@ skipping 185 matching lines @@
             expected = os.path.join(
                 os.path.dirname(__file__),
                 'expecteddata',
                 filename.format(name, ext),
             )
 
             assert_manifest_content(
                 package.read(os.path.join(folder, '{}.{}'.format(name, ext))),
                 expected,
             )