Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/test_packagerWebExt.py

Index: tests/test_packagerWebExt.py
diff --git a/tests/test_packagerWebExt.py b/tests/test_packagerWebExt.py
new file mode 100644
index 0000000000000000000000000000000000000000..07462565005f8770b1190032ea7bbbba5ea07f9c
--- /dev/null
+++ b/tests/test_packagerWebExt.py
@@ -0,0 +1,261 @@
+# 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 json
+import re
+from struct import unpack
+
+import pytest
+from Crypto.Hash import SHA
+from Crypto.PublicKey import RSA
+from Crypto.Signature import PKCS1_v1_5
+
+
+from buildtools import packager
+from buildtools.tests.tools import (DirContent, ZipContent, copy_metadata,
+                                    run_webext_build, assert_manifest_content,
+                                    assert_all_locales_present, locale_files)
+from buildtools.tests.conftest import ALL_LANGUAGES
+
+
+LOCALES_MODULE = {
+    'test.Foobar': {
+        'message': 'Ensuring dict-copy from modules for $domain$',
+        'description': 'test description',
+        'placeholders': {'content': '$1', 'example': 'www.adblockplus.org'}
+    }
+}
+
+DTD_TEST = ('<!ENTITY access.key "access key(&amp;a)">'
+            '<!ENTITY ampersand "foo &amp;-bar">')
+
+PROPERTIES_TEST = 'description=very descriptive!'
+
+
+@pytest.fixture
+def gecko_import(tmpdir):
+    tmpdir.mkdir('_imp').mkdir('en-US').join('gecko.dtd').write(DTD_TEST)
+
+
+@pytest.fixture
+def locale_modules(tmpdir):
+    mod_dir = tmpdir.mkdir('_modules')
+    lang_dir = mod_dir.mkdir('en-US')
+    lang_dir.join('module.json').write(json.dumps(LOCALES_MODULE))
+    lang_dir.join('unit.properties').write(json.dumps(PROPERTIES_TEST))
+
+
+@pytest.fixture
+def icons(srcdir):
+    icons_dir = srcdir.mkdir('icons')
+    for filename in ['abp-16.png', 'abp-19.png', 'abp-53.png']:
+        shutil.copy(
+            os.path.join(os.path.dirname(__file__), filename),
+            os.path.join(str(icons_dir), filename),
+        )
+
+
+@pytest.fixture
+def all_lang_locales(tmpdir):
+    return locale_files(ALL_LANGUAGES, '_locales', tmpdir)
+
+
+@pytest.fixture
+def chrome_metadata(tmpdir):
+    filename = 'metadata.chrome'
+    copy_metadata(filename, tmpdir)
+
+
+@pytest.fixture
+def gecko_webext_metadata(tmpdir, chrome_metadata):
+    filename = 'metadata.gecko-webext'
+    copy_metadata(filename, tmpdir)
+
+
+@pytest.fixture
+def keyfile(tmpdir):
+    """Test-privatekey for signing chrome release-package"""
+    return os.path.join(os.path.dirname(__file__), 'chrome_rsa.pem')
+
+
+@pytest.fixture
+def lib_files(tmpdir):
+    files = packager.Files(['lib'], set())
+    files['ext/a.js'] = 'var bar;'
+    files['lib/b.js'] = 'var foo;'
+
+    tmpdir.mkdir('lib').join('b.js').write(files['lib/b.js'])
+    tmpdir.mkdir('ext').join('a.js').write(files['ext/a.js'])
+
+    return files
+
+
+def assert_gecko_locale_conversion(package):
+    locale = json.loads(package.read('_locales/en_US/messages.json'))
+
+    assert locale.get('test_Foobar', {}) == LOCALES_MODULE['test.Foobar']
+    assert locale.get('access_key', {}) == {'message': 'access key'}
+    assert locale.get('ampersand', {}) == {'message': 'foo -bar'}
+    assert locale.get('_description', {}) == {'message': 'very descriptive!"'}
+
+
+def assert_convert_js(package, excluded=False):
+    libfoo = package.read('lib/foo.js')
+
+    assert 'var bar;' in libfoo
+    assert 'require.modules["ext_a"]' in libfoo
+
+    assert ('var foo;' in libfoo) != excluded
+    assert ('require.modules["b"]' in libfoo) != excluded
+
+
+def assert_devenv_scripts(package, devenv):
+    manifest = json.loads(package.read('manifest.json'))
+    filenames = package.namelist()
+    scripts = [
+            'ext/common.js',
+            'ext/background.js',
+    ]
+
+    if devenv:
+        assert 'qunit/index.html' in filenames
+        assert 'devenvPoller__.js' in filenames
+        assert 'devenvVersion__' in filenames
+
+        assert '../ext/common.js' in package.read('qunit/index.html')
+        assert '../ext/background.js' in package.read('qunit/index.html')
+
+        assert set(manifest['background']['scripts']) == set(
+            scripts + ['devenvPoller__.js']
+        )
+    else:
+        assert 'qunit/index.html' not in filenames

[Sebastian Noack, 2017/09/21 19:09:05]

Neither should 'devenvPoller__.js' and devenvVersion__' be in the build. In
order to avoid duplication, you can do this like that:

  assert devenv == ('qunit/index.html' in filenames)
  assert devenv == ('devenvPoller__.js' in filenames)
  assert devenv == ('devenvVersion__' in filenames) 

[tlucas, 2017/09/22 09:02:19]

I agree, although i will switch places, see the discussion in
https://codereview.adblockplus.org/29501558/diff2/29501559:29505589/tests/tes...
in LEFT line 192

+
+        assert set(manifest['background']['scripts']) == set(scripts)
+
+
+def assert_base_files(package):
+    filenames = set(package.namelist())
+
+    assert 'bar.json' in filenames
+    assert 'manifest.json' in filenames
+    assert 'lib/foo.js' in filenames
+    assert 'foo/logo_50.png' in filenames
+    assert 'icons/logo_150.png' in filenames
+
+
+def assert_chrome_signature(filename, keyfile):
+    with open(filename, 'r') as fp:
+        content = fp.read()
+
+    _, _, l_pubkey, l_signature = unpack('<4sIII', content[:16])
+    signature = content[16 + l_pubkey: 16 + l_pubkey + l_signature]
+
+    digest = SHA.new()
+    with open(keyfile, 'r') as fp:
+        rsa_key = RSA.importKey(fp.read())
+
+    signer = PKCS1_v1_5.new(rsa_key)
+
+    digest.update(content[16 + l_pubkey + l_signature:])
+    assert signer.verify(digest, signature)
+
+
+def assert_locale_upfix(package):
+    translations = [
+        json.loads(package.read('_locales/{}/messages.json'.format(lang)))
+        for lang in ALL_LANGUAGES
+    ]
+
+    manifest = package.read('manifest.json')
+
+    # Chrome Web Store requires descriptive translations to be present in
+    # every language.
+    for match in re.finditer(r'__MSG_(\S+)__', manifest):
+        name = match.group(1)
+
+        for other in translations[1:]:
+            assert translations[0][name]['message'] == other[name]['message']
+
+
+@pytest.mark.usefixtures(
+    'all_lang_locales',
+    'locale_modules',
+    'gecko_import',
+    'icons',
+    'lib_files',
+    'chrome_metadata',
+    'gecko_webext_metadata',
+)
+@pytest.mark.parametrize('platform', ['chrome', 'gecko-webext'])
+@pytest.mark.parametrize('dev_build_release,buildnum', [
+    ('build', '1337'),
+    ('devenv', None),
+    ('release', None),
+])
+def test_build_webext(platform, dev_build_release, keyfile, tmpdir, srcdir,
+                      buildnum, capsys):
+    release = dev_build_release == 'release'
+    devenv = dev_build_release == 'devenv'
+
+    if platform == 'chrome' and release:
+        key = keyfile
+    else:
+        key = None
+
+    filenames = {
+        'gecko-webext': 'adblockplusfirefox-1.2.3{}.xpi',
+        'chrome': 'adblockpluschrome-1.2.3{{}}.{}'.format(
+            {True: 'crx', False: 'zip'}[release]
+        ),
+    }
+
+    expected_manifest = os.path.join(
+        os.path.dirname(__file__),
+        'expecteddata',
+        'manifest_{}_{}_{}.json'.format(platform, dev_build_release, buildnum),

[Sebastian Noack, 2017/09/21 19:09:04]

The buildnum is redundant here. The filenames are still distinct if you remove
that part.

[tlucas, 2017/09/22 09:02:20]

Done.

+    )
+
+    run_webext_build(platform, dev_build_release, srcdir, buildnum=buildnum,
+                     keyfile=key)
+
+    # The makeIcons() in packagerChrome.py should warn about non-square
+    # icons via stderr.
+    out, err = capsys.readouterr()
+    assert 'icon should be square' in err
+
+    if devenv:
+        content_class = DirContent
+        out_file_path = os.path.join(str(srcdir), 'devenv.' + platform)
+    else:
+        content_class = ZipContent
+
+        if release:
+            add_version = ''
+        else:
+            add_version = '.' + buildnum
+
+        out_file = filenames[platform].format(add_version)
+
+        out_file_path = os.path.abspath(os.path.join(
+            os.path.dirname(__file__), os.pardir, out_file))
+        assert os.path.exists(out_file_path)
+
+    if release and platform == 'chrome':
+        assert_chrome_signature(out_file_path, keyfile)
+
+    with content_class(out_file_path) as package:
+        assert_manifest_content(
+            package.read('manifest.json'), expected_manifest
+        )
+        assert_base_files(package)

[Sebastian Noack, 2017/09/21 19:09:06]

The features tested here are relevant for "edge" builds as well, yet we only
test them "chrome" and "gecko-webext"?

FWIW, I don't see why the tests have been separated into test_packagerWebExt.py
and test_packagerEdge.py in the first place.

[tlucas, 2017/09/22 09:02:18]

This test happens in edge as well, see the 7 single assertions after
"assert_all_locales_present()" in test_build_edge. I would agree on moving them
to a function.

Regarding the two separate files, imho it makes sense to create one test file
for one module, which would be packagerEdge and packagerChrome.

[Sebastian Noack, 2017/09/22 18:31:04]

The logic there seems inconsistent. It seems that either the test build for
"edge" includes less files (and is using less feature), or that you just don't
check for the same files as you do here. Neither makes any sense to me.

Also I wasn't specifically talking about assert_base_files(), but about all the
assert_*() functions here. The same features tested here, are also supported by
the "edge" target.
These aren't unit tests for the packager modules, but integration tests for the
command line interface. (Or alternatively you could argue that the module tested
here is build.py, and not packager*.py). That most of the logic for the "chrome"
and "gecko-webext" targets are implemented in packagerChrome.py, and most of the
logic for the "edge" target is implemented in packagerEdge.py, are
implementation details that aren't relevant on the level we are testing here.

It seems it would simplify the tests a bit, and would make it easier to avoid
unintended inconsistencies if we merge those test modules.

[Sebastian Noack, 2017/09/25 20:21:19]

Yes, I agree that there is no reason to split out common functionality in other
modules if we unify the tests. I'd expect to unify the code path for testing
Chrome/Firefox/Edge builds for the most part (e.g. the entry point should be a
single parametized function for all targets), so that the amount of code will
decrease as well, so that hopefully the readability will not suffer.

+        assert_devenv_scripts(package, devenv)
+        assert_all_locales_present(package, '_locales')
+        assert_gecko_locale_conversion(package)
+        assert_convert_js(package, platform == 'gecko-webext')
+
+        if platform == 'chrome':
+            assert_locale_upfix(package)