Make "bin/trial --version" runnable in Python3

[vperic]

As a first step to allowing Trial to run in Python 3, it should be possible to ask it about the version of Twisted currently used. This will assure that the relevant files (mostly in twisted.python) at least have Python3-compatible syntax.

[vperic]

(In [34938]) Create branch trial-version-5802

[thijs]

I'd like to point out that #5785 and #4244 are also relevant to this ticket (and currently up for review).

[itamar]

Some initial thoughts:

1. If you copy code from six, you need to conform to their license.
2. You should have a test for the reraise function.
3. Having 5 lines of code we copy/paste each time we want to import StringIO seems excessive. Maybe we should be importing twisted.python.compat.StringIO? Or have some utility function for importing such modules, if there's many of them and we don't want to force all of them to be imported each time.

[vperic]

1, 2: I'll check the license and copy their tests as well. 3: Actually, this already exists in a number of places: the C extension is tried and then the default is loaded. The situation is actually simplified in Python 3, because you can use a single line and it will try the C extension and automatically fall-back to the Python code (which is what we are currently doing everywhere), but it has to look like that so that we can support both 2 and 3 with the same code (well, as far as I can see, perhaps there's a simpler way). One solution is to put it in t.p.compat; it's used quite often.

[vperic]

Ok, I've basically got this working (I took some "shortcuts" so that should be cleaned up before submitting the code, but it works). The final patch is around 500 lines, so I'll probably split it into a few, but it's mostly simple syntax changes. However, I did get a few test failures which I can't explain, so I'd appreciate it if someone else could take a look:

twisted.test.test_failure.FailureTestCase.testLackOfTB
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/vperic/devel/Twisted/twisted/test/test_failure.py", line 460, in test_failureConstructionFindsOriginalFailure
    self.assertEqual(f.getTraceback(), newF.getTraceback())
  File "/home/vperic/devel/Twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = 'Traceback (most recent call last):\n  File "/home/vperic/devel/Twisted/twisted/trial/unittest.py", line 719, in _run\n    self.getSuppress(), method)\n  File "/home/vperic/devel/Twisted/twisted/internet/defer.py", line 134, in maybeDeferred\n    result = f(*args, **kw)\n  File "/home/vperic/devel/Twisted/twisted/internet/utils.py", line 191, in runWithWarningsSuppressed\n    result = f(*a, **kw)\n  File "/home/vperic/devel/Twisted/twisted/test/test_failure.py", line 454, in test_failureConstructionFindsOriginalFailure\n    f = getDivisionFailure()\n--- <exception caught here> ---\n  File "/home/vperic/devel/Twisted/twisted/test/test_failure.py", line 33, in getDivisionFailure\n    1/0\nexceptions.ZeroDivisionError: division by zero\n'
b = 'Traceback (most recent call last):\n  File "/home/vperic/devel/Twisted/twisted/trial/unittest.py", line 750, in deferTestMethod\n    d = self._run(self._testMethodName, result)\n  File "/home/vperic/devel/Twisted/twisted/trial/unittest.py", line 719, in _run\n    self.getSuppress(), method)\n  File "/home/vperic/devel/Twisted/twisted/internet/defer.py", line 134, in maybeDeferred\n    result = f(*args, **kw)\n  File "/home/vperic/devel/Twisted/twisted/internet/utils.py", line 191, in runWithWarningsSuppressed\n    result = f(*a, **kw)\n--- <exception caught here> ---\n  File "/home/vperic/devel/Twisted/twisted/test/test_failure.py", line 457, in test_failureConstructionFindsOriginalFailure\n    f.raiseException()\n  File "/home/vperic/devel/Twisted/twisted/python/failure.py", line 379, in raiseException\n    reraise(self.type, self.value, self.tb)\n  File "<string>", line 2, in reraise\n    \nexceptions.ZeroDivisionError: division by zero\n'


twisted.test.test_failure.FindFailureTests.test_failureConstructionFindsOriginalFailure
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/vperic/devel/Twisted/twisted/test/test_failure.py", line 442, in test_findFailure
    self.assertEqual(failure.Failure._findFailure(), f)
  File "/home/vperic/devel/Twisted/twisted/trial/unittest.py", line 270, in assertEqual
    % (msg, pformat(first), pformat(second)))
twisted.trial.unittest.FailTest: not equal:
a = None
b = <twisted.python.failure.Failure <type 'exceptions.ZeroDivisionError'>>

The problem is the "reraise" line in failure.py. I copied this code from six, and it works fine in most cases (it's called quite often actually), but fails in these two. This is kinda beyond me, so I'd appreciate any help. Is it possible that the extra function call messes up with the traceback somehow? The reraise function is necessary because the syntax for the raise statement changed in incompatible ways between Py2 and 3.

[itamar]

I assume something about having an extra function confuses _findFailure? Not the simplest code, but perhaps some pdb debugging would point you in the right direction.

[itamar]

OK, I figured out the issue in _findFailure: it expects the last frame of re-raised exceptions to be Failure.raiseException, but it's actually twisted.python.compat.reraise since the addition of an extra function call has extended the stack. I would suggest you recreate the logic for compat.reraise in the definition of raiseException, i.e. define different versions for Python 2 and Python 3 and don't use the compat function. You could also modify _findFailure, but it's terrible enough as it is.

[tom.prince]

While this is a bug with twisted's python 3 support, it isn't directly actionable. To be precise, any patch that fixes this directly is likely to big to pass review.

From #twisted-dev:

<exarkun> Should http://twistedmatrix.com/trac/ticket/5802 even still be open?
<exarkun> It seems incompatible with a test-driven development approach.
<tomprince> Well, it probably wants to be handled as multiple tickets, but it is a bug.
<tomprince> trac doesn't seem to support dependencies.
<exarkun> What do you mean "it is a bug"?
<tomprince> Unless it is clear what steps to take, and there are tickets open for them I don't think that should be closed.
<tomprince> Does 'bin/trial --version' work on python 3?
<exarkun> No.
<exarkun> Are you highlighting the distinction between "enhancement" and "defect"?
<exarkun> Or just observing that it's a ticket that exists in the issue tracker?
<tomprince> I'm saying that it represents a isssue with our python3 support that needs to be fixed. (For an interpretation of "needs" that includes supporting python3)
<exarkun> Okay, that may be true.  But just because it was filed doesn't mean it's a good ticket.  The way work is divided up has a big impact on how easily it can be done.  The particular way that ticket implies work should be divided up is probably not likely to lead to a good outcome, and it's basically directly contrary to the documented plan.
<exarkun> We can close the ticket without saying `bin/trial --version´ is never going to be supported on Python 3.
<exarkun> The first step to making trial work on Python 3 has already been taken, in fact - and it wasn't to make the command line script support a random command line argument, it was to start porting the underlying library that supports the command line tool.
<exarkun> (Arguably that wasn't the first step either, because it came after porting the other dependencies of that underlying library)_
<therve> As described, the ticket is a development step, not a defect
<therve> So it should be closed if we don't consider it a good step
<tomprince> Is not a good step, or not a good first step?
<tomprince> In any case, should it be closed, or it be updated to be clearer what should be done regarding it.
<exarkun> Running `bin/trial --version´ seems like a thing you might want to do in order to find out what the next development step is
<exarkun> Although you might also just look at the trial test suite and port something that hasn't been ported yet
<exarkun> If you run out of test modules and `bin/trial --version´ still doesn't work then maybe you're on to something
<tomprince> Being able to run 'bin/trial --version' is a nice visible milestone, for those not yet confortable with tdd.
<exarkun> Unfortunately those are the people most likely to do a bad job of porting while trying to resolve that ticket
<exarkun> It's an incoherent set of changes for a single patch, it doesn't respect the test suite, and it encourages someone to modify untested code
<exarkun> A better visible milestone would be "make unit test x.y.z pass"
<exarkun> So I propose closing #5802 and filing a ticket for that, whatever the next sensible x.y.z is