Fix devhub tests for python3 (#10579)

Fix devhub tests for python3

.travis.yml

@@ -24,10 +24,10 @@ jobs:
     - { stage: python3working, python: 3.6, env: TOXENV=assets }
     - { stage: python3working, python: 3.6, env: TOXENV=es }
     - { stage: python3working, python: 3.6, env: TOXENV=addons-versions-and-files }
+    - { stage: python3working, python: 3.6, env: TOXENV=devhub }
     - { stage: python3working, python: 3.6, env: TOXENV=reviewers-and-zadmin }
     - { stage: python3working, python: 3.6, env: TOXENV=accounts-users-and-ratings }
     - { stage: python3working, python: 3.6, env: TOXENV=main }
-    - { stage: python3, python: 3.6, env: TOXENV=devhub }
     - { stage: python3, python: 3.6, env: TOXENV=amo-lib-locales-and-signing }
 
 env:

src/olympia/addons/forms.py

@@ -5,6 +5,7 @@
 from django.conf import settings
 from django.core.files.storage import default_storage as storage
 from django.forms.formsets import BaseFormSet, formset_factory
+from django.utils.encoding import force_text
 from django.utils.translation import ugettext, ugettext_lazy as _, ungettext
 
 import six
@@ -229,7 +230,7 @@ def clean_categories(self):
                 'You can have only {0} categories.',
                 max_cat).format(max_cat))
 
-        has_misc = filter(lambda x: x.misc, categories)
+        has_misc = list(filter(lambda x: x.misc, categories))
         if has_misc and total > 1:
             raise forms.ValidationError(ugettext(
                 'The miscellaneous category cannot be combined with '
@@ -292,7 +293,7 @@ def icons():
     dirs, files = storage.listdir(settings.ADDON_ICONS_DEFAULT_PATH)
     for fname in files:
         if b'32' in fname and b'default' not in fname:
-            icon_name = fname.split(b'-')[0]
+            icon_name = force_text(fname.split(b'-')[0])
             icons.append(('icon/%s' % icon_name, icon_name))
     return sorted(icons)
 

src/olympia/devhub/file_validation_annotations.py

@@ -5,7 +5,6 @@
 from xml.parsers.expat import ExpatError
 
 from django.utils.translation import ugettext
-from django.utils.encoding import force_bytes
 
 from olympia import amo
 from olympia.lib.akismet.models import AkismetReport
@@ -123,10 +122,8 @@ def annotate_search_plugin_validation(results, file_path, channel):
         return
 
     try:
-        # Requires bytes because defusedxml fails to detect
-        # unicode strings as filenames.
-        # https://gist.github.com/EnTeQuAk/25f99701d8b123f7611acd6ce0d5840b
-        dom = minidom.parse(force_bytes(file_path))
+        with open(file_path, 'r') as file_obj:
+            dom = minidom.parse(file_obj)
     except DefusedXmlException:
         url = 'https://pypi.python.org/pypi/defusedxml/0.3#attack-vectors'
         insert_validation_message(

src/olympia/devhub/forms.py

@@ -67,15 +67,15 @@ def clean(self):
         if any(self.errors):
             return
         # cleaned_data could be None if it's the empty extra form.
-        data = filter(None, [f.cleaned_data for f in self.forms
-                             if not f.cleaned_data.get('DELETE', False)])
+        data = list(filter(None, [f.cleaned_data for f in self.forms
+                                  if not f.cleaned_data.get('DELETE', False)]))
         if not any(d['role'] == amo.AUTHOR_ROLE_OWNER for d in data):
             raise forms.ValidationError(
                 ugettext('Must have at least one owner.'))
         if not any(d['listed'] for d in data):
             raise forms.ValidationError(
                 ugettext('At least one author must be listed.'))
-        users = [d['user'] for d in data]
+        users = [d['user'].id for d in data]
         if sorted(users) != sorted(set(users)):
             raise forms.ValidationError(
                 ugettext('An author can only be listed once.'))
@@ -313,7 +313,7 @@ def clean_source(self):
                             'Unsupported file type, please upload an archive '
                             'file {extensions}.'.format(
                                 extensions=valid_extensions_string)))
-            except (zipfile.BadZipfile, tarfile.ReadError, IOError):
+            except (zipfile.BadZipfile, tarfile.ReadError, IOError, EOFError):
                 raise forms.ValidationError(
                     ugettext('Invalid or broken archive.'))
         return source
@@ -420,9 +420,9 @@ def __init__(self, *args, **kwargs):
         self.fields['max'].queryset = qs.all()
 
     def clean(self):
-        min = self.cleaned_data.get('min')
-        max = self.cleaned_data.get('max')
-        if not (min and max and min.version_int <= max.version_int):
+        min_ = self.cleaned_data.get('min')
+        max_ = self.cleaned_data.get('max')
+        if not (min_ and max_ and min_.version_int <= max_.version_int):
             raise forms.ValidationError(ugettext('Invalid version range.'))
         return self.cleaned_data
 
@@ -465,8 +465,8 @@ def clean(self):
         if any(self.errors):
             return
 
-        apps = filter(None, [f.cleaned_data for f in self.forms
-                             if not f.cleaned_data.get('DELETE', False)])
+        apps = list(filter(None, [f.cleaned_data for f in self.forms
+                                  if not f.cleaned_data.get('DELETE', False)]))
 
         if not apps:
             # At this point, we're raising a global error and re-displaying the
@@ -653,7 +653,7 @@ def clean(self):
         super(CombinedNameSummaryCleanMixin, self).clean()
         name_summary_locales = set(
             list(self.cleaned_data.get('name', {}).keys()) +
-            self.cleaned_data.get('summary', {}).keys())
+            list(self.cleaned_data.get('summary', {}).keys()))
         default_locale = self.instance.default_locale.lower()
         name_values = self.cleaned_data.get('name') or {}
         name_default = name_values.get(default_locale) or ''

src/olympia/devhub/tasks.py

@@ -17,6 +17,7 @@
 from django.core.validators import ValidationError
 from django.db import transaction
 from django.template import loader
+from django.utils.encoding import force_text
 from django.utils.translation import ugettext
 
 import celery
@@ -161,7 +162,7 @@ def wrapper(task, akismet_results, id_or_path, **kwargs):
         task.ignore_result = True
         try:
             data = fn(id_or_path, **kwargs)
-            results = json.loads(data)
+            results = json.loads(force_text(data))
             if akismet_results:
                 annotations.annotate_akismet_spam_check(
                     results, akismet_results)
@@ -448,7 +449,7 @@ def run_addons_linter(path, channel):
     if error:
         raise ValueError(error)
 
-    parsed_data = json.loads(output)
+    parsed_data = json.loads(force_text(output))
 
     result = json.dumps(fix_addons_linter_output(parsed_data, channel))
     track_validation_stats(result)
@@ -460,7 +461,7 @@ def track_validation_stats(json_result):
     """
     Given a raw JSON string of validator results, log some stats.
     """
-    result = json.loads(json_result)
+    result = json.loads(force_text(json_result))
     result_kind = 'success' if result['errors'] == 0 else 'failure'
     statsd.incr('devhub.linter.results.all.{}'.format(result_kind))
 

src/olympia/devhub/templatetags/jinja_helpers.py

@@ -1,5 +1,3 @@
-import urllib
-
 from collections import defaultdict
 
 from django.utils.encoding import force_bytes
@@ -9,6 +7,7 @@
 import six
 
 from django_jinja import library
+from six.moves.urllib.parse import unquote_to_bytes
 
 from olympia import amo
 from olympia.access import acl
@@ -120,7 +119,7 @@ def display_url(url):
     Note: returns a Unicode object, not a valid URL.
     """
     url = force_bytes(url, errors='replace')
-    return urllib.unquote(url).decode('utf-8', errors='replace')
+    return unquote_to_bytes(url).decode('utf-8', errors='replace')
 
 
 @library.global_function

src/olympia/devhub/tests/test_feeds.py

@@ -1,5 +1,7 @@
 import uuid
 
+from django.utils.encoding import force_text
+
 import six
 from six.moves.urllib_parse import urlencode
 
@@ -176,34 +178,35 @@ def test_filter_addon_otherguy(self):
     def test_rss(self):
         self.log_creates(5)
         # This will give us a new RssKey
-        r = self.get_response()
+        self.get_response()
         key = RssKey.objects.get()
 
         assert isinstance(key.key, uuid.UUID)
 
-        r = self.get_response(privaterss=key.key)
-        assert r['content-type'] == 'application/rss+xml; charset=utf-8'
-        assert '<title>Recent Changes for My Add-ons</title>' in r.content
+        res = self.get_response(privaterss=key.key)
+        assert res['content-type'] == 'application/rss+xml; charset=utf-8'
+        assert b'<title>Recent Changes for My Add-ons</title>' in res.content
 
     def test_rss_accepts_verbose(self):
         self.log_creates(5)
-        r = self.get_response()
+        self.get_response()
         key = RssKey.objects.get()
-        r = self.get_response(privaterss=str(key.key))
-        assert r['content-type'] == 'application/rss+xml; charset=utf-8'
-        assert '<title>Recent Changes for My Add-ons</title>' in r.content
+        res = self.get_response(privaterss=str(key.key))
+        assert res['content-type'] == 'application/rss+xml; charset=utf-8'
+        assert b'<title>Recent Changes for My Add-ons</title>' in res.content
 
     def test_rss_single(self):
         self.log_creates(5)
         self.log_creates(13, self.addon2)
 
         # This will give us a new RssKey
-        r = self.get_response(addon=self.addon.id)
+        self.get_response(addon=self.addon.id)
         key = RssKey.objects.get()
-        r = self.get_response(privaterss=key.key)
-        assert r['content-type'] == 'application/rss+xml; charset=utf-8'
-        assert len(pq(r.content)('item')) == 5
-        assert '<title>Recent Changes for %s</title>' % self.addon in r.content
+        res = self.get_response(privaterss=key.key)
+        assert res['content-type'] == 'application/rss+xml; charset=utf-8'
+        assert len(pq(res.content)('item')) == 5
+        assert '<title>Recent Changes for %s</title>' % self.addon in (
+            force_text(res.content))
 
     def test_rss_unlisted_addon(self):
         """Unlisted addon logs appear in the rss feed."""
@@ -293,11 +296,11 @@ def test_hidden(self):
         res = self.get_response(addon=self.addon.id)
         key = RssKey.objects.get()
         res = self.get_response(privaterss=key.key)
-        assert "<title>Comment on" not in res.content
+        assert b'<title>Comment on' not in res.content
 
     def test_no_guid(self):
         self.log_creates(1)
         self.get_response(addon=self.addon.id)
         key = RssKey.objects.get()
         res = self.get_response(privaterss=key.key)
-        assert "<guid>" not in res.content
+        assert b'<guid>' not in res.content

src/olympia/devhub/tests/test_forms.py

@@ -6,6 +6,7 @@
 from django.conf import settings
 from django.core.files.storage import default_storage as storage
 from django.utils import translation
+from django.utils.encoding import force_text
 
 import mock
 import pytest
@@ -255,8 +256,8 @@ def test_preview_modified(self, update_mock):
         name = 'transparent.png'
         form = forms.PreviewForm({'caption': 'test', 'upload_hash': name,
                                   'position': 1})
-        with storage.open(os.path.join(self.dest, name), 'w') as f:
-            shutil.copyfileobj(open(get_image_path(name)), f)
+        with storage.open(os.path.join(self.dest, name), 'wb') as f:
+            shutil.copyfileobj(open(get_image_path(name), 'rb'), f)
         assert form.is_valid()
         form.save(addon)
         assert update_mock.called
@@ -267,8 +268,8 @@ def test_preview_size(self, pngcrush_image_mock):
         name = 'teamaddons.jpg'
         form = forms.PreviewForm({'caption': 'test', 'upload_hash': name,
                                   'position': 1})
-        with storage.open(os.path.join(self.dest, name), 'w') as f:
-            shutil.copyfileobj(open(get_image_path(name)), f)
+        with storage.open(os.path.join(self.dest, name), 'wb') as f:
+            shutil.copyfileobj(open(get_image_path(name), 'rb'), f)
         assert form.is_valid()
         form.save(addon)
         preview = addon.previews.all()[0]
@@ -433,7 +434,8 @@ def test_success(self, save_persona_image_mock,
         # Upload header image.
         img = open(get_image_path('persona-header.jpg'), 'rb')
         r_ajax = self.client.post(header_url, {'upload_image': img})
-        data.update(header_hash=json.loads(r_ajax.content)['upload_hash'])
+        content = json.loads(force_text(r_ajax.content))
+        data.update(header_hash=content['upload_hash'])
 
         # Populate and save form.
         self.post()

src/olympia/devhub/tests/test_helpers.py

@@ -1,12 +1,12 @@
 # -*- coding: utf-8 -*-
-import urllib
-
 from django.utils import translation
+from django.utils.encoding import force_bytes, force_text
 
 import pytest
 import six
 
 from mock import Mock
+from six.moves.urllib.parse import quote
 
 from olympia import amo
 from olympia.activity.models import ActivityLog
@@ -72,17 +72,17 @@ class TestDisplayUrl(amo.tests.BaseTestCase):
 
     def setUp(self):
         super(TestDisplayUrl, self).setUp()
-        self.raw_url = u'http://host/%s' % 'フォクすけといっしょ'.decode('utf8')
+        self.raw_url = u'http://host/%s' % u'フォクすけといっしょ'
 
     def test_utf8(self):
-        url = urllib.quote(self.raw_url.encode('utf8'))
-        assert render('{{ url|display_url }}', {'url': url}) == (
+        url = quote(self.raw_url.encode('utf8'))
+        assert render(u'{{ url|display_url }}', {'url': url}) == (
             self.raw_url)
 
     def test_unicode(self):
-        url = urllib.quote(self.raw_url.encode('utf8'))
-        url = six.text_type(url, 'utf8')
-        assert render('{{ url|display_url }}', {'url': url}) == (
+        url = quote(self.raw_url.encode('utf8'))
+        url = force_text(force_bytes(url, 'utf8'), 'utf8')
+        assert render(u'{{ url|display_url }}', {'url': url}) == (
             self.raw_url)
 
 

src/olympia/devhub/tests/test_tasks.py

@@ -75,7 +75,7 @@ def _uploader(resize_size, final_size):
     original_size = (339, 128)
 
     src = tempfile.NamedTemporaryFile(
-        mode='r+w+b', suffix='.png', delete=False, dir=settings.TMP_PATH)
+        mode='r+b', suffix='.png', delete=False, dir=settings.TMP_PATH)
 
     if not isinstance(final_size, list):
         final_size = [final_size]
@@ -122,18 +122,20 @@ def test_recreate_previews(pngcrush_image_mock):
     addon = addon_factory()
     # Set up the preview so it has files in the right places.
     preview_no_original = Preview.objects.create(addon=addon)
-    with storage.open(preview_no_original.image_path, 'w') as dest:
-        shutil.copyfileobj(open(get_image_path('preview_landscape.jpg')), dest)
-    with storage.open(preview_no_original.thumbnail_path, 'w') as dest:
-        shutil.copyfileobj(open(get_image_path('mozilla.png')), dest)
+    with storage.open(preview_no_original.image_path, 'wb') as dest:
+        shutil.copyfileobj(open(get_image_path('preview_landscape.jpg'), 'rb'),
+                           dest)
+    with storage.open(preview_no_original.thumbnail_path, 'wb') as dest:
+        shutil.copyfileobj(open(get_image_path('mozilla.png'), 'rb'), dest)
     # And again but this time with an "original" image.
     preview_has_original = Preview.objects.create(addon=addon)
-    with storage.open(preview_has_original.image_path, 'w') as dest:
-        shutil.copyfileobj(open(get_image_path('preview_landscape.jpg')), dest)
-    with storage.open(preview_has_original.thumbnail_path, 'w') as dest:
-        shutil.copyfileobj(open(get_image_path('mozilla.png')), dest)
-    with storage.open(preview_has_original.original_path, 'w') as dest:
-        shutil.copyfileobj(open(get_image_path('teamaddons.jpg')), dest)
+    with storage.open(preview_has_original.image_path, 'wb') as dest:
+        shutil.copyfileobj(open(get_image_path('preview_landscape.jpg'), 'rb'),
+                           dest)
+    with storage.open(preview_has_original.thumbnail_path, 'wb') as dest:
+        shutil.copyfileobj(open(get_image_path('mozilla.png'), 'rb'), dest)
+    with storage.open(preview_has_original.original_path, 'wb') as dest:
+        shutil.copyfileobj(open(get_image_path('teamaddons.jpg'), 'rb'), dest)
 
     tasks.recreate_previews([addon.id])