Port twisted.trial.runner to Python 3

[exarkun]

twisted.trial.runner is a big part of the guts of the trial command line tool. In order to be able to run the trial command line tool on Python 3, we need to port twisted.trial.runner to Python 3.

[itamar]

Not going to happen in minimal port.

[realcr]

Python3 porting fixes

[realcr]

I Ported some parts of files to be Python3 compatible. Mostly did simple things:

• Used the 'as' keyword in the except statements.
• Dealt with lazy evaluation of map.
• Dealt with sys.maxint.
• Some names of packages that have changed, like StringIO are dealt with.
• Sorting using Key Function instead of Compare function.

The trial tests for python2 passed. Tests for python3 didn't pass for me in the beginning, and they still don't pass in the same way.

It's my first time, please tell me if something is need to be done better or in another way. Regards, real.

[realcr]

Some porting to Python3. This time the right order :)

[itamar]

Thanks for working on this! Please read https://twistedmatrix.com/trac/wiki/Plan/Python3#Methodology as well as the rest of the page and resubmit (presumably as multiple tickets).

[realcr]

Some minor changes to trial.runner to work on python3.3 too:

• implementer instead of implements.
• open instead of file.
• FilePath should only be constructed with bytes, so added encode('utf-8') to self.workingDirectory.

Note that the trial.runner is not fully ported yet.

Included patch file runner_port3.patch

[realcr]

minor changes to runner.py for python3 porting.

[kmike]

Hey Twisted developers,

we need twisted.trial.runner to start porting Scrapy, so I tried to understand better what should be done for this task. I used 'modulegraph' package to get a list of modules that need to be ported before twisted.trial.runner. Here are the results (modules that are marked as ported are excluded):

twisted.internet

• twisted.internet._baseprocess (imported by ported twisted.internet.posixbase)
• twisted.internet._dumbwin32proc (imported by ported twisted.internet.posixbase)
• twisted.internet._oldtls (imported by ported twisted.internet.tcp)
• twisted.internet._pollingfile (imported by ported twisted.internet.posixbase)
• twisted.internet._ssl (imported by ported twisted.internet.tcp)
• twisted.internet._sslverify (imported by ported twisted.internet.ssl)
• twisted.internet.fdesc (imported by ported twisted.internet.base)
• twisted.internet.process (imported by ported twisted.internet.posixbase)
• twisted.internet.unix (imported by ported twisted.internet.posixbase)
• twisted.internet.win32eventreactor (imported by ported twisted.internet.selectreactor)

twisted.internet looks in a good shape. All modules that are not explicitly ported are dependencies of modules that are explicitly ported.

twisted.persisted

• twisted.persisted
• twisted.persisted.styles

It seems some other twisted.persisted modules should be also ported.

twisted.python

• twisted.python._initgroups (imported by ported twisted.python.util)
• twisted.python._inotify (imported by ported twisted.python.runtime)
• twisted.python.modules
• twisted.python.reflect
• twisted.python.win32 (imported from several ported modules)
• twisted.python.zippath
• twisted.python.zipstream (not needed, see ​http://twistedmatrix.com/trac/ticket/4035)

twisted.python.zippath and twisted.python.modules must be ported.

It looks like all twisted.python.reflect usages in trial can be replaced with twisted.python._reflectpy3.

twisted.trial

• twisted.trial._asyncrunner (used by ported twisted.trial.unittest)
• twisted.trial.reporter

Both must be ported. For twisted.trial.reporter see ​http://twistedmatrix.com/trac/ticket/5964

Conclusion

twisted.trial.reporter, twisted.python.zippath and modules from twisted.persisted don't depend on other unported modules and can be ported first.

After that twisted.python.modules (which depends on twisted.python.zippath) and twisted.trial._asyncrunner (which depends on twisted.trial.reporter) can be ported.

After that it should become possible to port twisted.trial.runner. Porting of twisted.python.reflect is not necessary for this sub-task.

I haven't looked into code in much details, so I may have missed something. But hope this helps.

[exarkun]

Hi kmike,

Your comment is incredibly helpful. Thank you very much!

[multani]

This still doesn't meet the criteria from https://twistedmatrix.com/trac/wiki/Plan/Python3#Methodology I'm removing the review keyword in the meanwhile.

[cpdean]

I'm going to start looking at this to port twisted.trial.test.test_reporter #5964

[hawkowl]

(In [44052]) Branching to trialrunner-py3-5965.

[hawkowl]

(In [44936]) Branching to trialrunner-py3-5965-2.

[adiroiban]

I have merged the change for twisted/plugins py3 syntax in trunk #7915 so your branch should no longer need to touch the plugins code.

Looking forward to review your branch :)

Thanks!

[hawkowl]

(In [45032]) Branching to trialrunner-py3-5965-3.

[hawkowl]

(In [45132]) Branching to trialrunner-py3-5965-4.

[hawkowl]

(In [45155]) Branching to trialrunner-py3-5965-5.

[hawkowl]

(In [45181]) Branching to trialrunner-py3-5965-6.

[hawkowl]

(In [45213]) Branching to trialrunner-py3-5965-7.

[hawkowl]

Okay, this is a big one.

This:

• Ports trial (most of it) to Python 3.
• Makes the test suite pass, with some now-irrelevant ones (eg. loading unbound methods) skip.
• Makes the trial3 executable install, and it's usable.

Please review. I originally didn't intend on having this as one ticket, but the changes are mostly inseparable from each other, and the vast majority of the changes are porting tests + the new loader.

[adiroiban]

Many thanks for working on this! I am looking forward to replace admin/run-python3-tests with trial.

The review has no conver letter so I am a bit confused about the changes and how I can test them.

Why do we need trial and trial3 ? Why not just trial with a generic #!/usr/bin/env python ?

A trial3 would make sense for me if it could use the info from dist3

Why do we need Py3TestLoader? Why can we just use and update the py2 loader ? or just use something based on stdlib?

---

In twisted/trial/reporter.py why do we need the new stop method?... and it comes witout docstring

For filenameToModule I see a new code which handles the case when reflactAny return something which is not in package. Do we have a test for this case?

---

In twisted/trial/test/test_loader.py is see that skip = "Only relevant on Python 3." is applied only on Py3 but it does not make sense .... for me :)

For test_loadFailure the docstring should be more details... the current docstring is useless for me as it does not add any new information ... and it is just a restatment of test_loadFailure. I would like to see a description of what is the current state and what is the resuled state.

---

In twisted/trial/test/test_reporter.py why not use just except RuntimeError as excValue ?

Why do we need test, stream, publisher, result defiend on SubunitReporterTests?

---

I tried

./bin/trial3 twisted.web.test ./bin/trial3 twisted.python.test.test_constants.ValuesTests ./bin/trial3 twisted.python.test.test_constants.ValuesTests.test_iterconstants

and they work... I have also tried running the test from testModules and they look ok

---

I tried ./bin/trial3 twisted/python/test/ and it does not work... but it works with normal trial

---

Also, if I run ./bin/trial3 twisted/python/test/test_components.py I see the test running but I get this error... maybe it is not related to the changes from this branch... but it pass on py2

[ERROR]
Traceback (most recent call last):
  File "twisted/python/test/test_components.py", line 139, in testInheritanceAdaptation
    c.removeComponent(co1)
  File "/home/adi/chevah/twisted/twisted/python/components.py", line 272, in removeComponent
    l.append(reflect.namedObject(k))
  File "/home/adi/chevah/twisted/twisted/python/reflect.py", line 168, in namedObject
    module = namedModule('.'.join(classSplit[:-1]))
  File "/home/adi/chevah/twisted/twisted/python/reflect.py", line 158, in namedModule
    m = getattr(m, p)
builtins.AttributeError: 'module' object has no attribute 'test'

twisted.python.test.test_components.ComponentizedTests.testInheritanceAdaptation

If I try ./build/py3/bin/python3 ./admin/run-python3-tests twisted.python.test.test_components it works and all test pass

---

Please check my comments and resubmit for review.

Thanks!

[hawkowl]

(In [45260]) Branching to trialrunner-py3-5965-8.

[hawkowl]

Hi Adi, thanks for the review.

• trial3 has some minor differences, because exc_clear is not a thing on py3.
• admin/run-python3-tests has also been updated to run only the dist3 tests.
• I've removed the stop changes -- that's related to something different
• The filenameToModule changes are covered by existing test cases -- there's a strange thing Python 3 does where it makes modules without __init__s (see ​https://www.python.org/dev/peps/pep-0420/ for details)
• twisted/trial/test/test_loader.py has had the comment fixed
• Fixed up the loadFailure docstring. It's more or less a copy of loadFailingMethod, but one that can work on Py3.
• twisted/trial/test/test_reporter.py's redefining of excValue is because on Python 3, it is correctly scoped, and e stops existing afterwards, so we need to explicitly export the value.
• Not sure why SubunitReporterTests does that. Might need refactoring later.
• ./bin/trial3 twisted/python/test/ not working has been fixed
• Tried that locally and it passes. Strange.

I'm liking where this branch is getting to, so putting it up for review again. Builders are looking happy (the "9 errors" are due to the INotify tests failing, see https://twistedmatrix.com/trac/ticket/7968 ), coverage is looking acceptable. There's a lot of junk in the py3.4 coverage report, I should have that fixed very soon (by the time someone reviews it), but just scroll down to get the important bits.

[adiroiban]

Thanks for the update!

I don't understand this documentation:

On Python 3, 'findByName' returns TestCases, not objects

---

trial3 has some minor differences, because exc_clear is not a thing on py3.

In this case think that we need a comment inside trial3 to explain why we need this.

I think that that we should have as single bin/trial script and it should use #!/usr/bin/env python, not force bin/trial to python2... if you think that we need a trial force on python2 maybe we should have a bin/trial2 script.

---

I see that I can now run trial with a diretory path. Thanks!

---

In twisted/trial/test/packages.py i was expecting to see a comment describing why do we need to invalidate the cache on Py3.. and why not on Py2

---

It is great to see trial used for running the py3 tests.

I see this error here ​https://buildbot.twistedmatrix.com/builders/debian7-py3.4/builds/294/steps/shell/logs/stdio

Exception ignored in: <generator object _runSequentially at 0x7f4bc3953c18>
Traceback (most recent call last):
  File "/buildslave/debian7-python-3.4-tests/Twisted/twisted/trial/util.py", line 280, in _runSequentially
    defer.returnValue(results)
  File "/buildslave/debian7-python-3.4-tests/Twisted/twisted/internet/defer.py", line 1105, in returnValue
    raise _DefGen_Return(val)
twisted.internet.defer._DefGen_Return: [(True, None), (False, <<class 'twisted.python.failure.Failure'> <class 'GeneratorExit'>>)

I don't see it in the fedora builder - ​https://buildbot.twistedmatrix.com/builders/fedora19-py3.3/builds/3290

---

Why have you removed the last line from twisted/trial/test/test_pyunitcompat.py

---

Thanks for the explanation regarding excValue = e. I think that this needs a comment to explain why this "trick" is required.

---

The current branch looks good... looking forward for the cleanup you were talking about at the end of your previous comment.

From my point of view we can also merge this now and do the cleanup in a separate branch. This branch is already big and we can start using trial on py3 and if we found other problems, fix them in dedicated tickets.

Thanks!

Thanks!

[hawkowl]

(In [45302]) Branching to trialrunner-py3-5965-9.

[hawkowl]

So, I've:

• Removed trial3, it just uses a standard trial now.
• Fixed the skip comment, added more out-of-message comments as for why it's there.
• Added some more comments
• Fixed the 'ignored' exception, it wasn't happening on Fedora because it was a Python 3.4+ thing. That's fixed now.

As for why I removed that line, it's just my editor stripping useless whitespace on save.

I've spun the builders, I think I'll put it up for one more cycle.

[adiroiban]

Looks good. Thanks!

Please add a comment about why we need to catch AttributError raised by sys.exc_clear()

Feel free to address the comment and merge.

Thanks!

[hawkowl]

(In [45320]) Merge trialrunner-py3-5965-9: Port trial to Python 3

Author: hawkowl Reviewers: adiroiban Fixes: #5965