Issue 5383 - Add tests for the Chrome and Firefox packagers

packagerChrome.py

Index: packagerChrome.py
diff --git a/packagerChrome.py b/packagerChrome.py
index d75203d17b9e63262e69f44e5112cfc729005d8d..82fe95801ef0cd8bbaf8cf4ff59b8c0f1d3cb420 100644
--- a/packagerChrome.py
+++ b/packagerChrome.py
@@ -43,13 +43,13 @@ def processFile(path, data, params):
 
 
 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