Skip to content
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

python3 compatibility #185

Merged
merged 19 commits into from Jul 22, 2018

Conversation

@keszybz
Copy link
Contributor

commented Aug 9, 2016

Not everything is converted. Test results:

$ py.test-2.7 --tb=no .
================ test session starts ===========================
platform linux2 -- Python 2.7.11, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /var/tmp/mirrormanager2, inifile: 
plugins: cov-2.2.1
collected 126 items 

tests/test_mdtr.py FF
tests/test_mm_lib_model.py ...................
tests/test_mmlib.py ...............................................
tests/test_mta.py F
tests/test_ui_admin.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
tests/test_ui_app.py ..FFFFFFFFFFFFF.F.F...F.
tests/test_umdl.py FF
========== 52 failed, 74 passed in 81.31 seconds ===============
py.test-3.5 --tb=no .
================== test session starts =======================
platform linux -- Python 3.5.1, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /var/tmp/mirrormanager2, inifile: 
plugins: cov-2.2.1
collected 126 items 

tests/test_mdtr.py FF
tests/test_mm_lib_model.py ...................
tests/test_mmlib.py ...............................................
tests/test_mta.py F
tests/test_ui_admin.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
tests/test_ui_app.py FFFFFFFFFFFFFFFFFFFFFFFF
tests/test_umdl.py FF
============ 60 failed, 66 passed in 62.26 seconds  =================

As you can see, tests import fine under Python 3.5, and there's only a few more failures. A bunch of tests fail because of bytes/str incompatiblity under Python 3. This is in the testing code, and I don't know what the proper solution is, so I left this alone.

mirrorlist/mirrorlist_client.wsgi runs, mirrorlist/mirrorlist_server.py fails with GeoIP import which I can't easy obtain.

Nibbles-away-at #139.

@@ -3,6 +3,7 @@
'''
mirrormanager2 tests.
'''
from builtins import range

This comment has been minimized.

Copy link
@pypingou

pypingou Aug 11, 2016

Member

This seems to fail on py2

This comment has been minimized.

Copy link
@keszybz

keszybz Aug 11, 2016

Author Contributor

Aw, it's a module from python-future.

@pypingou

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Most of the changes look good, although some seem to not run on py2.

I am a little more concern about the state of the test suite, I wonder if we do not want to fix it first so that we have a clearer picture of this PR.

@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

I forgot that python2 doesn't have the builtins module, and it is provided by python2-future rpm. Using future is a bit more convenient, but in this case it's not worth the depenency, I think. I'll rework the patch to simply use normal range under python2, in this case it really doesn't matter.

I am a little more concern about the state of the test suite, I wonder if we do not want to fix it first so that we have a clearer picture of this PR.

From what I could tell, the test suite has only partial coverage, so it's not going to provide any kind of certainty anyway, unless it is significantly extended and reworked.

Most of those patches are to imports and syntax, so they're relatively safe: if they fail, they fail at import time, so if the code actually loads, it most likely works correctly too.

@keszybz keszybz force-pushed the keszybz:py3k branch from f79a34a to f22348a Aug 11, 2016
@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

Updated.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2016

@keszybz Since you've added commits after posting the original message with test results, have the test results improved any?

The GeoIP import issue should be resolved now, since python3-GeoIP is now available.

@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2016

master @ today:

$ py.test-2.7 tests --tb=no
================== test session starts =====================
platform linux2 -- Python 2.7.12, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /home/zbyszek/python/mirrormanager2, inifile: 
plugins: xdist-1.13.1
collected 126 items 

tests/test_mdtr.py FF
tests/test_mm_lib_model.py ...................
tests/test_mmlib.py ...............................................
tests/test_mta.py F
tests/test_ui_admin.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
tests/test_ui_app.py ..FFFFFFFFFFFFF.F.F...F.
tests/test_umdl.py FF

======== 52 failed, 74 passed in 50.29 seconds ==================
py.test-2.7 tests --tb=no  23.96s user 2.26s system 51% cpu 51.393 total

In my 3k branch with the following patch on top:

diff --git mirrormanager2/lib/umdl.py mirrormanager2/lib/umdl.py
index 18f62712ee..fc5e0ab4df 100644
--- mirrormanager2/lib/umdl.py
+++ mirrormanager2/lib/umdl.py
@@ -353,7 +353,7 @@ def make_repository(session, directory, relativeDName, category, target):
         if ver is None:
             if not relativeDName.endswith('/'):
                 relativeDName += '/'
-            ver = create_version_from_path(category, relativeDName)
+            ver = create_version_from_path(session, category, relativeDName)
             session.add(ver)
             session.flush()
             version_cache.append(ver)
$ py.test-2.7 tests --tb=no
================== test session starts =====================
platform linux2 -- Python 2.7.12, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /home/zbyszek/python/mirrormanager2, inifile: 
plugins: xdist-1.13.1
collected 126 items 

tests/test_mdtr.py FF
tests/test_mm_lib_model.py ...................
tests/test_mmlib.py ...............................................
tests/test_mta.py F
tests/test_ui_admin.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
tests/test_ui_app.py ..FFFFFFFFFFFFF.F.F...F.
tests/test_umdl.py FF
============= 52 failed, 74 passed in 50.08 seconds ========
$ py.test-3.5 tests --tb=no
============================= test session starts ==============================
platform linux -- Python 3.5.1, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /home/zbyszek/python/mirrormanager2, inifile: 
plugins: cov-2.2.1
collected 126 items 

tests/test_mdtr.py FF
tests/test_mm_lib_model.py ...................
tests/test_mmlib.py ...............................................
tests/test_mta.py F
tests/test_ui_admin.py FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF
tests/test_ui_app.py FFFFFFFFFFFFFFFFFFFFFFFF
tests/test_umdl.py FF

==================== 60 failed, 66 passed in 46.65 seconds =====================

And again, a bunch of failures are because of bytes/str issues:

>       self.assertEqual(stdout, '')
E       AssertionError: b'' != ''

A lot of those would probably go away if the tests were changed to use universal_newlines=True or something. I can do that patch too, if you think the general direction is worth pursing and there's a chance that this'll be used at some point.

Oh, I found one more thing to fix:

>           if isinstance(admins, basestring):
E           NameError: name 'basestring' is not defined
@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2016

mirrorlist/mirrorlist_client.wsgi runs, mirrorlist/mirrorlist_server.py fails in handle_country_continent_redirect (new_db is empty), in both python2 and python3, it's probably because of my incomplete environement.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2016

@pypingou How does this look to you so far?

@adrianreber

This comment has been minimized.

Copy link
Member

commented Jul 7, 2018

@keszybz: are you still interested in this PR? The test suite is functional again since a few months and I would like to merge this if you would rebase. If this is too old and you do not care about it anymore (understandable after almost two years) I would also cherry-pick your commits and fixup the commits with the conflicts manually.

@keszybz keszybz force-pushed the keszybz:py3k branch from dca56e8 to a394cbc Jul 7, 2018
@keszybz keszybz changed the title Initial stab at python3 compatibility python3 compatibility Jul 7, 2018
keszybz added 14 commits Aug 8, 2016
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
For those uses there is no practical difference between range and xrange
(no big ranges are used), so just get rid of xrange.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
… and from __future__ import print_function for python2 compat.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
futurize-3 -w -n -p -f libfuturize.fixes.fix_absolute_import .

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
2to3 doesn't know that those objects are a dict too
and does not apply the fixer.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
@keszybz keszybz force-pushed the keszybz:py3k branch from a394cbc to 4d37354 Jul 7, 2018
@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2018

I rebased the branch, with some new commits. All the tests now pass for me on F28.

keszybz added 2 commits Jul 7, 2018
tests/test_ui_app.py::FlaskUiAppTest::test_site_new
  /home/zbyszek/python/mirrormanager2/tests/../mirrormanager2/app.py:288: FlaskWTFDeprecationWarning: "flask_wtf.Form" has been renamed to "FlaskForm" and will be removed in 1.0.
    form = forms.AddSiteForm()

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
keszybz added 3 commits Jul 7, 2018
This makes tests/test_umdl.py and tests/test_mdtr.py pass under
python3.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
This fixes tests/test_crawler.py under python3.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
This is the right thing to do according to
pallets/flask#716.
— data is intentionally the undecoded bytes, and we need to either
decode it outselves, our use .get_data(as_text=True). This latter
seems to be a nicer option.

I used temporary 'data' variable to assign the decoded text.
.get_data() is caching by default, so this doesn't save much work,
but avoid the fairly verbose method invocation in asserts.

In some places 'data' was used for .post(), and this is renamed
to 'post_data' to avoid the name clash.

This makes all tests pass under python3.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
@keszybz keszybz force-pushed the keszybz:py3k branch from 4d37354 to b3d7490 Jul 7, 2018
@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2018

Hm, rawhide build fails with "nosetests: no such command". Maybe it'd be better to switch to pytest anyway?

@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2018

Tests also pass in mock with python3.7 (using pytest-2 and pytest-3).

@adrianreber

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

@keszybz Do you have patches to use pytest? Is that a drop-in replacement for nosetest?

@keszybz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2018

It's a drop-in replacement and imho the best option currently. (There's probably some niche stuff that is not entirely compatible, but pytest supports pretty much everything that the built-in unittest and also nosetests do, usually with the same syntax). Upstream nosetests development has stopped, see http://nose.readthedocs.io/en/latest/#note-to-users.

@adrianreber adrianreber merged commit 7360aa9 into fedora-infra:master Jul 22, 2018
2 checks passed
2 checks passed
DCO All commits have a DCO sign-off from the author
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adrianreber

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

@keszybz Thanks for your work. I just changed mirrormanager to pytest and merged your pull request.

@keszybz keszybz deleted the keszybz:py3k branch Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.