Issue 5383 - Add tests for the Chrome and Firefox packagers

tests/test_packagerWebExt.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 json
 import re
+from struct import unpack
 
 import pytest
-
-from buildtools import packager
-from buildtools.tests.tools import DirContent

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

These imports also can be combined.

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

Acknowledged.

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

Done.

-from buildtools.tests.tools import ZipContent
-from buildtools.tests.tools import copy_metadata
-from buildtools.tests.tools import run_webext_build
-from buildtools.tests.tools import assert_all_locales_present
-from buildtools.tests.tools import assert_manifest_content
-from buildtools.tests.tools import locale_files
+from Crypto.Hash import SHA
+from Crypto.PublicKey import RSA
+from Crypto.Signature import PKCS1_v1_5
+
+
+from buildtools import packager, packagerChrome
+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'}
     }
 }
@@ skipping 110 matching lines @@
     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):
-    from struct import unpack

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

Is there any specific reason to have these imports are inside of the function?

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

Not at all - merely a result of a speedy adjustment - will change :)

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

Done.

-    from Crypto.Hash import SHA
-    from Crypto.PublicKey import RSA
-    from Crypto.Signature import PKCS1_v1_5
-
     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())
 
@@ skipping 11 matching lines @@
 
     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',
-    'gecko_import',
-    'locale_modules',
-    'icons',
-    'lib_files',
-    'chrome_metadata',
-)
-@pytest.mark.parametrize('dev_build_release', ['build', 'devenv', 'release'])
-def test_build_chrome(dev_build_release, keyfile, tmpdir, srcdir, capsys):
-    from buildtools import packagerChrome
-    release = dev_build_release == 'release'
-    devenv = dev_build_release == 'devenv'
-
-    run_webext_build('chrome', dev_build_release, srcdir, packagerChrome,
-                     keyfile if release else None)
-
-    # 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.chrome')
-    else:
-        content_class = ZipContent
-        out_file = 'adblockpluschrome-1.2.3'
-        if not release:
-            out_file += '.0'
-
-        if release:
-            out_file += '.crx'
-        else:
-            out_file += '.zip'
-
-        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:
-        assert_chrome_signature(out_file_path, keyfile)
-
-    with content_class(out_file_path) as package:
-        assert_base_files(package)
-        assert_devenv_scripts(package, devenv)
-        assert_all_locales_present(package, '_locales')
-        assert_locale_upfix(package)
-        assert_gecko_locale_conversion(package)
-        assert_convert_js(package)
-        expected = os.path.join(
-            os.path.dirname(__file__),
-            'expecteddata',
-            'manifest_chrome_{}.json'.format(dev_build_release),
-        )
-        assert_manifest_content(package.read('manifest.json'), expected)
 
 
 @pytest.mark.usefixtures(
     'all_lang_locales',
     'locale_modules',
     'gecko_import',
     'icons',
     'lib_files',
+    'chrome_metadata',
     'gecko_webext_metadata',
 )
-@pytest.mark.parametrize('dev_build_release', ['build', 'devenv', 'release'])
-def test_build_gecko_webext(dev_build_release, tmpdir, srcdir, capsys):

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

This test seems very similar to the previous one apart from some file naming
differences. What do you think about merging them and using parametrization to
switch between the two versions of the file names (and other minor difference)?
Seems like this wouldn't make things much more complicated and we would
eliminate a lot of code duplication.

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

I agree - will do (also i fear the complexity of the resulting test function
might be a bit over the top)

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

Done.

-    from buildtools import packagerChrome
+@pytest.mark.parametrize('platform', ['chrome', 'gecko-webext'])
+@pytest.mark.parametrize('dev_build_release,buildnum', [
+    ('build', None),
+    ('build', '1337'),
+    ('devenv', None),
+    pytest.param('devenv', '1337', marks=pytest.mark.xfail),
+    ('release', None),
+    ('release', '1337'),
+])
+def test_build_webext(platform, dev_build_release, keyfile, tmpdir, srcdir,
+                      buildnum, capsys):
     release = dev_build_release == 'release'
     devenv = dev_build_release == 'devenv'
 
-    run_webext_build('gecko-webext', dev_build_release, srcdir, packagerChrome)
+    if platform == 'chrome' and release:
+        key = keyfile
+    else:
+        key = None
+
+    filenames = {
+        'gecko-webext': 'adblockplusfirefox-1.2.3{}.{}',
+        'chrome': 'adblockpluschrome-1.2.3{}.{}'
+    }
+
+    extensions = {
+        'gecko-webext': {
+            True: 'xpi',
+            False: 'xpi',
+        },
+        'chrome': {
+            True: 'crx',
+            False: 'zip',
+        },
+    }
+
+    expected_manifest = os.path.join(
+        os.path.dirname(__file__),
+        'expecteddata',
+        'manifest_{}_{}_{}.json'.format(platform, dev_build_release, buildnum),
+    )
+
+    run_webext_build(platform, dev_build_release, srcdir, packagerChrome,
+                     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.gecko-webext')
+        out_file_path = os.path.join(str(srcdir), 'devenv.' + platform)
     else:
         content_class = ZipContent
-        out_file = 'adblockplusfirefox-1.2.3{}.xpi'.format(
-            '.0' if not release else ''
-        )
+
+        if release:
+            add_version = ''
+        else:
+            add_version = '.' + (buildnum or '0')
+        extension = extensions[platform][release]
+
+        out_file = filenames[platform].format(add_version, extension)
 
         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)
         assert_devenv_scripts(package, devenv)
         assert_all_locales_present(package, '_locales')
         assert_gecko_locale_conversion(package)
-        assert_convert_js(package, True)
-
-        expected = os.path.join(
-            os.path.dirname(__file__),
-            'expecteddata',
-            'manifest_gecko-webext_{}.json'.format(dev_build_release),
-        )
-        assert_manifest_content(package.read('manifest.json'), expected)
+        assert_convert_js(package, platform == 'gecko-webext')
+
+        if platform == 'chrome':
+            assert_locale_upfix(package)