Opened 2 years ago

Closed 2 years ago

#6009 enhancement closed fixed (fixed)

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

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/trialutil-py3-6009
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

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 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/trialutil-py3-6009

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

comment:2 Changed 2 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 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

comment:4 Changed 2 years ago by itamar

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 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

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 2 years ago by itamarst

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

comment:7 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

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 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  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 2 years ago by itamarst

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

comment:10 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun
  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 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

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 2 years ago by itamar

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 2 years ago by exarkun

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 2 years ago by itamar

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

comment:15 Changed 2 years ago by exarkun

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

comment:16 Changed 2 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(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.