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 difflib
+import json
 import os
+import re
 import shutil
 import zipfile
-import json
-import re
+from xml.etree import ElementTree
 from struct import unpack
-import difflib
 
 import pytest
+from Crypto.Signature import PKCS1_v1_5
+from Crypto.PublicKey import RSA
 from Crypto.Hash import SHA
-from Crypto.PublicKey import RSA
-from Crypto.Signature import PKCS1_v1_5
-from xml.etree import ElementTree
-
 

[Sebastian Noack, 2017/10/17 22:19:06]

Nit: One blank line is enough here.

[tlucas, 2017/10/18 11:23:07]

Done.

 from buildtools import packager
 from buildtools.packagerChrome import defaultLocale
 from buildtools.build import processArgs
 
 LOCALES_MODULE = {
     'test.Foobar': {
         'message': 'Ensuring dict-copy from modules for $domain$',
         'description': 'test description',
         'placeholders': {'content': '$1', 'example': 'www.adblockplus.org'}
     }
@@ skipping 90 matching lines @@
         return content
 
 
 def copy_metadata(filename, tmpdir):
     """Copy the used metadata to the used temporary directory."""
     path = os.path.join(os.path.dirname(__file__), filename)
     destination = str(tmpdir.join(filename))
     shutil.copy(path, destination)
 
 
-def run_webext_build(ext_type, build_opt, srcdir, buildnum=None, keyfile=None):
+def run_webext_build(ext_type, build_opt, srcdir, keyfile=None):
     """Run a build process."""
-    if build_opt == 'build':
-        build_args = ['build']
-    elif build_opt == 'release':
-        build_args = ['build', '-r']
-    else:
-        build_args = ['devenv']
-
-    args = ['build.py', '-t', ext_type] + build_args
+    cmd_mapping = {
+        'devenv': ['devenv'],
+        'development_build': ['build', '-b', '1337'],
+        'release_build': ['build', '-r'],
+    }
+
+    args = ['build.py', '-t', ext_type] + cmd_mapping[build_opt]
 
     if keyfile:
         args += ['-k', keyfile]
-    if buildnum:

[Sebastian Noack, 2017/10/17 22:19:09]

It doesn't seem unneccessary to paremtize the build version, as it is always
1337 for development builds, and absent otherwise.

[tlucas, 2017/10/18 11:23:06]

I assume you meant "It doesn't seem necessary", from what i understand from the
rest of this comment.

If i'm correct, please see the comment below, regarding the
prefix-parametrization.


If i'm wrong, could you please clarify?

[Sebastian Noack, 2017/10/19 05:37:50]

Sorry for the confusion, I commented below. Either way, the build number can be
hard-coded as part of the command line whichever way we go.

-        args += ['-b', buildnum]
 
     processArgs(str(srcdir), args)
 
 
 def locale_files(languages, rootfolder, srcdir):
     """Generate example locales.
 
     languages: tuple, list or set of languages to include
     rootdir: folder-name to create the locale-folders in
     """
@@ skipping 83 matching lines @@
 
     tmpdir.mkdir('lib').join('b.js').write(files['lib/b.js'])
     ext_dir = tmpdir.mkdir('ext')
     ext_dir.join('a.js').write(files['ext/a.js'])
     ext_dir.join('c.js').write(files['ext/c.js'])
 
     return files
 
 
 def comparable_xml(xml):
-    """Create a nonambiguous representation of a given XML tree."""
+    """Create a nonambiguous representation of a given XML tree.
+
+    Note that this function is not safe against ambiguous tags
+    containing differently distributed children, e.g.:
+
+    '<a><b><c/></b><b><d/></b></a>'
+    vs.
+    '<a><b/><b><c/><d/></b></a>'
+
+    This is irrelevant for comparing the XML used by the tests of this
+    module.
+    """
     def get_leafs_string(tree):
         """Recursively build a string representing all xml leaf-nodes."""
         root_str = '{}|{}|{}'.format(tree.tag, tree.tail, tree.text).strip()
         result = []
 
         if len(tree) > 0:
             for subtree in tree:
                 for leaf in get_leafs_string(subtree):
                     result.append('{}__{}'.format(root_str, leaf))
         for k, v in tree.attrib.items():
@@ skipping 36 matching lines @@
     assert 'var bar;' in libfoo
     assert 'webpack:///./ext/a.js' in libfoomap
 
     assert 'var this_is_c;' in libfoo
     assert 'webpack:///./ext/c.js' in libfoomap
 
     assert ('var foo;' in libfoo) != excluded
     assert ('webpack:///./lib/b.js' in libfoomap) != excluded
 
 
-def assert_devenv_scripts(package, devenv):
-    manifest = json.loads(package.read('manifest.json'))
+def assert_devenv_scripts(package, prefix, devenv):
+    manifest = json.loads(package.read(os.path.join(prefix, 'manifest.json')))
     filenames = package.namelist()
     scripts = [
         'ext/common.js',
         'ext/background.js',
     ]
 
-    assert ('qunit/index.html' in filenames) == devenv
-    assert ('devenvPoller__.js' in filenames) == devenv
-    assert ('devenvVersion__' in filenames) == devenv
+    assert (os.path.join(prefix, 'qunit/index.html') in filenames) == devenv
+    assert (os.path.join(prefix, 'devenvPoller__.js') in filenames) == devenv
+    assert (os.path.join(prefix, 'devenvVersion__') in filenames) == devenv
 
     if devenv:
-        assert '../ext/common.js' in package.read('qunit/index.html')
-        assert '../ext/background.js' in package.read('qunit/index.html')
+        quint_index = package.read(os.path.join(prefix, 'qunit/index.html'))
+        assert '../ext/common.js' in quint_index
+        assert '../ext/background.js' in quint_index
 
         assert set(manifest['background']['scripts']) == set(
             scripts + ['devenvPoller__.js']
         )
     else:
         assert set(manifest['background']['scripts']) == set(scripts)
 
 
-def assert_base_files(package, platform):
+def assert_base_files(package, platform, prefix):
     filenames = set(package.namelist())
 
-    if platform in {'chrome', 'gecko'}:
-        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
-    else:
+    if platform == 'edge':
         assert 'AppxManifest.xml' in filenames
         assert 'AppxBlockMap.xml' in filenames
         assert '[Content_Types].xml' in filenames
 
-        assert 'Extension/bar.json' in filenames
-        assert 'Extension/lib/foo.js' in filenames
-
         assert package.read('Assets/logo_44.png') == '44'
         assert package.read('Extension/icons/abp-44.png') == '44'
+
+    assert os.path.join(prefix, 'bar.json') in filenames
+    assert os.path.join(prefix, 'manifest.json') in filenames
+    assert os.path.join(prefix, 'lib/foo.js') in filenames
+    assert os.path.join(prefix, 'foo/logo_50.png') in filenames
+    assert os.path.join(prefix, '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()
@@ skipping 25 matching lines @@
 
 @pytest.mark.usefixtures(
     'all_lang_locales',
     'locale_modules',
     'icons',
     'lib_files',
     'chrome_metadata',
     'gecko_webext_metadata',
     'edge_metadata',
 )
-@pytest.mark.parametrize('platform,dev_build_release,buildnum', [
-    ('chrome', 'build', '1337'),

[Sebastian Noack, 2017/10/17 22:19:07]

The naming of the second parameter is somewhat misleading:

* "build" is development build
* "release" is release build

[tlucas, 2017/10/18 11:23:11]

Renamed it to "command", adhering to the help text in build.py.

[Sebastian Noack, 2017/10/19 05:37:52]

I wasn't talking about the name of the argument variable, though that one was
bogus too, but about its values. "build" is misleading  as both "build" and
"release" are builds, but the former is the development build, while the latter
is the release build.

-    ('chrome', 'devenv', None),
-    ('chrome', 'release', None),
-    ('gecko', 'build', '1337'),
-    ('gecko', 'devenv', None),
-    ('gecko', 'release', None),
-    ('edge', 'build', '1337'),
-    pytest.param('edge', 'devenv', None, marks=pytest.mark.xfail),
-    ('edge', 'release', None),
+@pytest.mark.parametrize('platform,command', [
+    ('chrome', 'development_build'),
+    ('chrome', 'devenv'),
+    ('chrome', 'release_build'),
+    ('gecko', 'development_build'),
+    ('gecko', 'devenv'),
+    ('gecko', 'release_build'),
+    ('edge', 'development_build'),
+    ('edge', 'devenv'),
+    ('edge', 'release_build'),
 ])
-def test_build_webext(platform, dev_build_release, keyfile, tmpdir, srcdir,
-                      buildnum, capsys):
-    release = dev_build_release == 'release'
-    devenv = dev_build_release == 'devenv'
+def test_build_webext(platform, command, keyfile, tmpdir, srcdir, capsys):
+    release = command == 'release_build'
+    devenv = command == 'devenv'
 
     if platform == 'chrome' and release:
         key = keyfile
     else:
         key = None
 
     manifests = {
         'gecko': [('', 'manifest', 'json')],
         'chrome': [('', 'manifest', 'json')],
         'edge': [('', 'AppxManifest', 'xml'),
                  ('Extension', 'manifest', 'json')],
     }
 
     filenames = {
         'gecko': 'adblockplusfirefox-1.2.3{}.xpi',
         'chrome': 'adblockpluschrome-1.2.3{{}}.{}'.format(
             {True: 'crx', False: 'zip'}[release]
         ),
         'edge': 'adblockplusedge-1.2.3{}.appx',
     }
 
     if platform == 'edge':
         prefix = 'Extension'
     else:
         prefix = ''

[Sebastian Noack, 2017/10/17 22:19:09]

I wonder whether we should include stuff like (but not limited) to this, in the
parametization above, getting rid of unnecessary if-statements.

[tlucas, 2017/10/18 11:23:08]

Assuming i was correct above:

You argued that we don't need parametrization for buildnum (since it is directly
dependent on whether we perform a development build or not)

Here you suggest that we introduce parametrization for 'prefix', which is
directly dependent on whether we we perform an edge-build or not.

I guess there are two options to choose from (but i think we should keep it
consistent):

a) Get rid of parametrization for everything that is directly dependent on an
already given parameter (resulting in more ifs).

b) Parametrize basically everything that would need further processing in the
parametrized functions.

I don't have a too strong opinion about this, but it might seem more natural
that parametrization solely is responsible for cycling through the combinations
of the actual desired tests (which in this case would be the combinations of
platform and command), hence option a).

For the sake of readability i could add another function (e.g.
process_parameters()) which could then, outside of run_webext_build(), prepare
those parameters that depend on the parametrization.

What do you think about this?

[Sebastian Noack, 2017/10/19 05:37:50]

Sorry, I actually thought that I deleted this comment before submitting. I'm not
too happy with all those ifs in the logic here. It makes it difficult to verify
that the right things get tested, and in general, abstraction is better than
exceptions (or spaghetti code). But for the matter of progress, I wouldn't
necessarily insist on a refactoring at the current point.

 
-    run_webext_build(platform, dev_build_release, srcdir, buildnum=buildnum,
-                     keyfile=key)
+    run_webext_build(platform, command, srcdir, keyfile=key)
 
     # The makeIcons() in packagerChrome.py should warn about non-square
     # icons via stderr.
-    if platform in {'chrome', 'gecko'}:

[Sebastian Noack, 2017/10/17 22:19:08]

Why is this specific to Chrome/Gecko?

[tlucas, 2017/10/18 11:23:09]

It is not - i guess this is merely a relict from the time when i did not yet
have the full scope of metadata inheritance in mind.

Checking for all platforms now.

-        out, err = capsys.readouterr()
-        assert 'icon should be square' in err
+    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
+            add_version = '.1337'
 
         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_base_files(package, platform)
+        assert_base_files(package, platform, prefix)
         assert_all_locales_present(package, prefix)
         assert_webpack_bundle(package, prefix, platform == 'gecko')
 
         if platform == 'chrome':
             assert_locale_upfix(package)
-        if platform in {'chrome', 'gecko'}:
-            assert_devenv_scripts(package, devenv)
+
+        assert_devenv_scripts(package, prefix, devenv)
 
         for folder, name, ext in manifests[platform]:
-            filename = '{{}}_{}_{}.{{}}'.format(platform, dev_build_release)
+            filename = '{{}}_{}_{}.{{}}'.format(platform, command)
             expected = os.path.join(
                 os.path.dirname(__file__),
                 'expecteddata',
                 filename.format(name, ext),
             )
 
             assert_manifest_content(
                 package.read(os.path.join(folder, '{}.{}'.format(name, ext))),
                 expected,
             )