Opened 3 years ago

Closed 3 years ago

#7803 enhancement closed fixed (fixed)

Port just enough of trial to run --version on Python 3

Reported by: Adi Roiban Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: trial Keywords:
Cc: Branch: branches/py3-trial-version-7803-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban

Description (last modified by hawkowl)

Porting just enough to run --version lays out some groundwork that lets further changes focus on porting functionality, not just getting it importing at all.

Original desc:

This is done in parallel with #5966 ... for which I see that there was no recent activity and that some proposed changes are already in trunk

My goal is to have an initial version of trial running on Python3 so that it can replace the run-python3-tests and later run its own tests suite and other tests.

I already have a local branch on which trial works on python3 with just a few changes to filepath and plugins registration using @implementer decorator... and it can run the tests executed by run-python3-tests

The plan is to break my local branch into simple tickets and send them for review.

Change History (21)

comment:1 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/py3-trial-version-7803

(In [43986]) Branching to py3-trial-version-7803.

comment:2 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I see that #3843 ask to deprecate twisted.persisted but #6910 and #6911 were created to port them to Python3.

I have delayed the imports for twisted.persisted.sod for runtime... in this as long as python3 apps don't use persisted all should be fine.

I had no idea how to port _preample to Pyhon3 so for now I have created a trial3 script and in setup3.py installed all scripts. Please suggest a way to handle this.

I assume that spewer function is only used for testing and not in production... I check the code in spewer and it looks like it was not designed to be easy to write an automated test for it. I tested it my branch which has a functional trial on py3 and it works.

I would like to have trial working on Python3 so that on my local computer I can just run this even if trial is not 100% ported... but it can be used to port trial itself.

tox -e py26,py27,py33 twisted/application/test/test_internet.py

Please take a look at the changes and let me know what do you think.

Thanks!

comment:3 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to Adi Roiban

Hi Adi!

Few things:

  • trial3 should have "#!/usr/bin/env python3", not "env python", as that's python2
  • The comments about sob are a bit wonky, should be:

# twisted.persisted is proposed for deprecation and is not yet ported to # to Python3 so we only import it if some code really needs to use it

  • We don't want to install all scripts yet. bin/trial3 is not installed, but runs from the bin/ directory for developers
  • Preamble doesn't need to be ported, it works fine on Py2 and Py3 (it seems)

Thanks! :)

comment:4 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review!

I have removed trial3 as trial2 also works on Python3... I was confused by the _preamble import and this is why I have created trial3 script.


I have updated the comments for twisted.persisted


I have reverted the changes in dist3.py


I have added a test for changes from t.application.services... please check test is ok. I don't know what docstring to add at that test without duplicating the docstring from the main code.

I have created ticket #7816 to look at opportunistic port of application.services


Please check latest changes

with these changes I can do (build/py33/ is my py33 tox environment)

$ ./build/py33/bin/python3 bin/trial --version
Twisted version: 15.0.0

sending changes to buildbot...

don't know what test to run on buildbot for these changes. hope that soon we can have trial ported on py3 and integrate it with buildbots

comment:5 Changed 3 years ago by hawkowl

Thanks Adi.

They look okay to me on first glance, but I'd prefer a second opinion (maybe Glyph's) to make sure that we're on the right track here -- as this isn't adding anything Trial to the distribution (which is what the policy says to do), but it's getting "bootstrappable" so that Trial can be used to actually test Trial.

I like this path, but, again, I defer.

comment:6 Changed 3 years ago by Adi Roiban

Branch: branches/py3-trial-version-7803branches/py3-trial-version-7803-2

(In [44318]) Branching to py3-trial-version-7803-2.

comment:7 Changed 3 years ago by Adi Roiban

I have created new branch to fix conflicts and sent it to buildbot. Please review the latest diff. Thanks!

comment:8 Changed 3 years ago by Dustin J. Mitchell

I like this approach, too, not least because trial is a Buildbot requirement and it will be hard for us to tell whether Buildbot works on Python3 without Trial.

However, on the py3-trial-version-7803-2 branch (https://github.com/twisted/twisted/compare/py3-trial-version-7803-2), I'm running python3 bin/trial twisted.application.test.test_service (or for any test script) and seeing

  File "bin/trial", line 18, in <module>
    run()
  File "/home/dustin/code/twisted/t/twisted/twisted/scripts/trial.py", line 602, in run
    config.parseOptions()
  File "/home/dustin/code/twisted/t/twisted/twisted/python/usage.py", line 277, in parseOptions
    self.postOptions()
  File "/home/dustin/code/twisted/t/twisted/twisted/scripts/trial.py", line 473, in postOptions
    _BasicOptions.postOptions(self)
  File "/home/dustin/code/twisted/t/twisted/twisted/scripts/trial.py", line 383, in postOptions
    self['reporter'] = self._loadReporterByName(self['reporter'])
  File "/home/dustin/code/twisted/t/twisted/twisted/scripts/trial.py", line 370, in _loadReporterByName
    for p in plugin.getPlugins(itrial.IReporter):
  File "/home/dustin/code/twisted/t/twisted/twisted/plugin.py", line 209, in getPlugins
    allDropins = getCache(package)
  File "/home/dustin/code/twisted/t/twisted/twisted/plugin.py", line 143, in getCache
    for plugmod in mod.iterModules():
  File "/home/dustin/code/twisted/t/twisted/twisted/python/modules.py", line 134, in iterModules
    children.sort()
AttributeError: 'map' object has no attribute 'sort'

which I realize is a known issue (children now returns a map, not a list), but I'm not sure how this is causing me issues on this branch but hasn't bothered either of you.

comment:9 Changed 3 years ago by hawkowl

Author: adiroibanhawkowl, adiroiban
Branch: branches/py3-trial-version-7803-2branches/py3-trial-version-7803-3

(In [44679]) Branching to py3-trial-version-7803-3.

comment:10 Changed 3 years ago by hawkowl

Hi dustin, this is just a port for --version, to get the very basic part working, and then porting the actual trial bits :)

comment:11 Changed 3 years ago by hawkowl

I've merged it forward and instead of altering spewer (which doesn't work on py3 with the changes made in this branch), I just make it so that spewer doesn't have to be imported if it's not being used, which sidesteps this a little.

Builders spun.

comment:12 Changed 3 years ago by adiroiban1

Thanks for working on this :)

What is the purpose of this comments ?

# Won't work on Python 3, because util.spewer is not ported.

with the current code, spew will fail at runtime so I don't think that we need to comments.

I remember testing spewer on py3 and it was flooding my console. I think that instead of leaving those comments we should create a separate ticket to port spewer and let people know that spew option does not work in python. The ticket can explain why spewer don't work.

Maybe the comment can be part of opt_spew docstring so that users will see that it is not yet working on py3.

Even if spewer is not yet working on Py3 maybe we can leave the change in utils so that the code can be compiled by py3 and we don't need to change the imports in trial.py


Also I think that we should create a bug report from Dustin's comment and have it as a blocker for porting trial on py3

Cheers!

comment:13 Changed 3 years ago by Glyph

Hi hawkowl,

Thanks for working on this.

  1. The first thing I think that needs addressing here is the use of the word "opportunistic". The dictionary definition I have is "exploiting chances offered by immediate circumstances without reference to a general plan or moral principle", which seems like not a great way to make changes :-). I think this is my fault, because I might have suggested it when the ticket about twisted.persisted.styles was filed to mean something like "incomplete port that allows a module to at least import". Maybe we should just say "incomplete" or "partial" if that's all it means? Maybe a better description would be "port just enough of trial to run the --version command line?"
  2. As adi rightly pointed out, the comments about un-ported code are not helpful; lots of code isn't ported and it isn't specifically called out in comments. The links to specific tickets are helpful, so if you wanted to replace the general "won't work on py3" comments with links, that would also be fine. We should just go ahead and port the code in further tickets and track what is and isn't ported by reference to tests, and start paying attention to coverage reports. This does imply some additional work - once we have trial ported we should really get a py3 coverage builder going. Can you file a ticket for that in an appropriate place? (Somewhere on twisted-infra probably?)
  3. In test_service, the additional if not _PY3 assertion seems gross to me. Specifically, I don't think we should have tests which have different meanings on py3 and py2; if we have some behavior that isn't yet ported, we should have a separate test in a separate module that is specifically not marked as ported yet so that it will be caught by the normal process. By leaving an assertion in a test like this, following the normal process, we may end up with a bunch of edge-cases that are not covered on py3. I think that if the tests were split into py2 and py3 modules that covered both halves of this then the similarly-problematic conditional in the implementation would be adequately covered and someone would quickly notice it needed to be ported.
  4. Speaking of test_service, "sut" is not a very meaningful variable name. I am guessing you mean "system-under-test" but in this case it's actually the unit under test, the system under test would be the whole module or maybe even Twisted as a whole :). So let's go with a more meaningful name like "app" or somesuch.

Speaking of that persisted ticket I never finished, I guess I should just finish that so that fewer things should have weird edge cases sticking out that need stuff like that if _PY3 conditional. So again, my fault :).

Please address these points as best you can and submit for re-review. I would say to just land but I think we need to be really careful about the test coverage discipline we're using for saying py3 is "supported".

comment:14 Changed 3 years ago by Glyph

Keywords: review removed

comment:15 Changed 3 years ago by Glyph

Owner: set to hawkowl

comment:16 Changed 3 years ago by hawkowl

Component: coretrial
Description: modified (diff)
Summary: Opportunistic port of trial --version to Python 3Port just enough of trial to run --version on Python 3

comment:17 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Fixed up a few things according to the comments. Builders spun, look green so far, please review.

comment:18 Changed 3 years ago by adiroiban1

Changes look good!

I think that comments like this should stay and be updated to the new style guide

https://github.com/twisted/twisted/blob/trunk/docs/core/development/policy/coding-standard.rst#comments

# TODO https://twistedmatrix.com/trac/ticket/6910

I wrote the code before the FIXME convention was accepted in trunk.

I think that all references to tickets should use the same standard like

# FIXME: https://twistedmatrix.com/trac/ticket/6910

Thanks!

Last edited 3 years ago by adiroiban1 (previous) (diff)

comment:19 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Regarding comment 8 , this should be already fixed by #7804


Please update the FIXME comments and merge. Thanks!

Looking forward for running trial under py3 :)


Just a minor comment

I like sut as it can also stand for stuff under test :)

I am using this as a standard way to highlight the object which is tested for unit/system/integration/black-back/functional tests. From the context of the test case it should be easier to see that the sut is a protocol/factory/application/logger/observer/http client/http server ...etc

The idea is that in a test you might have many factories, protocols, clients, servers and other object and when scanning old cold is hard to see if the f variable is a dependent factory or the factory under test.

comment:20 Changed 3 years ago by Adi Roiban

FIXME are ok. Sorry for the previous comment.

Please merge! Thanks!

comment:21 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44885]) Merge py3-trial-version-7803-3: Port just enough of trial to run --version on Python 3

Author: adiroiban, hawkowl Reviewers: hawkowl, glyph, adiroiban Fixes: #7803

Note: See TracTickets for help on using tickets.