Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/test_packagerChrome.py

Index: tests/test_packagerChrome.py
diff --git a/tests/test_packagerChrome.py b/tests/test_packagerChrome.py
new file mode 100644
index 0000000000000000000000000000000000000000..d5302a547811ceb97a8a627e366b914c5bdcbe5d
--- /dev/null
+++ b/tests/test_packagerChrome.py
@@ -0,0 +1,335 @@
+# 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 re
+import json
+import zipfile
+
+from xml.etree import ElementTree
+from itertools import product
+
+import pytest
+
+from buildtools import packager, packagerChrome
+
+ICON_SIZES = [32, 48, 64, 256, 53]
+
+TR_FA = [True, False]

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

Not sure having this constant is worth it. TR_FA is a bit of a confusing name
and if you call it TRUE_FALSE, then it's kind of pointless to have the constant.

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

This will be gotten rid of by proper parametrization anyway: Acknowledged.

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

Done.

+
+MINIMUM_MANIFEST_JSON = [

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

Isn't this name a bit confusing given that the content of this constant is not
JSON but a list? Maybe just call it MINIMUM_MANIFEST?

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

Acknowledged.

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

Done.

+        ('manifest_version', 2),

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

For consistency with other code, it would be better to indent this list as
follows:

FOO = [
    'bar',
    'baz',
]

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

Acknowledged.

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

Done.

+        ('author', 'Eyeo GmbH'),
+        ('permissions', ['tabs', '<all_urls>', 'contextMenus', 'webRequest',
+                         'webRequestBlocking', 'webNavigation', 'storage',
+                         'unlimitedStorage', 'notifications']),
+        ('minimum_chrome_version', '49.0'),
+        ('minimum_opera_version', '36.0'),
+        ('devtools_page', 'devtools.html'),
+        ('description', '__MSG_description__'),
+        ('default_locale', 'en_US'),
+        ('short_name', '__MSG_name__'),
+        ('storage', {'managed_schema': 'managed-storage-schema.json'}),
+        ('options_ui', {'open_in_tab': True, 'page': 'options.html'}),
+    ]
+
+
+@pytest.fixture
+def icons(tmpdir):
+    """Valid .png files for testing make_icons"""
+    try:

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

Since this runs in tests, we probably know which branch of this try/except will
actually work. Or do we not know it?

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

We actually do, i just added this try/except before adding pillow to the
tox.ini. Acknowledged

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

Done.

+        from PIL import Image
+    except:
+        import Image
+
+    paths = []
+    icon_dir = tmpdir.mkdir('tmp_icons')
+
+    for size in ICON_SIZES:
+        img = Image.new('1', (size, size if size != 53 else size + 1), 0)
+        paths += [str(icon_dir.join('abp-{}.png'.format(size)))]

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

What do you think about rewriting this as follows?

        img_path = icon_dir.join(...).strpath
        img.save(img_path)
        paths.append(img_path)

Seems like it would be a bit more readable.

Also, using `some_pypath.strpath` is preferable to `str(some_pythonpath)` since
it's a more specific code that uses py.path API and it's also consistent with
how we're doing it in most other tests.

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

I agree, this would be a lot more readable.

Regarding the str-comment:
the str(some_pypath) was inspired by the original test_packagerEdge.py content -
which style should i follow here?

Also: Afaik the __str__ method simply returns self.strpath for now and imho the
explicit conversion is more readable - but i wouldn't mind changing that (and
maybe adjust the respective code in test_packagerEdge as well?)

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

Done / as discussed, not changing the str()-thing

+        img.save(paths[-1])
+
+    return paths
+
+
+@pytest.fixture
+def keyfile(tmpdir):
+    """Test-privatekey for signing files"""
+
+    content = """-----BEGIN RSA PRIVATE KEY-----
+MIIEowIBAAKCAQEA0P06DVxOWZ97cvBZWz7DV0DYTFF7Cdrxnu/sCxnHp8IEq+sM
+rqodp2i8eTsddnthTnA93wiFiFXz6/9liQKrSBtogEGG2tmrT73jy5mdOehm7h6m
+Ujy6wLuZQiuY/RWJ6453/0H3bKCA8IlIdU8Pkkn5DJWH+YiiSUD01U5ZMLn8m/+O
+D/r9CLsgWoJV4VCXyzMItcc8hlRbiSe/3DQmZOPbF0Bd1Jv6lNA4RG0U8oA6q5s1
+5w3n//FjNOreVocyxgWWWGk/awlkJme1xn3yB3yh74b51FHsr9vhiTjcs+QNO1BS
+9fA/oavzDOs1Q4fLnTbKn6Jlg5OAjWtiq4vjSQIDAQABAoH/fojTnUNGLP1iwTTE
+5Xoay7l3PL4YwN7PbGvXfuEdAXV3Xp/yDc7yJWpEsyIXtKT/RX6v91oxf1qLVVhN
+Iad8DSyLGRyTie5Aywct5RgdGfKcX5AvI5uhdxAeuvGqr5Fa8ERSYzqNlDeZ2glE
+1cIIq4oeQIBI08zmdXPeyUemuNjK8TEWKOTMOjDLhav/DMviI1cvqp5VIh3RseMr
+1dwprXe0/LWuCh0BrCZZNH/0smd2etXEq06RhgnE2sZkowktNhSgc2pqls92FT88
+ObjI/jRXf6OwK7HxIJ1tMKlk/VCvAI2PNWi6E7IZbqN7Rg7859uWybGZkP/h1Qf5
+fRitAoGBAOJnP3kgavnrUJ482R8MNjOwtqbLf4obS5Y/olOHUi+mu2x+EwxgDtaE
+sXeMjoIAuZvDoAIG0Hn7+J5ZSYkXT9aXQy/odVWDIpFNFFXNFnS73sAWay/FsMJc
+NvgbbWJk9fCq23O7ihuSkfM+zh8dAZoHzV4aw3DsZj37yzuTmTJbAoGBAOxPM6Wy
+fENT8ECGl9d/j8QBV4LvjrfIbz/RcCxgZHG50ELnxCo7FiYRg3d2DjR6Q+EpjclX
+CC+IwCGGf+RBz53WzJPOBkz+plPZzrhhhtpBuWgegeF+TmYT9pPd9XRHJs5qFCak
+J3xxgBur3NDtejNLWBc4bgFfrv8hdYeuIKorAoGBAJplWNbssadvv1G6I0NWG5yS
+lW0X+Akh5iE3kiaucPDIHqa1L55P366RXUku1Hx5rBo6hWL8bK3TlM/ACjLwb+Ti
+0NHaEDJZtHgsfYKp0veWqyiJ4Vz5zzosktwOMEFaopIWooPBUETPZrLgkMaNDGuT
+iIz1aXUX8f2xOf6OAHpjAoGBAMNqq9c2xrQW0fNKWn8HKih4w5mv6WHqCrXHyO+p
+tualNqhdaUdTHXnVudYsdorHISMubeY2ZrqIZ/pRc6mbEsoAO6VvFp7NZ1aoI98u
+J4qOF3kW4WlMPiEEGUEmqDjELj88UfWygkTSx5Iaibzs5cVNZUeujqnsKpcpYDwX
+r0OJAoGBAIWgnx72WjcFmL1kgOu2kH8tH5+cIAb/THX8lnCulQIlxss8n26NMMDa
+Kmh5gC2SOAe92cXG6jCYHZqZtl5gM6xGky1B9j7TDrXVPj+QDcItCuNN4zrdtiah
+US0KNMZKEMjxsaaMDPHj/bP9ULLbMHvtfvt4EnBQoVCDSZdfeBvZ
+-----END RSA PRIVATE KEY-----"""
+
+    keyfile_path = str(tmpdir.mkdir('rsakey').join('keyfile'))
+
+    with open(keyfile_path, 'wb') as key_fp:
+        key_fp.write(content)
+
+    return keyfile_path
+
+
+@pytest.mark.parametrize(

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

What do you think about creating 3 fixtures instead: `gecko_metadata`,
`chrome_metadata` and `edge_metadata`? Then the tests could depend on those
fixtures directly instead of having to copy these `mark.parametrize` and
`mark.usefixtures` everywhere.

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

This would also reduce the amount of readMetadata-calls. Acknowledged.

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

Done.

+    'metadata_files',
+    [['metadata.chrome', 'metadata.gecko-webext']],
+    indirect=True)
+@pytest.mark.usefixtures('metadata_files')
+def test_create_manifest(srcdir, files):
+    for release, devenv, ext_type in product(
+            TR_FA, TR_FA, ['chrome', 'gecko-webext']):
+
+        metadata = packagerChrome.readMetadata(str(srcdir), ext_type)
+
+        version = packager.getBuildVersion(str(srcdir), metadata, release)
+        params = {
+            'type': ext_type,
+            'baseDir': str(srcdir),
+            'releaseBuild': release,
+            'version': version,
+            'devenv': devenv,
+            'metadata': metadata,
+        }
+        manifest = json.loads(packagerChrome.createManifest(params, files))
+
+        expected_values = MINIMUM_MANIFEST_JSON + [
+            ('name', '__MSG_name_' + ('devbuild__' if not release else '_')),
+            ('version', '1.2.3' + ('.0' if not release else '')),
+        ]
+
+        if ext_type == 'gecko-webext':
+            expected_values += [
+                ('applications', {
+                    'gecko': {
+                        'id': '{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}',
+                        'strict_min_version': '50.0',
+                        }
+                    })
+            ]
+            if not release:
+                expected_values[-1][1]['gecko'].update({
+                        'update_url': ''.join((
+                            'https://downloads.adblockplus.org/devbuilds/',
+                            'adblockplusfirefox/updates.json'))
+                    })
+
+        for key, value in expected_values:
+            assert manifest.get(key, None) == value
+
+
+def test_sign_binary(files, keyfile):
+    from Crypto.Hash import SHA
+    from Crypto.PublicKey import RSA
+    from Crypto.Signature import PKCS1_v1_5
+
+    digest = SHA.new()
+
+    signature = packagerChrome.signBinary(files.zipToString(), keyfile)
+
+    with open(keyfile, 'r') as fp:
+        rsa_key = RSA.importKey(fp.read())
+
+    signer = PKCS1_v1_5.new(rsa_key)
+
+    digest.update(files.zipToString())
+    assert signer.verify(digest, signature)
+
+
+def test_get_public_key(keyfile):
+    expected = ''.join((

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

You can actually write this as follows:

    expected = (
        '3495802348590234850934850234509'
        '4909324063490682390462820934680'
        ...
    )

When a bunch of string literals are written one after another like this, Python
will just concatenate them into one big constant.

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

Acknowledged.

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

Done.

+        '30820122300d06092a864886f70d01010105000382010f003082010a0282010100d0',
+        'fd3a0d5c4e599f7b72f0595b3ec35740d84c517b09daf19eefec0b19c7a7c204abeb',
+        '0caeaa1da768bc793b1d767b614e703ddf08858855f3ebff658902ab481b68804186',
+        'dad9ab4fbde3cb999d39e866ee1ea6523cbac0bb99422b98fd1589eb8e77ff41f76c',
+        'a080f08948754f0f9249f90c9587f988a24940f4d54e5930b9fc9bff8e0ffafd08bb',
+        '205a8255e15097cb3308b5c73c86545b8927bfdc342664e3db17405dd49bfa94d038',
+        '446d14f2803aab9b35e70de7fff16334eade568732c6059658693f6b09642667b5c6',
+        '7df2077ca1ef86f9d451ecafdbe18938dcb3e40d3b5052f5f03fa1abf30ceb354387',
+        'cb9d36ca9fa2658393808d6b62ab8be3490203010001'))
+
+    publickey = packagerChrome.getPublicKey(keyfile)
+    assert publickey.encode('hex') == expected
+
+
+@pytest.mark.parametrize('metadata_files', ['metadata.chrome'], indirect=True)
+@pytest.mark.usefixtures('metadata_files')
+def test_get_package_files(srcdir):
+    extensions = ['html', 'js', 'json', 'xml']

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

What do you think about replacing this with:

    files = ['foo.html', 'foo.js', 'foo.json', 'foo.xml']

Then you wouldn't need to prepend 'foo.' to each extension twice in the
following code and it would become simpler and more readable.

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

Acknowledged.

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

Done.

+    for extension in extensions:
+        srcdir.join(''.join(('foo.', extension))).write('', ensure=True)
+
+    for devenv in [True, False]:

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

What do you think about making devenv a parameter to the test:

@pytest.mark.parametrize('devenv', [True, False])
def test_get_package_files(srcdir, devenv):
    ...

This would convert this test into two test items that might pass or fail
independently thus producing better granularity of the reporting. And it seems
like semantically this `devenv` thing is a parameter to the test, so it's a bit
cleaner if it is explicitly declared as such.

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

Acknowledged.

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

Done.

+        params = {
+            'baseDir': str(srcdir),
+            'devenv': devenv,
+        }
+
+        files = packagerChrome.getPackageFiles(params)
+
+        assert devenv == ('qunit' in files)

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

This is clever and compact, but I'm wondering if it would be more readable to
write:

    if devenv:
        assert 'qunit' in files
    else:
        assert 'qunit' not in files

This seems to express intent better. I don't feel very strong about this one, so
if you don't like using 4 lines instead of one, may I suggest changing the order
in your version:

    assert ('qunit' in files) == devenv

This reads a bit better I think.

P.S. Another one-liner version, that seems a bit more readable to me is this:

    assert 'qunit' in files if devenv else 'qunit' not in files

I'm happy to let you choose which one you prefer ;)

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

In this case i'll choose your reordered approach - it follows the somehow more
natural-seeming order of "primary-thing-to-check vs thing-to-check-against" 

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

Done.

+
+        for expected in ['icons', 'ui', 'skin', 'ext', '_locales', 'lib',
+                         'jquery-ui'] + ['foo.' + ext for ext in extensions]:
+            assert expected in files
+
+
+def test_make_icons(icons, capsys):
+    files = packager.Files(set(), set())
+    for path in icons:
+        files[path] = open(path, 'r').read()
+    result = packagerChrome.makeIcons(files, icons)
+
+    out, err = capsys.readouterr()
+
+    assert 'should be square' in err
+    assert not any(size not in result for size in ICON_SIZES)

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

Is this the same as `assert all(size in result for size in ICON_SIZES)`?

Also, what do you think about `assert set(result) >= set(ICON_SIZES)`?

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

The result would be the same - but all(..) is more readable, yes - and get's rid
of 2 nots. Changing it.


Comparing the sets would be even more readable, but why greater-than-equal? From
what i see / thought of, we should expect the exact same values on both sides

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

Done.

+
+
+@pytest.mark.parametrize('metadata_files', ['metadata.chrome'], indirect=True)
+@pytest.mark.usefixtures('metadata_files')
+def test_create_script_page(srcdir):
+    metadata = packagerChrome.readMetadata(str(srcdir), 'chrome')
+    params = {'metadata': metadata}
+
+    template = packagerChrome.createScriptPage(

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

Could you please reformat this as follows:

    template = packagerChrome.createScriptPage(
        params, ...
    )

I know this is not mentioned in the style guide but we've been trying to stick
to this style as it's considered more readable by some and equally readable by
others. The rule will probably make it to the style guide at some point :D

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

Acknowledged.

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

Done.

+        params, 'testIndex.html.tmpl', ('general', 'testScripts'))
+
+    template_scripts = [elem.attrib['src'] for elem in

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

Not that it matters for performance, but to improve clarity, since we're only
doing `foo in template_scripts` checks on this, maybe it should be a set
comprehension?

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

Acknowledged.

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

Done.

+                        ElementTree.fromstring(template).iter('script')]
+
+    for src in metadata.get('general', 'testScripts').split():
+        assert src in template_scripts
+
+
+@pytest.mark.parametrize('metadata_files', ['metadata.chrome'], indirect=True)
+@pytest.mark.usefixtures('metadata_files')
+def test_convert_js(srcdir, files):
+    metadata = packagerChrome.readMetadata(str(srcdir), 'chrome')
+    params = {
+        'type': 'chrome',
+        'metadata': metadata,
+    }
+
+    packagerChrome.convertJS(params, files)
+    source = files['lib/foo.js']
+
+    assert 'var foo;' in source
+    assert 'var bar;' in source
+
+
+def test_to_json():
+    data = {'bar': ['foo', 'foobar', 1, True]}
+    data_json = packagerChrome.toJson(data)
+
+    assert data_json[-1] == '\n'

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

Maybe `data_json.endswith('\n')`?

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

Acknowledged.

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

Done.

+    json.loads(data_json)
+
+
+@pytest.mark.parametrize('metadata_files', ['metadata.chrome'], indirect=True)
+@pytest.mark.usefixtures('metadata_files')
+def test_import_gecko_locales(tmp_dir, srcdir, files):
+    metadata = packagerChrome.readMetadata(str(srcdir), 'chrome')
+    version = packager.getBuildVersion(str(srcdir), metadata, False)

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

Here and in other places: it's better to do it this way: `srcdir.strpath`.

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

Please see comment above

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

As discussed

+    params = {
+        'type': 'chrome',
+        'baseDir': str(srcdir),
+        'releaseBuild': False,
+        'version': version,
+        'devenv': False,
+        'metadata': metadata,
+    }
+
+    data = {'name': 'foo', 'name_devbuild': 'bar'}
+
+    t_dir = tmp_dir.mkdir('_trans').mkdir('en-US')
+    t_dir.join('test.properties').write(
+        '\n'.join(
+            '='.join((k, v)) for k, v in data.iteritems()))

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

What do you think about

    '{}={}'.format(k, v)

instead? Seems a bit more clear to me. Also `.iteritems()` is a performance
optimization that's not compatible with Python 3, so it's probably better to use
`.items()` here.

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

Acknowledged.

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

Done.

+
+    packagerChrome.importGeckoLocales(params, files)
+    trans_data = json.loads(files['_locales/en_US/messages.json'])
+
+    for key, value in data.iteritems():
+        assert trans_data.get('test_{}'.format(key), {})\

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

Flake8 should have told you that `'test_' + key` is better in this case. This
will also make the whole assert statement fit in 80 lines and you won't need to
split the line with "\" -- a practice that PEP8 frowns upon.

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

Acknowledged, although flake8 / tox did not inform / complain about this. Will
look into it.

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

Done. No idea why flake8 didn't bother though

+               .get('message', '') == value
+
+
+def test_fix_translations_for_cws(files):
+    files['_locales/de/messages.json'] = packagerChrome.toJson({})
+    files['manifest.json'] = packagerChrome.toJson(
+        {k: v for k, v in MINIMUM_MANIFEST_JSON})

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

You can just use `dict(MINIMUM_MANIFEST_JUST)` here for the same effect.

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

Acknowledged.

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

Done.

+    packagerChrome.fixTranslationsForCWS(files)
+
+    ger_messages = json.loads(files['_locales/de/messages.json'])
+    eng_messages = json.loads(files['_locales/en_US/messages.json'])
+    for match in re.finditer(r'__MSG_(\S+)__', files['manifest.json']):
+        name = match.group(1)
+        assert ger_messages[name]['message'] == eng_messages[name]['message']
+
+    assert len(ger_messages['description']['message']) <= 132

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

This is a curious assertion. What exactly are we checking here? Maybe a comment
would be useful?

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

ChromeWebStore enforces a max-length to be applied on translated strings used in
names / descriptions - but i agree on the comment-comment

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

Done.

+    assert len(ger_messages['name']['message']) <= 12
+
+
+@pytest.mark.parametrize(
+    'metadata_files',
+    [['metadata.chrome', 'metadata.gecko-webext']],
+    indirect=True)
+@pytest.mark.usefixtures('metadata_files', 'files')
+def test_create_build(srcdir, keyfile, tmp_dir):
+
+    basefiles = [
+        'manifest.json',
+        'lib/foo.js',
+        'foo/logo_50.png',
+        '_locales/en_US/messages.json',
+        '_locales/en-US/test.properties',
+        'logo_44.png',
+        'qunit/index.html',
+        'icons/logo_150.png',
+    ]
+
+    for release, devenv, ext_type, key in product(

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

Perhaps it would be better to make this a parametrization as well. Seems a bit
cleaner and we would see individual passes / failures for each parameter
combination in the report, which might be more useful. What do you think?

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

Totally agreed!

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

Done.

+            TR_FA, TR_FA, ['chrome', 'gecko-webext'], [None, keyfile]):
+
+        out_file = str(tmp_dir.join('out'))
+
+        packagerChrome.createBuild(
+            outFile=out_file,
+            baseDir=str(srcdir),
+            type=ext_type,
+            releaseBuild=release,
+            keyFile=key,
+            devenv=devenv)
+
+        with zipfile.ZipFile(out_file, 'r') as zipfp:
+            zipfp.testzip()
+
+            filenames = [zipinfo.filename for zipinfo in zipfp.infolist()]
+            for filename in basefiles if not devenv else basefiles + [
+                        'devenvPoller__.js',
+                        'devenvVersion__',
+                    ]:
+                assert filename in filenames