Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/test_packagerGecko.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 io
 
 import pytest
 
 import json
 
 from zipfile import ZipFile
 
 from xml.etree import ElementTree
-from itertools import product
 
 from buildtools import packagerGecko
 from buildtools import localeTools
 
-from buildtools.packager import readMetadata, getBuildVersion, Files
-from functools import reduce
-
-TR_FA = [True, False]
+from buildtools.packager import getBuildVersion, Files, readMetadata
+from buildtools.tests.tools import copy_metadata
 
 MESSAGES = '\n'.join((
     'name=Name {0}',
     'description=Awesome description {0}',
 ))
 
 
 @pytest.fixture
-def scripts(tmp_dir):
-    """Examplary scripts for testing addMissingFiles"""

[Vasily Kuznetsov, 2017/08/03 16:52:32]

AFAIK "examplary" nowadays is usually spelled "exemplary" and it's usually used
to mean something like "outstanding". I'd just say "Example scripts ..." but
perhaps you can also check with Tom ;)

[tlucas, 2017/08/03 21:26:02]

Your are right - but i like the "outstanding" part - will change it though ;)

[tlucas, 2017/08/04 14:52:00]

Done.

-    lib_dir = tmp_dir.mkdir('lib')
+def gecko_metadata(tmpdir):
+    filename = 'metadata.gecko'
+    copy_metadata(filename, tmpdir)
+
+    return readMetadata(str(tmpdir), 'gecko')
+
+
+@pytest.fixture
+def scripts(tmpdir):
+    """Example scripts for testing addMissingFiles"""
+    lib_dir = tmpdir.mkdir('lib')
     lib_dir.join('ext.js').write('require("hooks");')
 
-    content_dir = tmp_dir.mkdir('chrome').mkdir('content')
+    content_dir = tmpdir.mkdir('chrome').mkdir('content')
     content_dir.join('common.js').write('require("hooks");')
 
 
 @pytest.fixture
-def prefs_json(tmp_dir):
+def prefs_json(tmpdir):
     """Minimal .json file for testing processJSONFiles"""
-    lib_dir = tmp_dir.mkdir('lib')
+    lib_dir = tmpdir.mkdir('lib')
     lib_dir.join('prefs.json').write(json.dumps(
         {'foo': 'bar'}
     ))
 
 
 @pytest.fixture
-def locales(tmp_dir):
+def locales(tmpdir):
     """Minimal locales for testing locale-processing"""
-    chrome_dir = tmp_dir.mkdir('chrome')
+    chrome_dir = tmpdir.mkdir('chrome')
     locale_dir = chrome_dir.mkdir('locale')
 
     data = {
         'name': {'message': 'Name translated'},
-        'description': {'message': 'Description translated'}

[Vasily Kuznetsov, 2017/08/03 16:52:32]

When you use multiline layout for lists, sets and dicts, it's better to add a
comma to the last line as well. This allows adding more items later without
changing unrelated lines.

[tlucas, 2017/08/03 21:26:03]

As you can see in other collection-defintions, i normally do this - typo, fixing
it :)

[tlucas, 2017/08/04 14:51:59]

Done.

+        'description': {'message': 'Description translated'},
     }
 
     for locale in ['en-US', 'de', 'kn']:
         new_dir = locale_dir.mkdir(locale)
         new_dir.join('meta.properties').write(MESSAGES.format(locale))
         new_dir.join('test.json').write(json.dumps(data))
         if locale == 'kn':
             new_dir.join('.incomplete').write('')
 
 
 @pytest.fixture
-def subscriptions(tmp_dir):
+def subscriptions(tmpdir):
     """Examplary sbuscription-configuration"""
-    tmp_dir.join('subs.xml').write('\n'.join((

[Vasily Kuznetsov, 2017/08/03 16:52:32]

Wouldn't it be easier to just use a string literal here? If you don't like that,
it would be better to make the argument to `join` a list rather than a tuple.

[tlucas, 2017/08/03 21:26:03]

Acknowledged.

[tlucas, 2017/08/04 14:52:00]

Done.

-        '<subscriptions>',
-        '<subscription title="EasyList"',
-        'specialization="English"',
-        'url="https://easylist-downloads.adblockplus.org/easylist.txt"',
-        'homepage="https://easylist.adblockplus.org/"',
-        'prefixes="en"',
-        'author="fanboy, MonztA, Famlam, Khrin"',
-        'type="ads"/>',
-        '</subscriptions>',
-    )))
+    tmpdir.join('subs.xml').write(
+        '<subscriptions>'
+        '<subscription title="EasyList"'
+        ' specialization="English"'
+        ' url="https://easylist-downloads.adblockplus.org/easylist.txt"'
+        ' homepage="https://easylist.adblockplus.org/"'
+        ' prefixes="en"'
+        ' author="fanboy, MonztA, Famlam, Khrin"'
+        ' type="ads"/>'
+        '</subscriptions>'
+    )
 
 
 def test_package_files(tmpdir):
     tmpdir.join('foo.xml').write('')
     tmpdir.join('foo.txt').write('')
     tmpdir.join('foo.js').write('')
 
     params = {
         'baseDir': str(tmpdir)
     }
 
     files = packagerGecko.getPackageFiles(params)
     assert 'foo.xml' in files
     assert 'foo.js' in files
     assert 'foo.txt' not in files
 
 
 @pytest.mark.usefixtures('locales')
-def test_get_locales(tmp_dir):
-    for incomplete in [True, False]:

[Vasily Kuznetsov, 2017/08/03 16:52:32]

This could also be done via parametrize perhaps.

[tlucas, 2017/08/03 21:26:02]

Acknowledged.

[tlucas, 2017/08/04 14:52:00]

Done.

-        locales = packagerGecko.getLocales(str(tmp_dir), incomplete)
-
-        assert 'de' in locales
-        assert 'en-US' in locales
-        assert ('kn' in locales) == incomplete
-
-
-@pytest.mark.parametrize('metadata_files', ['metadata.gecko'], indirect=True)
-@pytest.mark.usefixtures('locales', 'metadata_files', 'subscriptions')
-def test_create_manifest(tmp_dir):

[Vasily Kuznetsov, 2017/08/03 16:52:32]

All this code is pretty cool and clever, but it makes me wonder if there could
be a simpler way to check the manifest. For example, could we just put an
expected result into a file and then just load both (output and expected result)
with ElementTree and verify that they are identical except for some details that
we know will be different?

[tlucas, 2017/08/03 21:26:02]

The cool- and cleverness could be a result of following your original-approach
;)
But yes, that would minimize the initialisation-boilerplate to a minimum. I will
adhere to this approach in test_packagerEdge.py as well!

[tlucas, 2017/08/04 14:51:59]

Done.

[Vasily Kuznetsov, 2017/08/10 19:48:27]

Acknowledged.

-    def first(elem):
-        return elem[0]
-
-    def text(elem):
-        return elem.text
-
-    def iteritems(func=None):
-        def wrapper(elements):
-            for elem in elements:
-                if func:
-                    yield func(elem)
-                else:
-                    yield elem
-        return wrapper
-
-    metadata = readMetadata(str(tmp_dir), 'gecko')
-    locales = packagerGecko.getLocales(str(tmp_dir))
-    contributors = packagerGecko.getContributors(metadata)
-
-    namespaces = {
-        'em': 'http://www.mozilla.org/2004/em-rdf#',
-        'ns': 'http://www.w3.org/1999/02/22-rdf-syntax-ns#',
-    }
-
-    base = [
-        ('.//*', [len], 54),
-        ('./ns:Description/em:id', [first, text],
-            '{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}'),
-        ('./ns:Description/em:optionsURL', [first, text],
-            'chrome://adblockplus/content/ui/settings.xul'),
-        ('./ns:Description/em:optionsType', [first, text], '2'),
-        ('./ns:Description/em:bootstrap', [first, text], 'true'),
-        ('./ns:Description/em:multiprocessCompatible', [first, text], 'true'),
-        ('./ns:Description/em:homepageURL', [first, text],
-            'http://adblockplus.org/'),
-        ('./ns:Description/em:creator', [first, text], 'Wladimir Palant'),
-        ('./ns:Description/em:contributor', [iteritems(text)],
-            ['Pety Pete', 'Neil Armstrong', 'Famlam', 'fanboy', 'Khrin',
-             'MonztA']),
-    ]
-
-    base += [
-        ('./ns:Description/em:localized/ns:Description[em:locale="{}"]/em:{}'
-            .format(locale, tag),
-            [first, text],
-            value.format(locale))
-        for locale in locales
-        for tag, value in [
-            ('name', 'Name {}'),
-            ('description', 'Awesome description {}')
-        ]
-    ]
-
-    tags = ['minVersion', 'maxVersion']
-    apps = metadata.items('compat')
-    comp = [
-        (
-            packagerGecko.KNOWN_APPS.get(app[0]),
-            tags[i],
-            app[1].split('/')[i]
-        ) for app in apps for i in range(2)
-    ]
-
-    base += [
-        (''.join((
-                './ns:Description/em:targetApplication/',
-                'ns:Description[em:id="{}"]/em:{}'
-            ))
-            .format(mapped_id, tag),
-            [first, text],
-            value)
-        for mapped_id, tag, value in comp
-    ]
-
-    for release, multicompartment in product(TR_FA, TR_FA):

[Vasily Kuznetsov, 2017/08/03 16:52:32]

Maybe better do this with parametrization?

[tlucas, 2017/08/03 21:26:02]

Acknowledged.

[tlucas, 2017/08/04 14:52:00]

Done.

-        version = getBuildVersion(str(tmp_dir), metadata, release, None)
-        expected = base + [
-            ('./ns:Description/em:version', [first, text], version),
-        ]
-        params = {
-            'baseDir': str(tmp_dir),
-            'locales': locales,
-            'metadata': metadata,
-            'version': version.encode('utf-8'),
-            'multicompartment': multicompartment,
-            'contributors': contributors
-        }
-        manifest = packagerGecko.createManifest(params)
-
-        tree = ElementTree.fromstring(manifest)
-
-        with open('/tmp/test.xml', 'w') as fp:

[Vasily Kuznetsov, 2017/08/03 16:52:32]

Why use '/tmp' instead of tmpdir fixture? Also keep in mind that you can't
really depend on '/tmp' always being there and you having write access to it.

[tlucas, 2017/08/03 21:26:02]

Acknowledged.

[tlucas, 2017/08/04 14:52:00]

Done (removed).

-            fp.write(manifest)
-
-        for expression, modifiers, value in expected:
-            res = reduce(
-                lambda val, func: func(val),
-                modifiers,
-                tree.findall(expression, namespaces=namespaces))
-
-            from collections import Iterable
-
-            if isinstance(res, Iterable) and not isinstance(res, str):
-                res = list(res)
-                for x in res:

[Vasily Kuznetsov, 2017/08/03 16:52:32]

Seems like you just want to compare `res` to `value` as sets here:

    assert set(res) == set(value)

However, this also seems redundant given that the assert below (line 219) always
runs.

[tlucas, 2017/08/03 21:26:02]

I agree with the assert set() == set() part, but only relying on assert 'res ==
value' from line 219 would require those creating the data to keep all potential
elements in the exact same order (which is not guaranteed while using etree
iirc) - or am i missing something?

[tlucas, 2017/08/04 14:52:00]

I totally missed something. Done.

-                    assert x in value
-                for x in value:
-                    assert x in res
-            assert res == value
-
-
-@pytest.mark.parametrize('metadata_files', ['metadata.gecko'], indirect=True)
-@pytest.mark.usefixtures('locales', 'metadata_files')
-def test_fixup_import_locales(files, tmp_dir):
-    locale_dir = tmp_dir.dirpath(tmp_dir.basename, 'chrome', 'locale')
+@pytest.mark.parametrize('incomplete', [True, False])
+def test_get_locales(tmpdir, incomplete):
+    locales = packagerGecko.getLocales(str(tmpdir), incomplete)
+
+    assert 'de' in locales
+    assert 'en-US' in locales
+    assert ('kn' in locales) == incomplete
+
+
+@pytest.mark.usefixtures('locales', 'subscriptions')
+@pytest.mark.parametrize('release', [True, False])
+@pytest.mark.parametrize('multicompartment', [True, False])
+def test_create_manifest(tmpdir, release, multicompartment, gecko_metadata):
+    locales = packagerGecko.getLocales(str(tmpdir))
+    contributors = packagerGecko.getContributors(gecko_metadata)
+    version = getBuildVersion(str(tmpdir), gecko_metadata, release, None)
+
+    rdfpath = os.path.join(
+        os.path.dirname(__file__),
+        'expecteddata',
+        'manifest_gecko_{}_{}.rdf'.format(release, multicompartment)

[Vasily Kuznetsov, 2017/08/10 19:48:28]

Nit: comma at the end of the line.

[tlucas, 2017/08/11 12:17:09]

Done.

+    )
+
+    with io.open(rdfpath, 'r') as fp:
+        expected = ElementTree.fromstring(fp.read())
+
+    params = {
+        'baseDir': str(tmpdir),
+        'locales': locales,
+        'metadata': gecko_metadata,
+        'version': version.encode('utf-8'),
+        'multicompartment': multicompartment,
+        'contributors': contributors

[Vasily Kuznetsov, 2017/08/10 19:48:28]

Nit: comma at the end of the line.

[tlucas, 2017/08/11 12:17:09]

Done.

+    }
+    manifest = packagerGecko.createManifest(params)
+
+    tree = ElementTree.fromstring(manifest)
+
+    from buildtools.tests.tools import get_leafs_string
+    assert set(get_leafs_string(tree)) == set(get_leafs_string(expected))
+
+
+@pytest.mark.usefixtures('locales')
+def test_fixup_import_locales(files, tmpdir, gecko_metadata):
+    locale_dir = tmpdir.dirpath(tmpdir.basename, 'chrome', 'locale')
     locale_dir.mkdir('fr').join('meta.properties')\

[Vasily Kuznetsov, 2017/08/10 19:48:28]

You don't need to break this line, it actually fits into one line.

[tlucas, 2017/08/11 12:17:09]

Done.

         .write(MESSAGES.format('fr'))
 
-    metadata = readMetadata(str(tmp_dir), 'gecko')
-    locales = packagerGecko.getLocales(str(tmp_dir), False)
-
-    params = {
-        'metadata': metadata,
+    locales = packagerGecko.getLocales(str(tmpdir), False)
+
+    params = {
+        'metadata': gecko_metadata,
         'locales': locales,
-        'baseDir': str(tmp_dir)
+        'baseDir': str(tmpdir)
     }
 
     # Should add missing fr/test.properties to files
     packagerGecko.fixupLocales(params, files)
 
     packagerGecko.importLocales(params, files)
     for locale in locales:
         properties = files['chrome/locale/{}/test.properties'.format(locale)]
         translation_data = list(
             localeTools.parsePropertiesString(properties, ''))
 
         for trans in [
                 ('name', None, 'Name translated'),
                 ('description', None, 'Description translated')]:
             assert trans in translation_data
 
 
-def test_process_json_files(tmp_dir, prefs_json):
-    params = {
-        'baseDir': str(tmp_dir),
+def test_process_json_files(tmpdir, prefs_json):
+    params = {
+        'baseDir': str(tmpdir),
         'jsonRequires': {},
     }
 
     files = Files(packagerGecko.getPackageFiles(params), set())
-    files.read(str(tmp_dir))
+    files.read(str(tmpdir))
 
     packagerGecko.processJSONFiles(params, files)
 
     assert params['jsonRequires'] == {'prefs.json': {'foo': 'bar'}}
 
 
-@pytest.mark.parametrize('metadata_files', ['metadata.gecko'], indirect=True)
-@pytest.mark.usefixtures('scripts', 'metadata_files')
-def test_add_missing_files(tmp_dir):
-    metadata = readMetadata(str(tmp_dir), 'gecko')
-
-    params = {
-        'baseDir': str(tmp_dir),
-        'metadata': metadata,
+@pytest.mark.usefixtures('scripts')
+def test_add_missing_files(tmpdir, gecko_metadata):
+    params = {
+        'baseDir': str(tmpdir),
+        'metadata': gecko_metadata,
         'jsonRequires': {},
         'multicompartment': True,
         'hasWebExtension': False,
     }
 
     files = Files(packagerGecko.getPackageFiles(params), set())
-    files.read(str(tmp_dir))
+    files.read(str(tmpdir))
 
     packagerGecko.addMissingFiles(params, files)
 
     assert 'let shutdownHandlers = [];' in files['bootstrap.js']
     assert 'Services.obs.addObserver(' in files['bootstrap.js']
     for filename in ['bootstrap.js', 'chrome/content/common.js',
                      'lib/ext.js', 'lib/hooks.js']:
         assert filename in files
 
 
-@pytest.mark.parametrize('metadata_files', ['metadata.gecko'], indirect=True)
-@pytest.mark.usefixtures('metadata_files', 'locales', 'subscriptions')
-def test_create_build(tmp_dir, capsys):
+@pytest.mark.usefixtures('gecko_metadata', 'locales', 'subscriptions')
+@pytest.mark.parametrize('release', [True, False])
+@pytest.mark.parametrize('multicompartment', [True, False])
+@pytest.mark.parametrize('all_locales', ['all', None])
+def test_create_build(tmpdir, capsys, release, multicompartment, all_locales):
     base_files = [
         'bootstrap.js',
         'chrome/locale/de/test.json',
         'chrome/locale/de/test.properties',
         'chrome/locale/en-US/test.json',
         'chrome/locale/en-US/test.properties',
         'install.rdf',
         'subs.xml'
     ]
 
-    for all_locales, release, multicompartment in product(

[Vasily Kuznetsov, 2017/08/03 16:52:32]

Also maybe parametrization?

[tlucas, 2017/08/03 21:26:03]

Acknowledged.

[tlucas, 2017/08/04 14:51:59]

Done.

-            ['all', None], TR_FA, TR_FA):
-
-        if all_locales is None:
-            expected = base_files
-        else:
-            expected = base_files + [
-                'chrome/locale/kn/test.json',
-                'chrome/locale/kn/test.properties'
-            ]
-
-        out_file = tmp_dir.join('{}_{}_{}.zip'.format(
-            all_locales, release, multicompartment))
-
-        out_file = str(out_file)
-
-        packagerGecko.createBuild(
-            str(tmp_dir),
-            locales=all_locales,
-            outFile=out_file,
-            releaseBuild=release,
-            multicompartment=multicompartment)
-
-        out, err = capsys.readouterr()
-
-        assert err ==\
-            "Warning: Mapped file adblockplusui/firstRun.html doesn't exist\n"
-
-        with ZipFile(out_file, 'r') as zipfp:
-            zipfp.testzip()
-
-            filenames = [zipinfo.filename for zipinfo in zipfp.infolist()]
-
-            for name in expected:
-                assert name in filenames
+    if all_locales is None:
+        expected = base_files
+    else:
+        expected = base_files + [
+            'chrome/locale/kn/test.json',
+            'chrome/locale/kn/test.properties'
+        ]
+
+    out_file = tmpdir.join('{}_{}_{}.zip'.format(
+        all_locales, release, multicompartment))
+
+    out_file = str(out_file)
+
+    packagerGecko.createBuild(
+        str(tmpdir),
+        locales=all_locales,
+        outFile=out_file,
+        releaseBuild=release,
+        multicompartment=multicompartment)
+
+    out, err = capsys.readouterr()
+
+    assert err ==\
+        "Warning: Mapped file adblockplusui/firstRun.html doesn't exist\n"
+
+    with ZipFile(out_file, 'r') as zipfp:
+        zipfp.testzip()
+
+        filenames = [zipinfo.filename for zipinfo in zipfp.infolist()]
+
+        for name in expected:
+            assert name in filenames