New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug 1467518: Migration from Python 2 to Python2/3 #5071

Merged
merged 5 commits into from Nov 12, 2018

Conversation

Projects
None yet
2 participants
@MatonAnthony
Copy link
Contributor

MatonAnthony commented Nov 2, 2018

Address the remaining low hanging fruits tests on the migration from Python 2.7 to a compatible Python 2.7/3.6+ compatible codebase.

MatonAnthony added some commits Nov 2, 2018

bug 1467518: Import reduce from six.moves
In Python 3 reduce is part of the functools package, six.moves.reduce is
a compatibility layer that works in both Python 2.7 and Python 3.
bug 1467518: Switch the paginate from zip to list
In Python 3 queryset returns a zip instead of a list.

We need a list to get information such as length.
bug 1467518: Migration towards Python 3
- Check against list of keys instead of ord_keys object (For the
usecases here the ordering is irrelevant)
- Adjust assert to use .decode()
@@ -167,7 +167,7 @@ def test_api_safe(client, section_doc, section_case, if_none_match, method):
str(section_doc.current_revision_id))

if method == 'GET':
assert response.content == exp_content
assert response.content == exp_content.encode('utf-8')

This comment has been minimized.

@MatonAnthony

MatonAnthony Nov 2, 2018

Contributor
Suggested change Beta
assert response.content == exp_content.encode('utf-8')
assert response.content == exp_content.decode('utf-8')

I think I made a mistake there. (It gives us an opportunity to test the suggestion features from Github

This comment has been minimized.

@jwhitlock

jwhitlock Nov 2, 2018

Member

I'd expect response.content to be a UTF-8 encoded binary string, and exp_content to be a binary string in Python 2 and a unicode string in Python 3. I think you need to change line 158 to:

exp_content = b''

or, better yet, get rid of the variable and change this line to:

Suggested change Beta
assert response.content == exp_content.encode('utf-8')
assert response.content == b''
@jwhitlock
Copy link
Member

jwhitlock left a comment

Thanks @MatonAnthony, this is a big step forward!

Before this change:

224 failed, 1532 passed, 1 xfailed, 3831 warnings, 6 error in 196.82 seconds

After this change:

150 failed, 1606 passed, 1 xfailed, 3997 warnings, 6 error in 183.69 seconds

I think you identified some further changes, and I have some nits. Since this doesn't break any Python 2 code, I'm approving as it is. Let me know if you want it merged (merge commit looks right to me) or if you want to continue working on it.

@@ -20,6 +20,7 @@
from django.utils.translation import ugettext_lazy as _
from polib import pofile
from pytz import timezone
from six.moves import xrange

This comment has been minimized.

@jwhitlock

jwhitlock Nov 2, 2018

Member

Interesting fix!

assert 'Results for' in response.content
assert 'an article title' in response.content
assert '4 documents found for "article" in English' in response.content
assert 'Results for' in response.content.decode('utf-8')

This comment has been minimized.

@jwhitlock

jwhitlock Nov 2, 2018

Member

nit: I've been using this pattern other places, which feels less repetitive and less magical to me:

content = response.content.decode(response.charset)
assert 'Results for' in content
assert 'an article title' in content
assert '4 documents found for "article" in English' in content
@@ -467,7 +467,7 @@ def test_feeds_update_after_doc_tag_change(client, wiki_user, root_doc):
response = client.get(reverse('wiki.feeds.recent_documents',
args=['atom', tag]), follow=True)
assert response.status_code == 200
assert root_doc.title not in response.content
assert root_doc.title not in response.content.decode()

This comment has been minimized.

@jwhitlock

jwhitlock Nov 2, 2018

Member

nit: Don't omit the encoding in general. The docs say this uses the "default string encoding" when omitted. sys.getdefaultencoding() returns ascii for me. Probably OK for the test, if there are no non-ASCII characters in the response, but not in general.

@@ -167,7 +167,7 @@ def test_api_safe(client, section_doc, section_case, if_none_match, method):
str(section_doc.current_revision_id))

if method == 'GET':
assert response.content == exp_content
assert response.content == exp_content.encode('utf-8')

This comment has been minimized.

@jwhitlock

jwhitlock Nov 2, 2018

Member

I'd expect response.content to be a UTF-8 encoded binary string, and exp_content to be a binary string in Python 2 and a unicode string in Python 3. I think you need to change line 158 to:

exp_content = b''

or, better yet, get rid of the variable and change this line to:

Suggested change Beta
assert response.content == exp_content.encode('utf-8')
assert response.content == b''
@MatonAnthony

This comment has been minimized.

Copy link
Contributor

MatonAnthony commented Nov 2, 2018

I will fix the nits first, then we can merge so we keep a small PR.

bug 1467518: Improve code quality
The previous code in this PR had some quality flaws.

- Multiple decoding of the same content
- Decoding UTF-8 content using default decoder (which can be ASCII on
some systems)

@MatonAnthony MatonAnthony force-pushed the MatonAnthony:1467518-part3 branch from e525ca5 to bf25601 Nov 11, 2018

@MatonAnthony

This comment has been minimized.

Copy link
Contributor

MatonAnthony commented Nov 11, 2018

  • The failing Python 2.7 Travis job is failing due to a GPG key related issue, it probably just needs a rerun.
  • The security/snyk job is failing, my understanding is that snyk is only checking for dependencies in package.json, I didn't modified the frontend, so it is probably unrelated. (I cannot really check because I don't have access to the project on snyk)
@jwhitlock

This comment has been minimized.

Copy link
Member

jwhitlock commented Nov 11, 2018

The test failure is the Docker image, and it appears that getting the GPG keys for the node package is failing. Restarting doesn't help. We saw similar issues when node 8 was made the LTS version, and the build became more reliable when we updated. node 10 was made the designated LTS version a few weeks ago. We can ignore it, and it may be better when someone kicks the server on Monday.

Snyk looks at the package.json in your PR and compares it to master. We recently updated package.json to avoid some security vulnerabilities, but your PR was branched before this change. The snyk warning can be ignored, and a rebase will fix it.

@jwhitlock jwhitlock merged commit 25041d4 into mozilla:master Nov 12, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
security/snyk - package.json (mdn) 1 new, high severity vulnerable dependency path
Details
@@ -51,7 +52,7 @@ def is_untrusted(request):

def paginate(request, queryset, per_page=20):
"""Get a Paginator, abstracting some common paging actions."""
paginator = Paginator(queryset, per_page)
paginator = Paginator(list(queryset), per_page)

This comment has been minimized.

@jwhitlock

jwhitlock Nov 19, 2018

Member

This caused bug 1508268, where the revision dashboard fails because the view is attempting to process the entire revision history. I'm reverting this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment