Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/test_packagerGecko.py

Index: tests/test_packagerGecko.py
diff --git a/tests/test_packagerGecko.py b/tests/test_packagerGecko.py
new file mode 100644
index 0000000000000000000000000000000000000000..f7df16734762a8b63a7cf3a2d2f9f408adc4faef
--- /dev/null
+++ b/tests/test_packagerGecko.py
@@ -0,0 +1,339 @@
+# 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 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]
+
+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')
+    lib_dir.join('ext.js').write('require("hooks");')
+
+    content_dir = tmp_dir.mkdir('chrome').mkdir('content')
+    content_dir.join('common.js').write('require("hooks");')
+
+
+@pytest.fixture
+def prefs_json(tmp_dir):
+    """Minimal .json file for testing processJSONFiles"""
+    lib_dir = tmp_dir.mkdir('lib')
+    lib_dir.join('prefs.json').write(json.dumps(
+        {'foo': 'bar'}
+    ))
+
+
+@pytest.fixture
+def locales(tmp_dir):
+    """Minimal locales for testing locale-processing"""
+    chrome_dir = tmp_dir.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.

+    }
+
+    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):
+    """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>',
+    )))
+
+
+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')
+    locale_dir.mkdir('fr').join('meta.properties')\
+        .write(MESSAGES.format('fr'))
+
+    metadata = readMetadata(str(tmp_dir), 'gecko')
+    locales = packagerGecko.getLocales(str(tmp_dir), False)
+
+    params = {
+        'metadata': metadata,
+        'locales': locales,
+        'baseDir': str(tmp_dir)
+    }
+
+    # 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),
+        'jsonRequires': {},
+    }
+
+    files = Files(packagerGecko.getPackageFiles(params), set())
+    files.read(str(tmp_dir))
+
+    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,
+        'jsonRequires': {},
+        'multicompartment': True,
+        'hasWebExtension': False,
+    }
+
+    files = Files(packagerGecko.getPackageFiles(params), set())
+    files.read(str(tmp_dir))
+
+    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):
+    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