Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/tools.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 os
 import shutil
 import zipfile
 import json
+import difflib
 
 from xml.etree import ElementTree
 
 from buildtools.packagerChrome import defaultLocale
 from buildtools.tests.conftest import MESSAGES_EN_US
 
 
 ALL_LANGUAGES = ['en_US', 'de', 'it']
 
 
@@ skipping 81 matching lines @@
 
 
 def copy_metadata(filename, tmpdir):
     """Copy the used metadata to the used temporary directory."""
     path = os.path.join(os.path.dirname(__file__), filename)
     destination = str(tmpdir.join(filename))
     shutil.copy(path, destination)
 
 
 def run_webext_build(ext_type, build_type, srcdir, packager_module,
-                     keyfile=None):
-    """Run a build process
+                     buildnum=None, keyfile=None):
+    """Run a build process.
 
     Run a desired build process without unintenionally invoking the
     releaseAutomation.
     """
     from buildtools.build import processArgs
     args = ['build.py', '-t', ext_type, build_type]
 
     if keyfile:
         args += ['-k', keyfile]
+    if buildnum:
+        args += ['-b', buildnum]
 
     if not build_type == 'release':
         processArgs(str(srcdir), args)
     else:
         # We don't want to accidentaly run releaseAutomation in tests
         packager_module.createBuild(
             str(srcdir),
             ext_type,
             releaseBuild=True,
             keyFile=keyfile,
+            buildNum=buildnum,
         )
 
 
 def locale_files(languages, rootfolder, srcdir):
     """Generate example locales.
 
     languages: tuple, list or set of languages to include
     rootdir: folder-name to create the locale-folders in
     """
     for lang in languages:
         subfolders = rootfolder.split(os.pathsep) + [lang, 'messages.json']
         json_file = srcdir.ensure(*subfolders)
         if lang == defaultLocale:
             json_file.write(MESSAGES_EN_US)
         else:
             json_file.write('{}')
 
 
 def assert_all_locales_present(package, locales_path):
     names = {x for x in package.namelist() if x.startswith(locales_path)}
     assert names == {
         os.path.join(locales_path, lang, 'messages.json')
         for lang in ALL_LANGUAGES
     }
 
 
 def compare_xml(a, b):
+    """Assert equal content in two manifests in XML format."""
     def get_leafs_string(tree):
-        """Recursively build a string representing all leaf-nodes"""
+        """Recursively build a string representing all xml leaf-nodes."""
         root_str = '{}|{}|{}'.format(tree.tag, tree.tail, tree.text).strip()
         result = []
 
         if len(tree) > 0:
             for subtree in tree:
                 for leaf in get_leafs_string(subtree):
                     result.append('{}__{}'.format(root_str, leaf))
-        else:
-            result.append(root_str)
+        for k, v in tree.attrib.items():
+            result.append('{}__{}:{}'.format(root_str, k, v))
         return result
 
-    tree_a = ElementTree.fromstring(a)
-    tree_b = ElementTree.fromstring(b)
-
-    return set(get_leafs_string(tree_a)) == set(get_leafs_string(tree_b))

[Vasily Kuznetsov, 2017/09/11 12:50:08]

This function and the one below seems to be only used in assertions. It's
probably better to just move the `assert`s into the functions -- this way pytest
will produce more helpful error messages with the diffs between the things we're
comparing.

[tlucas, 2017/09/12 11:32:12]

Acknowledged.

+    # XML data itself shall not be sorted, hence we can safely sort
+    # our string representations in order to produce a valid unified diff.
+    strs_a = sorted(get_leafs_string(ElementTree.fromstring(a)))
+    strs_b = sorted(get_leafs_string(ElementTree.fromstring(b)))
+
+    diff = list(difflib.unified_diff(strs_a, strs_b, n=0))
+    assert len(diff) == 0, '\n'.join(diff)
 
 
 def compare_json(a, b):

[Vasily Kuznetsov, 2017/09/11 12:50:08]

It seems like this function essentially compares two JSON strings ignoring the
order of items in lists. I think it would make sense to give it a docstring that
explains how this comparison differs from just `a == b` (and perhaps why it's
needed).

Also, I wonder if this is actually the best way to do this. What do you think
about traversing the deserialized data converting lists to sets and then just
comparing the results. Something like:

    def lst2set(obj):
        if isinstance(obj, list):
            return {lst2set(item) for item in obj}
        if isinstance(obj, dict):
            return {k, lst2set(v) for k, v in obj.items()}
        return obj

[tlucas, 2017/09/12 11:32:11]

I'm not sure if it would be a good idea to just ignore a list containing equal
items, e.g. [1,2,2,3] - on the other hand that would apply to a possibly desired
order too - what do you think about this?

[Sebastian Noack, 2017/09/12 21:55:26]

I agree, turning lists into sets when comparing the content of manifest.json
will potentially hide bugs. While for some values (e.g. permissions) the order
doesn't matter, for others like list of scripts that are executed in that order
the order matters. Also I cannot think of any scenario for any legit duplicate
in a list in manifest.json.

[tlucas, 2017/09/13 13:43:25]

Agreed - i came up with a different solution, please see in the patch set's
mail.

-    def get_leafs_string(obj):
-        """Recursively build a string representing all leaf-nodes"""
-        result = []
-        if isinstance(obj, list):
-            for item in obj:
-                for repr_str in get_leafs_string(item):
-                    result.append(':,' + repr_str)
-        elif isinstance(obj, dict):
-            for k, v in obj.items():
-                for repr_str in get_leafs_string(v):
-                    result.append('{}:{}'.format(k, repr_str))
-        else:
-            result = [str(obj)]
-
-        return result
-
-    tree_a = json.loads(a)
-    tree_b = json.loads(b)
-    return set(get_leafs_string(tree_a)) == set(get_leafs_string(tree_b))
+    """Assert equal content in two manifests in JSON format.
+
+    Compare the content of two JSON strings, respecting the order of items in
+    a list as well as possible duplicates.
+    Raise a unified diff if equality could not be asserted.
+    """
+    json_a = json.dumps(json.loads(a), sort_keys=True, indent=0).split('\n')
+    json_b = json.dumps(json.loads(b), sort_keys=True, indent=0).split('\n')
+
+    diff = list(difflib.unified_diff(json_a, json_b, n=0))
+    assert len(diff) == 0, '\n'.join(diff)
 
 
 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':
-            assert compare_xml(manifest, fp.read())
+            compare_xml(manifest, fp.read())
         else:
-            assert compare_json(manifest, fp.read())
+            compare_json(manifest, fp.read())