Issue 5383 - Add tests for the Chrome and Firefox packagers

packagerChrome.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 errno
 import io
 import json
 import os
 import re
 from StringIO import StringIO
@@ skipping 25 matching lines @@
     return result
 
 
 def processFile(path, data, params):
     # We don't change anything yet, this function currently only exists here so
     # that it can be overridden if necessary.
     return data
 
 
 def makeIcons(files, filenames):
-    try:
-        from PIL import Image
-    except ImportError:
-        import Image
     icons = {}
     for filename in filenames:
-        width, height = Image.open(StringIO(files[filename])).size
+        png_tuple = (137, 'PNG', '\r\n', '\x1a', '\n')
+        header = struct.unpack('>B3s2scc', files[filename][0:8])

[Sebastian Noack, 2017/09/20 00:58:00]

There is no reason to split the magic number into separate parts.

Also if there are less bytes in the file than to be unpacked, this will cause
an exception that is less descriptive than the one you would raise otherwise.

So how about this?

  try:
    magic, width, height = struct.unpack_from('>8s8xii', files[filename])
  except struct.error:
    magic = None
  if magic != '\x89PNG\r\n\x1a\n':
    raise Exception(...)

[tlucas, 2017/09/20 08:52:49]

Done.

+        assert header == png_tuple, '{} is no valid PNG.'.format(filename)

[Vasily Kuznetsov, 2017/09/19 17:52:26]

Python's `assert` is not a reliable method to test for actually possible
erroneous conditions because it's removed if the code is compiled with
optimization [1]. It's only safe to rely on asserts during tests (since you know
that the tests won't run with optimizations) and my understanding is that for
real errors you should use a `raise` statement instead and raise a real
exception (not `AssertionError`).

This check is for a rather rare condition, but it's still an error that might
occur and that we (as developers of this script) have not control over. I think
we should use a `raise` here.

[1]:https://docs.python.org/2/reference/simple_stmts.html#grammar-token-assert_stmt

[Sebastian Noack, 2017/09/20 00:58:00]

I'd like to add that the general idea of programming with assertions (not only
in Python) is to test your code assumptions in order to make debugging easier.
Asserts are meant to detect programming mistakes, not to signal runtime errors
that might occur if the code is behaving as expected. That Python removes
assertions from optimized bytecode, and that you mostly find assertions in test
suites, is just the logical consequence.

[tlucas, 2017/09/20 08:52:49]

Thanks for the insight - restructured this code along with the comment above.

+
+        width, height = struct.unpack('>ii', files[filename][16:24])
         if(width != height):
             print >>sys.stderr, 'Warning: %s size is %ix%i, icon should be square' % (filename, width, height)
         icons[width] = filename
     return icons
 
 
 def createScriptPage(params, template_name, script_option):
     template = getTemplate(template_name, autoEscape=True)
     return template.render(
         basename=params['metadata'].get('general', 'basename'),
@@ skipping 353 matching lines @@
                 params, 'testIndex.html.tmpl', ('general', 'testScripts')
             )
 
     zipdata = files.zipToString()
     signature = None
     pubkey = None
     if keyFile != None:
         signature = signBinary(zipdata, keyFile)
         pubkey = getPublicKey(keyFile)
     writePackage(outFile, pubkey, signature, zipdata)