Opened 3 years ago

Last modified 8 minutes ago

#5965 enhancement new

Port twisted.trial.runner to Python 3

Reported by: exarkun Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: trial Keywords:
Cc: jml, kmike, jon@…, adi@… Branch: branches/trialrunner-py3-5965-9
(diff, github, buildbot, log)
Author: hawkowl Launchpad Bug:

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 realcr 19 months ago.
Python3 porting fixes
twisted_383e25.patch (23.3 KB) - added by realcr 19 months ago.
Some porting to Python3. This time the right order :)
runner_port3.patch (1.8 KB) - added by realcr 19 months ago.
minor changes to runner.py for python3 porting.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jml added

comment:2 Changed 3 years ago by thijs

  • Milestone changed from Python-3.x to Python 3.3 Minimal

comment:3 Changed 3 years ago by itamar

  • Milestone changed from Python 3.3 Minimal to Python-3.x

Not going to happen in minimal port.

Changed 19 months ago by realcr

Python3 porting fixes

comment:4 Changed 19 months ago by realcr

  • Author set to 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 19 months ago by realcr

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

comment:5 Changed 19 months ago by itamar

  • 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 19 months ago by realcr

  • 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 19 months ago by realcr

minor changes to runner.py for python3 porting.

comment:7 Changed 18 months 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 18 months ago by exarkun

Hi kmike,

Your comment is incredibly helpful. Thank you very much!

comment:9 Changed 17 months ago by multani

  • Cc jon@… 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 17 months ago by multani

  • Keywords review removed

comment:11 Changed 16 months ago by cpdean

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

comment:12 Changed 16 months ago by cpdean

  • Owner set to cpdean

comment:13 Changed 5 months ago by hawkowl

  • Author changed from realcr to hawkowl, realcr
  • Branch set to branches/trialrunner-py3-5965

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

comment:14 Changed 2 months ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965 to branches/trialrunner-py3-5965-2

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

comment:15 Changed 2 months ago by 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!

comment:16 Changed 2 months ago by adiroiban

  • Cc adi@… added

comment:17 Changed 7 weeks ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-2 to branches/trialrunner-py3-5965-3

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

comment:18 Changed 4 weeks ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-3 to branches/trialrunner-py3-5965-4

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

comment:19 Changed 4 weeks ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-4 to branches/trialrunner-py3-5965-5

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

comment:20 Changed 4 weeks ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-5 to branches/trialrunner-py3-5965-6

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

comment:21 Changed 3 weeks ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-6 to branches/trialrunner-py3-5965-7

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

comment:22 Changed 3 weeks ago by hawkowl

  • Author changed from hawkowl, realcr to hawkowl
  • Keywords review added; patch python3 removed
  • Owner cpdean 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 3 weeks ago by adiroiban

  • 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 12 days ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-7 to branches/trialrunner-py3-5965-8

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

comment:25 Changed 11 days 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 11 days ago by hawkowl

  • Owner hawkowl deleted

comment:27 Changed 7 days ago by adiroiban

  • 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 7 days ago by hawkowl

  • Branch changed from branches/trialrunner-py3-5965-8 to branches/trialrunner-py3-5965-9

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

comment:29 Changed 3 days 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 8 minutes ago by adiroiban

  • 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!

Note: See TracTickets for help on using tickets.