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

Port txmongo to Python 3.3+ #124

Merged
merged 7 commits into from
Aug 11, 2015
Merged

Port txmongo to Python 3.3+ #124

merged 7 commits into from
Aug 11, 2015

Conversation

hawkowl
Copy link
Member

@hawkowl hawkowl commented Aug 7, 2015

No description provided.

@fiorix
Copy link
Collaborator

fiorix commented Aug 7, 2015

Rebase it maybe? You did some stuff in c7f430c then changed back in b813711, etc.

@IlyaSkriblovsky
Copy link
Contributor

Nice job!

Wow, I've missed a moment when Trial was ported ty Py3

@hawkowl
Copy link
Member Author

hawkowl commented Aug 7, 2015

Trial is ported in trunk -- 15.4 will be the first release, which is why the travis only works on trunk.

Fixing up the auth tests now...

@IlyaSkriblovsky
Copy link
Contributor

Seems like all tests which talk to DB are hanging

@psi29a
Copy link
Contributor

psi29a commented Aug 7, 2015

Thanks for this! :) Seems to be hanging on python33 and python34 though, We'll have to dig a bit deeper on that.

@hawkowl
Copy link
Member Author

hawkowl commented Aug 7, 2015

I have "PASSED (skips=1, successes=144)" on local 3.3 and 3.4. The authentication tests were busted which I think caused a knock-on. Lets see how these go.

@IlyaSkriblovsky
Copy link
Contributor

That's strange, but when I run tests with Py3 on vagrant machine, all tests which talk to DB are hanging. But if I change "localhost" to "127.0.0.1" in test files, they work well (except auth tests which is distinct issue). Seems like same thing breaks tests on Travis.

@hawkowl
Copy link
Member Author

hawkowl commented Aug 7, 2015

Try updating, the auth tests work now with the latest branch.

I'm about to jet back to Perth, and this patch probably needs a bit more work -- making all trunk builds on allowed failures, moving pyflakes off allowed failures, and some code cleanup + fixing that minor coverage regression.

from twisted.trial import unittest
import txmongo
from txmongo.protocol import MongoClientProtocol
import txmongo.filter as qf

if _PY3:
from twisted.python.compat import xrange
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner to just use range instead of xrange

@IlyaSkriblovsky
Copy link
Contributor

Seems like name resolution is broken in Twisted trunk. Following code works on Twisted 15.3.0, but fails on trunk:

def done(result):
    print('done', result)
    reactor.stop()

reactor.resolve('localhost').addCallback(done)
reactor.run()                                                                                                                                                                                                        
Traceback (most recent call last):
  File "/vagrant/testresolve.py", line 7, in <module>
    reactor.resolve('localhost').addCallback(done)
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/internet/base.py", line 570, in resolve
    return self.resolver.getHostByName(name, timeout)
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/internet/base.py", line 271, in getHostByName
    self.reactor, self.reactor.getThreadPool(),
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/internet/base.py", line 991, in getThreadPool
    self._initThreadPool()
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/internet/base.py", line 956, in _initThreadPool
    from twisted.python import threadpool
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/python/threadpool.py", line 16, in <module>
    from twisted._threads import pool as _pool
  File "/home/vagrant/pyenv3/lib/python3.4/site-packages/twisted/_threads/__init__.py", line 15, in <module>
    from ._pool import pool
ImportError: No module named 'twisted._threads._pool'

That's why all our tests that try to connect to MongoDB on localhost are hanging.

We can work around this by changing "localhost" to "127.0.0.1", but I'd prefer to wait when Twisted trunk will be fixed.

@@ -2,7 +2,7 @@
# Use of this source code is governed by the Apache License that can be
# found in the LICENSE file.

import types
from __future__ import absolute_import, division
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no division operations in this file :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going off the Twisted py3 porting standards (https://twistedmatrix.com/trac/wiki/Plan/Python3#Reviewerchecklist) where it's in there regardless. If that's an issue, I can change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I don't mind at all. Just thought this is a trace of some changes that were reverted.

@hawkowl
Copy link
Member Author

hawkowl commented Aug 11, 2015

The tests pass! Just have to figure out what coveralls is complaining about...

@psi29a
Copy link
Contributor

psi29a commented Aug 11, 2015

That coveralls decreased by 0.02% is no big deal, we're still above 95%.

Hawky, you feel comfortable with merging it anyway? :)

@hawkowl
Copy link
Member Author

hawkowl commented Aug 11, 2015

I think it's in a merging state, if you do. :)

@IlyaSkriblovsky
Copy link
Contributor

Coveralls is not an issue: we have 3 lines in protocol.py which can be passed or missed with some probability during replicaset tests.

@hawkowl, I saw you have fixed Twisted trunk to make (in particular) DNS resolution work. Thanks!

psi29a added a commit that referenced this pull request Aug 11, 2015
Port txmongo to Python 3.3+
@psi29a psi29a merged commit 365a930 into twisted:master Aug 11, 2015
@psi29a
Copy link
Contributor

psi29a commented Aug 11, 2015

Good to go for us, thank you very much @hawkowl :D

@hawkowl
Copy link
Member Author

hawkowl commented Aug 11, 2015

No problem :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants