Opened 10 years ago
Closed 7 years 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)
Change History (34)
comment:1 Changed 10 years ago by
Cc: | Jonathan Lange added |
---|
comment:2 Changed 10 years ago by
Milestone: | Python-3.x → Python 3.3 Minimal |
---|
comment:3 Changed 10 years ago by
Milestone: | Python 3.3 Minimal → Python-3.x |
---|
comment:4 Changed 8 years ago by
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 8 years ago by
Attachment: | twisted_383e25.patch added |
---|
Some porting to Python3. This time the right order :)
comment:5 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
Attachment: | runner_port3.patch added |
---|
minor changes to runner.py for python3 porting.
comment:7 Changed 8 years ago by
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:9 Changed 8 years ago by
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 8 years ago by
Keywords: | review removed |
---|
comment:11 Changed 8 years ago by
I'm going to start looking at this to port twisted.trial.test.test_reporter #5964
comment:12 Changed 8 years ago by
Owner: | set to Conrad Dean |
---|
comment:13 Changed 7 years ago by
Author: | realcr → hawkowl, realcr |
---|---|
Branch: | → branches/trialrunner-py3-5965 |
(In [44052]) Branching to trialrunner-py3-5965.
comment:14 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965 → branches/trialrunner-py3-5965-2 |
---|
(In [44936]) Branching to trialrunner-py3-5965-2.
comment:15 Changed 7 years ago by
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 7 years ago by
Cc: | Adi Roiban added |
---|
comment:17 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965-2 → branches/trialrunner-py3-5965-3 |
---|
(In [45032]) Branching to trialrunner-py3-5965-3.
comment:18 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965-3 → branches/trialrunner-py3-5965-4 |
---|
(In [45132]) Branching to trialrunner-py3-5965-4.
comment:19 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965-4 → branches/trialrunner-py3-5965-5 |
---|
(In [45155]) Branching to trialrunner-py3-5965-5.
comment:20 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965-5 → branches/trialrunner-py3-5965-6 |
---|
(In [45181]) Branching to trialrunner-py3-5965-6.
comment:21 Changed 7 years ago by
Branch: | branches/trialrunner-py3-5965-6 → branches/trialrunner-py3-5965-7 |
---|
(In [45213]) Branching to trialrunner-py3-5965-7.
comment:22 Changed 7 years ago by
Author: | hawkowl, realcr → hawkowl |
---|---|
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 7 years ago by
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 7 years ago by
Branch: | branches/trialrunner-py3-5965-7 → branches/trialrunner-py3-5965-8 |
---|
(In [45260]) Branching to trialrunner-py3-5965-8.
comment:25 Changed 7 years ago by
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 7 years ago by
Owner: | hawkowl deleted |
---|
comment:27 Changed 7 years ago by
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 years ago by
Branch: | branches/trialrunner-py3-5965-8 → branches/trialrunner-py3-5965-9 |
---|
(In [45302]) Branching to trialrunner-py3-5965-9.
comment:29 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Not going to happen in minimal port.