Opened 4 years ago

Closed 16 months ago

#5965 enhancement closed fixed (fixed)

Port twisted.trial.runner to Python 3

Reported by: Jean-Paul Calderone Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: trial Keywords:
Cc: Jonathan Lange, kmike, Jonathan Ballet, Adi Roiban Branch: branches/trialrunner-py3-5965-9
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description

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.

Attachments (3)

python3_porting1.patch (23.3 KB) - added by real 3 years ago.
Python3 porting fixes
twisted_383e25.patch (23.3 KB) - added by real 3 years ago.
Some porting to Python3. This time the right order :)
runner_port3.patch (1.8 KB) - added by real 3 years ago.
minor changes to runner.py for python3 porting.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 4 years ago by Thijs Triemstra

Milestone: Python-3.xPython 3.3 Minimal

comment:3 Changed 4 years ago by Itamar Turner-Trauring

Milestone: Python 3.3 MinimalPython-3.x

Not going to happen in minimal port.

Changed 3 years ago by real

Attachment: python3_porting1.patch added

Python3 porting fixes

comment:4 Changed 3 years ago by real

Author: realcr
Keywords: review patch added

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.

Changed 3 years ago by real

Attachment: twisted_383e25.patch added

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

comment:5 Changed 3 years ago by Itamar Turner-Trauring

Keywords: review removed

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).

comment:6 Changed 3 years ago by real

Keywords: python3 review added

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

Changed 3 years ago by real

Attachment: runner_port3.patch added

minor changes to runner.py for python3 porting.

comment:7 Changed 3 years ago by kmike

Cc: kmike added

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.

comment:8 Changed 3 years ago by Jean-Paul Calderone

Hi kmike,

Your comment is incredibly helpful. Thank you very much!

comment:9 Changed 3 years ago by Jonathan Ballet

Cc: Jonathan Ballet added

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.

comment:10 Changed 3 years ago by Jonathan Ballet

Keywords: review removed

comment:11 Changed 3 years ago by Conrad Dean

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

comment:12 Changed 3 years ago by Conrad Dean

Owner: set to Conrad Dean

comment:13 Changed 21 months ago by hawkowl

Author: realcrhawkowl, realcr
Branch: branches/trialrunner-py3-5965

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

comment:14 Changed 19 months ago by hawkowl

Branch: branches/trialrunner-py3-5965branches/trialrunner-py3-5965-2

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

comment:15 Changed 18 months ago by Adi Roiban

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!

comment:16 Changed 18 months ago by Adi Roiban

Cc: Adi Roiban added

comment:17 Changed 18 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-2branches/trialrunner-py3-5965-3

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

comment:18 Changed 18 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-3branches/trialrunner-py3-5965-4

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

comment:19 Changed 17 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-4branches/trialrunner-py3-5965-5

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

comment:20 Changed 17 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-5branches/trialrunner-py3-5965-6

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

comment:21 Changed 17 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-6branches/trialrunner-py3-5965-7

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

comment:22 Changed 17 months ago by hawkowl

Author: hawkowl, realcrhawkowl
Keywords: review added; patch python3 removed
Owner: Conrad Dean deleted

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.

comment:23 Changed 17 months ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

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!

comment:24 Changed 17 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-7branches/trialrunner-py3-5965-8

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

comment:25 Changed 17 months ago by hawkowl

Keywords: review added

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.

comment:26 Changed 17 months ago by hawkowl

Owner: hawkowl deleted

comment:27 Changed 17 months ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

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!

comment:28 Changed 17 months ago by hawkowl

Branch: branches/trialrunner-py3-5965-8branches/trialrunner-py3-5965-9

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

comment:29 Changed 17 months ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

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.

comment:30 Changed 16 months ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

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!

comment:31 Changed 16 months ago by hawkowl

Resolution: fixed
Status: newclosed

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

Author: hawkowl Reviewers: adiroiban Fixes: #5965

Note: See TracTickets for help on using tickets.