Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5874 enhancement closed fixed (fixed)

split twisted.trial.unittest into two compilation units, one for basic synchronous features, one for reactor features

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone: Python 3.3 Minimal
Component: trial Keywords:
Cc: jml Branch: branches/py3-sync-testcase-5874-4
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description (last modified by washort)

To port a module to Python 3, the entire module must be converted to Python 3 syntax, as well as all modules that module imports (transitively).

To ease porting of trial, split twisted.trial.unittest into two modules, one of which contains the immediately needed, easier to port pieces (SynchronousTestCase) and the other of which contains all of the reactor-related functionality (TestCase).

SynchronousTestCase will go into twisted.trial._synctest and TestCase will go into twisted.trial._asynctest. twisted.trial.unittest will import the necessary public names for re-export, but it will not import _asynctest.TestCase on Python 3 until _asynctest and its dependencies are ported.

Change History (22)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jml added

comment:2 Changed 2 years ago by exarkun

  • Status changed from new to assigned

comment:3 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/py3-sync-testcase-5874

(In [35353]) Branching to 'py3-sync-testcase-5874'

comment:4 Changed 2 years ago by exarkun

(In [35355]) Delete the asynchronous stuff from _synctest.py and delete whatever was left in _synctest.py from _asynctest.py

Few code changes are actually made, this is largely straight deletion. The exceptions:

  1. unittest.py is re-created and made to import and re-export all the necessary names
  2. Some objects have their module clobbered to avoid upsetting tests that depend on the exact FQPN
  3. The reporter is adjusted to know about the new filenames.
  4. A use of __import__ is replaced with a use of import (thanks to __future__.absolute_import)
  5. necessary imports are added to _asynctest.py from _synctest.py to satisfy (previously in-compilation-unit) dependencies

refs #5874

comment:5 Changed 2 years ago by exarkun

(In [35357]) Unduplicate assertion assertions

Just deleted code, no interesting changes.

refs #5874

comment:6 Changed 2 years ago by exarkun

  • Branch changed from branches/py3-sync-testcase-5874 to branches/py3-sync-testcase-5874-2

(In [35375]) Branching to 'py3-sync-testcase-5874-2'

comment:7 Changed 2 years ago by exarkun

  • Branch changed from branches/py3-sync-testcase-5874-2 to branches/py3-sync-testcase-5874-3

(In [35460]) Branching to 'py3-sync-testcase-5874-3'

comment:8 Changed 2 years ago by exarkun

(In [35468]) Move acquireAttribute into _utilpy3 and port it to python 3. Also deprecate getPythonContainers because it is dumb and useless.

refs #5874

comment:9 Changed 2 years ago by exarkun

(In [35469]) Stop trying to use getPythonContainers. Also fiddle a few imports temporarily.

refs #5874

comment:10 Changed 2 years ago by exarkun

  • Branch changed from branches/py3-sync-testcase-5874-3 to branches/py3-sync-testcase-5874-4

(In [35550]) Branching to 'py3-sync-testcase-5874-4'

comment:11 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new

A map to these changes:

  1. twisted/trial/test/test_tests.py - moved suppression tests into new test_suppression.py with some changes:
    1. Changed those tests to use stdlib TestResults object instead of trial reporter. ie, twisted.trial.reporter.TestResults became unittest.TestResults. (tests are not about reporting logic, shouldn't make any difference)
    2. Changed those tests to construct stdlib test suite from desired tests directly. ie, use of twisted.trial.runner.TestLoader was removed and replaced by explicit unittest.TestSuite instantiation. (tests are not about test discovery/loading, should not make a difference)
  2. twisted/trial/test/suppression.py - async versions not defined on Python 3
  3. twisted/trial/test/test_warnings.py
    1. Switched to from twisted.trial.reporter.TestResult to unittest.TestResult (tests not about results reporting, shouldn't matter). In practice, this requires
    2. Switched from asserting that CustomWarning is raised and logged to the TestResult object to doing a string comparison, since unittest.TestResult keeps strings instead of objects.
  4. twisted/trial/unittest.py split into twisted/trial/_synctest.py and twisted/trial/_asynctest.py
    1. Some minor changes to _synctest.py to make it Python 3 compatible
      1. list(d.values()), switched to sync runWithWarningsSuppressed, and other similar changes.
      2. __eq__/__ne__/__hash__ changed a bit on SynchronousTestCase due to new Python 3 rules and Python 2.5 no longer needing support
      3. _parents defined more simply to avoid having to port/fix/test the stupid getPythonContainers function. Should always have the same value as it did before.
  5. twisted/trial/util.py
    1. some methods split off into _utilpy3.py, no changes made to moved functions
    2. getPythonContainers deprecated
  6. twisted/trial/reporter.py - _trimFrames changed again
  7. twisted/trial/test/test_assertions.py
    1. Some regular Python 3 porting changes
    2. Some tests moved unchanged off into test_asyncassertions.py
  8. twisted/trial/test/test_warning.py - normal porting changes

Collectively, this makes SynchronousTestCase usable on Python 3, though it may not actually have full test coverage. For example, I didn't port twisted/trial/test/test_log.py so there is no test coverage for how SynchronousTestCase interacts with logging on Python 3. I see a few options for this:

  1. As part of this ticket, port all the remaining tests which cover SynchronousTestCase functionality
  2. File new tickets for the various bits of untested functionality and resolve them after this one
  3. File new tickets for the various bits of untested functionality and resolve them as they're needed for other work

I'm happy to go with any of these approaches. Feel free to suggest one, or another alternative.

Build results

comment:12 Changed 2 years ago by itamar

  • Status changed from new to assigned
  1. I'm getting a failure when I run the python 3 tests (_most_ of the time):
    ======================================================================
    ERROR: test_renamedSource (twisted.trial.test.test_warning.FlushWarningsTests)
    test_renamedSource
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/itamarst/Devel/Twisted/branches/py3-sync-testcase-5874-4/twisted/trial/_synctest.py", line 1097, in _run
        runWithWarningsSuppressed(suppress, method)
      File "/home/itamarst/Devel/Twisted/branches/py3-sync-testcase-5874-4/twisted/python/_utilpy3.py", line 124, in runWithWarningsSuppressed
        return f(*args, **kwargs)
      File "/home/itamarst/Devel/Twisted/branches/py3-sync-testcase-5874-4/twisted/trial/test/test_warning.py", line 341, in test_renamedSource
        from twisted_renamed_helper import module
    ImportError: No module named 'twisted_renamed_helper'
    
    ----------------------------------------------------------------------
    
  2. I'd say go with "File new tickets for the various bits of untested functionality and resolve them after this one".

More if/when Ada sleeps.

comment:13 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to exarkun
  • Status changed from assigned to new

Thanks for doing this rather massive, unpleasant task. Some points to address:

  1. _asynctest.__all__ includes SynchronousTestCase. Possibly it shouldn't even have __all__, in which case _synctest.py shouldn't either.
  2. Add twisted.trial.itrial and twisted.trial.test to ported list.
  3. Probably missed this in previous ticket, but shouldn't assertFailure be in _asynctest?
  4. mktemp should document it returns a C{str}.
  5. test_warnings encodes mktemp result using ASCII, and in one places decodes. I think it should be UTF-8 (or whatever the magic encoding Python3 uses for paths is?), otherwise we'll break test suite for users who run it in non-ASCII paths.
  6. The deprecated, top-level assertion functions are now missing from twisted.trial.unittest; either make sure they end up there, or just drop them completely from _synctest.py.

Fix the above, and my previous comment re error, then merge.

comment:14 Changed 2 years ago by itamar

Oh, and the build is red.

comment:15 Changed 2 years ago by exarkun

I'm getting a failure when I run the python 3 tests (_most_ of the time):

http://bugs.python.org/issue15912 :/ "Fixed"? in r35578

_asynctest.all includes SynchronousTestCase. Possibly it shouldn't even have all, in which case _synctest.py shouldn't either.

Fixed in r35572

Add twisted.trial.itrial and twisted.trial.test to ported list.

Done in r35573 and r35575

Probably missed this in previous ticket, but shouldn't assertFailure be in _asynctest?

Ugh, indeed. Fixed in r35574

mktemp should document it returns a C{str}.

Yea, although I'm not yet convinced it shouldn't return bytes instead, but we can leave that for after we've collected more use-cases (obviously instead there should be a method that returns a FilePath, sigh). Done in r35577

test_warnings encodes mktemp result using ASCII, and in one places decodes. I think it should be UTF-8 (or whatever the magic encoding Python3 uses for paths is?), otherwise we'll break test suite for users who run it in non-ASCII paths.

There's no slave in this configuration. How will we know if it works? How will we know if it continues to work? Maybe this is an argument for switching mktemp to unicode on Python 3 right now.

The deprecated, top-level assertion functions are now missing from twisted.trial.unittest; either make sure they end up there, or just drop them completely from _synctest.py.

Done in r35576

comment:16 Changed 2 years ago by exarkun

Oh, and the build is red.

Oh, yes. Because twisted.internet.utils.runWithWarningsSuppressed and twisted.python._utilpy3.runWithWarningsSuppressed are not monkey-patch compatible, I think. So trial's internal warning tricks happen to work with the former but not the later. A potentially simple fix would be to make sure names are used the same way in each version, but I haven't spent more than about 10 seconds thinking about this yet.

comment:17 Changed 2 years ago by exarkun

(In [35582]) Switch to SynchronousTestCase for all versions and use flushWarnings instead of a third warning catching mechanism

refs #5874

comment:18 Changed 2 years ago by exarkun

(In [35585]) Apparently Todo is expected to be public

This fixes some link errors in the pydoctor documentation, at least.

refs #5874

comment:19 Changed 2 years ago by exarkun

(In [35586]) Try to fix some more pydoctor link errors

refs #5874

comment:20 Changed 2 years ago by exarkun

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

(In [35588]) Merge py3-sync-testcase-5874-4

Author: exarkun
Reviewer: itamar
Fixes: #5874
Fixes: #5963

Split twisted.trial.unittest into a Python 3 compatible part, currently with support for synchronous
unit tests (ie, those that don't return a Deferred), and a Python 2-only part. The API is unchanged,
but twisted.trial.unittest now imports on Python 3 and provides the Python 3 compatible functionality.

Also remove the long deprecated module-level assertion functions from twisted.trial.unittest.

comment:20 Changed 2 years ago by washort

  • Description modified (diff)
  • Priority changed from normal to highest

this is a test of submitting a ticket comment with frack.

comment:20 Changed 2 years ago by washort

  • Priority changed from highest to normal


Note: See TracTickets for help on using tickets.