Opened 5 years ago

Closed 4 years ago

#6009 enhancement closed fixed (fixed)

Port twisted.trial._asynctest.TestCase's dependencies in twisted.trial.util to Python 3

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/trialutil-py3-6009
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description

There are a number functions in twisted.trial.util that are used by twisted.trial._asynctest.TestCase; they should run on Python 3.

Change History (16)

comment:1 Changed 5 years ago by itamarst

Author: itamarst
Branch: branches/trialutil-py3-6009

(In [35798]) Branching to 'trialutil-py3-6009'

comment:2 Changed 5 years ago by itamarst

(In [35800]) move _removeSafely tests from test_runner.py into the more suitable test_util.py. Refs #6009

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

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

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

I didn't merge twisted.trial._utilpy3 back in in this ticket, just to make it clearer what's going on. Instead I opened a ticket, #6019.

comment:5 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

Thanks. This is a partial review, ran out of time before finishing:

  1. Failure on doc builder
  2. admin/_twistedpython3.py adds an incorrect element to the ported list, twisted.internet._utilspy3.py
  3. from foo import (bar\nbaz) style of imports is allowed now, fwiw
  4. The change to twisted/trial/test/test_reporter.py seems quite wrong to me. We shouldn't be weakening assertions like this. It's a horrible test, but it tests important things.

comment:6 Changed 4 years ago by itamarst

(In [35815]) Hopefully address review comments 1, 2 and 4. Refs #6009

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

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

I'll keep 3 in mind. New build started with fixes (if all goes well) for the other points: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/trialutil-py3-6009

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring
  1. Why runWithWarningsSuppressed = staticmethod(utils.runWithWarningsSuppressed) in DeferredSuppressedWarningsTests in test_utilspy3.py?
  2. in twisted/trial/utils.py, profiled shouldn't be killed, it should be deleted. Violence metaphors are anti-social.
  3. in test_util.py, test_callablesCalledInOrder ends with a comment that no longer makes any sense: # Because returning created Deferreds makes jml happy.. Also, the line of code following it appears useless.
  4. It would make me very happy if test_stripFlags were just deleted.
  5. Seems like the defer.py changes suggests that porting deferredGenerator or inlineCallbacks is a pre-requisite of this ticket. I suppose it doesn't actually matter though.
  6. twisted/trial/test/packages.py needs __future__ imports. Also a copyright header and a module docstring would not go amiss. For that matter, what does getModules do anyway? :/
  7. Also need future imports:
    1. twisted.internet._utilspy3
    2. twisted.internet.test.test_utilspy3
  8. Seems like twisted.trial._utilpy3 should not be deleted from admin/_twistedpython3.py in this branch.

comment:9 Changed 4 years ago by itamarst

(In [35822]) Address review comments. Refs #6009

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

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone
  1. Added an explanatory comment. staticmethod is there so it doesn't become a ... descriptor? A method requiring self, anyway.
  2. Done.
  3. Removed.
  4. YAGNI indeed.
  5. Right. But getting those tests to pass requires TestCase... so we're stuck with the usual "it almost certainly works" limbo until then.
  6. It should be obvious to anyone what it does, otherwise it would have had an explanatory docstring! I rewrote it to not be insane.
  7. Done.
  8. Done.

Another build: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/trialutil-py3-6009

comment:11 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

Added an explanatory comment. staticmethod is there so it doesn't become a ... descriptor? A method requiring self, anyway.

Sure, but why did you change the test from using utils.runWithWarningsSuppressed to self.runWithWarningsSuppressed in the first place?

Apart from that, looks fine (actually, I didn't re-read the whole diff, I assume when you said "Done" you meant it was done). If there's an extremely good reason for the staticmethod thing, go ahead and merge. Otherwise it seems like the function should just be called from the module in the usual way, and then merge.

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

That class is subclassing the TestCase class for the non-Deferred version of runWithWarningsSuppressed, so that they can share the same tests. Thus a staticmethod.

comment:13 Changed 4 years ago by Jean-Paul Calderone

The staticmethod is on SuppressedWarningsTests which subclasses twisted.trial.unittest.SynchronousTestCase directly, and which appears to have no subclasses. Am I missing something?

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

Remember, there's two runWithWarningsSuppressed. twisted.internet.test.test_utilpy3.DeferredSuppressedWarningsTests subclasses SuppressedWarningTests and overrides runWithWarningsSuppressed to test the other one.

comment:15 Changed 4 years ago by Jean-Paul Calderone

Thanks. Not sure why I couldn't find that subclass before.

comment:16 Changed 4 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [35861]) Merge trialutil-py3-6009.

Author: itamar Review: exarkun Fixes: #6009

Port rest of trial's utilities to Python 3.

Note: See TracTickets for help on using tickets.