Make reactor unclean warnings reported errors

[jml]

• Have reactor unclean errors be reported as actual errors (using addError or some such)
• Reporters keep count of these errors and report them as a separate error category
• A non-zero reactor unclean error count will not necessarily fail the suite
• buildbot should change colour if there are reactor unclean errors.

[exarkun]

Why shouldn't these eventually cause test suites to fail?

I don't like having an intermediate state between working and failing. If leaving the reactor unclear at the end of a test is problematic (it is), then it should fail the test. If it isn't (but it is), then trial shouldn't say a thing about it.

There are only two possible states: working and broken.

[jml]

Fair enough. The third point was included as an option for a more gentle transition from warning to error.

In that case, it's probably not worth counting 'unclean' errors in a separate category.

[jml]

Ready for review in unclean-errors-2091

[glyph]

Test failures:

===============================================================================
[ERROR]: twisted.conch.test.test_cftp.TestOurServerCmdLineClient.testWildcardPut
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb647b2ec>
===============================================================================
[ERROR]: twisted.conch.test.test_cftp.TestOurServerCmdLineClient.testWildcardPut
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessWriter object at 0xb647bdcc>
===============================================================================
[ERROR]: twisted.conch.test.test_cftp.TestOurServerCmdLineClient.testWildcardPut
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb647b4ac>
===============================================================================
[ERROR]: twisted.conch.test.test_conch.CmdLineClientTestCase.testLocalToRemoteForwarding
Traceback (most recent call last):
Failure: twisted.internet.defer.TimeoutError: <twisted.conch.test.test_conch.CmdLineClientTestCase testMethod=testLocalToRemoteForwarding> (testLocalToRemoteForwarding) still running at 120.0 secs
===============================================================================
[ERROR]: twisted.conch.test.test_conch.CmdLineClientTestCase.testLocalToRemoteForwarding
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1236815412 [0.479525089264s] called=0 cancelled=0 ConchTestForwardingProcess._connect()>
===============================================================================
[ERROR]: twisted.conch.test.test_conch.CmdLineClientTestCase.testRemoteToLocalForwarding
Traceback (most recent call last):
  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/process.py", line 668, in maybeCallProcessEnded
    self.proto.processEnded(failure.Failure(e))
  File "/home/glyph/Projects/Twisted/trunk/twisted/conch/test/test_conch.py", line 89, in processEnded
    self.deferred.callback(None)
  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/defer.py", line 239, in callback
    self._startRunCallbacks(result)
  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/defer.py", line 290, in _startRunCallbacks
    raise AlreadyCalledError
twisted.internet.defer.AlreadyCalledError:
===============================================================================
[ERROR]: twisted.conch.test.test_conch.CmdLineClientTestCase.testRemoteToLocalForwarding
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <<class 'twisted.internet.tcp.Port'> of twisted.conch.test.test_conch.EchoFactory on 56601>
===============================================================================
[ERROR]: twisted.mail.test.test_imap.Timeout.testLongFetchDoesntTimeout
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1266736276 [1799.99948096s] called=0 cancelled=0 SimpleServer.__timedOut()>===============================================================================
[ERROR]: twisted.test.test_ssl.StolenTCPTestCase.testProperlyCloseFiles
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <Protocol #1032 on 0>
===============================================================================
[ERROR]: twisted.test.test_ssl.StolenTCPTestCase.testProperlyCloseFiles
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <<class 'twisted.internet.tcp.TLSConnection'> to ('127.0.0.1', 43565) at b0cab48c>
===============================================================================
[ERROR]: twisted.test.test_ssl.StolenTCPTestCase.testProperlyCloseFiles
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <Protocol #1033 on 0>
===============================================================================
[ERROR]: twisted.test.test_ssl.StolenTCPTestCase.testProperlyCloseFiles
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <Protocol #1031 on 0>
-------------------------------------------------------------------------------
Ran 2992 tests in 409.660s
FAILED (skips=43, expectedFailures=18, errors=12, successes=2928)

This is not to mention the numerous failures that will occur in user code.

Reject: this is a lot of test failures.

I also think that this should transition through an optional period where there is a flag to at least disable dirty-reactor errors; perhaps it shouldn't even be the default, but it should still be there so that users upgrading twisted can verify that it's not an API change that broke their tests, but rather a refinement in what "successful test" means.

[jml]

Granted the failures in the Twisted suite need to be fixed before merging.

However, we have already gone through a transition period. Trial has long been issuing warnings about dirty reactors say "this will be an error soon".

[jml]

glyph, what platform generates those errors? I tried running the conch tests on my powerbook and didn't get any errors.

[dreid]

fix implementations and usage of _getBatchOutput to not use iterate

[jml]

Up for review in unclean-errors-2091-2

The test_cftp stuff deliberately doesn't fix all the tests. I'm trying to get my branches smaller.

The "fix" in imap4.py is maybe a little dodgy. I don't know how to test it (apart from testLongFetchDoesntTimeout).

[jml]

I've also added an option, --unclean-warnings that will turn the errors into warnings.

[exarkun]

• There's a conflict in test_cftp.py. I resolved it locally by keeping both pieces, since that seemed like the correct resolution.
• SFTPTestProcess needs a ton of docstrings.
• I suspect the buffering/parsing in SFTPTestProcess's outReceived is not strictly correct. Data may be delivered in any chunk size, so the clearBuffer call which is added as a callback to onOutReceived may run too early. On the other hand, this code was pretty broken before, it would seem. :( Please create a ticket for rewriting SFTPTestProcess-using code to be correct (and no need to fix it in this branch unless you want to).
• onProcessEnd should be set to None when it's fired, in the same manner onOutReceived is.
• imap4.py change looks okay. This is only a test issue, since the tests respond to the sendPositiveResponse call synchronously, before the timeout can be cleared. Real usage would never have had a problem here, nor will it have a problem with cancelling the timeout before sending the message.
• add a docstring to test_tests.TestCleanup?.testLeftoverSockets and testLeftoverPendingCalls
• maybe at the end of those two methods, a failure.trap() call would make more sense? I dunno though, just an idea.
• add docstrings for TestGetWarnings?' test methods (good job writing those tests, though :)
• add docstrings for test_runner.py's test_unclearnWarningsOffByDefault and test_getsUncleanWarnings.
• add docstrings for util.py's cleanReactorStateBecauseIAmLazy (and maybe rename? although it's on _Janitor so whatever) and postCaseCleanup.
• Some tests fail on selectreactor with dirty reactor errors in this branch for me, still:

  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testProcessAlias
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb28b650c>
  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testProcessAlias
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessWriter object at 0xb28b652c>
  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testProcessAlias
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb28b654c>
  -------------------------------------------------------------------------------
  

  Looks like all tests I wrote, sorry :) I can try to take a look at this, if you like, but I'm not sure when (not tonight, certainly).
• Lots and lots of tests fail with other reactors

[jml]

OK. I've addressed everything expect for the last two points. Maybe we can have one of those trial parties glyph was talking about to fix the dirty reactor errors on the other reactors?

[jml]

Merged forward to unclean-errors-2091-3

[jml]

I've just fixed the rest of the tests in test_cftp.

I think that might fix the errors in the gtk & gtk2 reactors -- I've triggered the buildbot to verify. Other than that, I can't replicate the errors exarkun sees with any of the buildslaves. Note that all the win32 slaves are broken.

[exarkun]

Looking more closely at the test_mail failures, I think there may still be a problem in the reporting code in trial. The three errors are for the same test, and each is reporting a single left over selectable. Shouldn't these selectables all appear in a single error report?

[jml]

Yeah, maybe. On the other hand, why shouldn't they appear in distinct error reports?

[jml]

OK, this has been lying dormant too long.

What I meant was, the three separate error reports is a deliberate feature. Three selectables means three separate errors that require three incremental code changes.

Merged forward to unclean-errors-2091-4, resolving a couple of conflicts on the way. Please review.

[exarkun]

Waiting for some more changes to be committed...

[jml]

Oops.

[exarkun]

Windows Python 2.4 buildslave fails a handful of tests.

The conch changes are very impressive. As unit tests, they still suck (as you noted in a few places), of course, but the cleanups you've made are significant. Good job on this.

[jml]

Fixed the handful of tests. Back for rereview

[jml]

exarkun has done most of the branch. test_conch and test_pb are the only things that have changed since his review.

[jerub]

Eeexcellent.

This branch needs to be merged forward. I've reviewed test_conch and test_pb.

To be accepted for merging, please do the following:

• merge forward.
• test branch on OSX, Win32 and Linux.
• if the branch tests green, please merge.

[jml]

I had to fix some tests in twisted.mail.test.test_mail.ProcessAliasTestCase. The fixes aren't super-hot -- the tests and code should be refactored significantly. However, I think it's enough to get this branch merged and go in to the release.

Thanks.

[jml]

Now in unclean-errors-2091-5

[jerub]

Yay, looks great now. Passes tests on OSX too. Please merge. :)

[jml]

(In [18811]) Reactor unclean warnings in tests are now errors.

• Fixes #2091
• Author: jml & others
• Reviewers: exarkun, jerub

For some time, Trial has been promising to turn "reactor unclean" warnings into errors. This branch does that, fixing many tests in Twisted which generated such warnings.

To help test authors in debugging reactor unclean warnings, Trial now provides an option, --unclean-warnings which emulates the old behaviour. That is, reactor unclean errors will be printed as warnings and will not fail tests.

This branch also tidies up much of the test cleanup code in Trial, and deprecates APIs and exceptions which are no longer used.

[exarkun]

r18812 reverted this merge.

twisted.conch.test.test_conch.UnixClientTestCase?.test_localToRemoteForwarding failed on the debian-py2.3-select with about a dozen errors similar to these:

===============================================================================
[ERROR]: twisted.conch.test.test_conch.UnixClientTestCase.test_localToRemoteForwarding
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.conch.client.unix.SSHUnixServerFactory on '/home/buildbot/.conch-testuser-127.0.0.1-41937'>
===============================================================================
[ERROR]: twisted.conch.test.test_conch.UnixClientTestCase.test_localToRemoteForwarding
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <<class 'twisted.internet.tcp.Port'> of twisted.conch.test.test_conch.EchoFactory on 57444>
===============================================================================

Another error occurred on the debian-py2.4-select slave:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
-------------------------------------------------------------------------------

The debian64-py2.4-select slave had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 1)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall 189779840 [59.9543559551s] called=0 cancelled=1>
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testProcessAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
-------------------------------------------------------------------------------

The suse-py2.5-select slave had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure <class 'twisted.internet.error.ProcessTerminated'>>, 0)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
-------------------------------------------------------------------------------

The poll step on the debian-py2.4-reactors slave had these errors which may or may not be related:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 0)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1266791124 [59.8638839722s] called=0 cancelled=1>
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.test.test_adbapi.SQLiteReflectorTestCase.testReflector
Traceback (most recent call last):
Failure: twisted.internet.defer.TimeoutError: <twisted.test.test_adbapi.SQLiteReflectorTestCase testMethod=testReflector> (testReflector) still running at 30.0 secs
===============================================================================

Along with this one:

===============================================================================
[ERROR]: twisted.test.test_socks.Bind.test_simple
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1343194644 [29.9994950294s] called=0 cancelled=1>
===============================================================================

The epoll step had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 1)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
-------------------------------------------------------------------------------

The gtk step had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 0)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1271266132 [59.9661631584s] called=0 cancelled=1>
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.test.test_pb.NewStyleCachedTestCase.testNewStyleCache
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <Broker #0 on 0>
-------------------------------------------------------------------------------

The gtk2 slave had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 1)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.test.test_pb.NewStyleCachedTestCase.testNewStyleCache
Traceback (most recent call last):
Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <Broker #0 on 0>
-------------------------------------------------------------------------------

Both steps of the quick slave had these errors:

===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.defer.FirstError: FirstError(<twisted.python.failure.Failure twisted.internet.error.ProcessTerminated>, 0)
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1233269908 [59.9196410179s] called=0 cancelled=1>
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
===============================================================================
[ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testCyclicAlias
Traceback (most recent call last):
Failure: twisted.internet.error.ProcessTerminated: A process has ended with a probable error condition: process ended with exit code 1.
-------------------------------------------------------------------------------

[exarkun]

One other thing: the win32-win32er slave had about 250 more errors with this branch merged than without it. win32er may not be a completely supported reactor, but introducing this many new errors to even an unsupported reactor makes me very nervous and I think we should try to avoid doing it.

I suggest splitting this branch into a lot of smaller pieces, most of which fix tests in various parts of Twisted and can be reviewed and merged independently. Once the test suite is really ready for the trial change, we can go ahead with that part of the merge.

[jml]

I have filed tickets #2266, #2267, #2268 and #2269 to cover the tests listed in your revert message.

With respect to the win32er slave, I don't quite understand why 250 more errors makes you nervous. After all, the defects are there anyway. Still, I agree that we should try to avoid introducing that many new test errors.

[glyph]

We shouldn't really block the 2.5 release on this. It's a new feature, and as long as we don't have an official release plan, releases should not be blocked on new features; just on regressions.

[jml]

(In [19320]) Make sure that certain conch tests do not tarnish the reactor state.

• Author: jml, radix
• Reviewer: jerub
• Fixes #2267
• Refs #2091

Clean up a lot of cruft in test_cftp and test_conch. The tests are still pretty bad, but at least their badness is clear.

[jml]

For reference, the next action on this ticket is to fix #2266, #2268 and #2269.

Once that is done, we need to cherry-pick the trial changes out of the #2091 branch, merge the test fixes, and then merge the trial changes.

[radix]

I did some work on this.

I took a new strategy.

The strategy is that the buildbots will be configured to use --unclean-warnings.

I also changed the implementation of how unclean errors are converted to warnings, so that we don't have to break backwards compatibility with existing Reporter factories: reporters are wrapped in a Reporter that converts errors to warnings, instead of relying on arbitrary reporters to do that.

[radix]

btw the branch is unclean-errors-2091-6

[therve]

• pyflakes errors:

  twisted/python/util.py:8: 'IMethod' imported but unsed
  twisted/python/util.py:8: 'IAttribute' imported but unused
  twisted/trial/util.py:24: 'gc' imported but unused
  twisted/trial/util.py:27: redefinition of unused 'failure' from line 25
  twisted/trial/util.py:27: 'log' imported but unused
  twisted/trial/util.py:27: 'threadpool' imported but unused
  twisted/trial/util.py:28: redefinition of unused 'interfaces' from line 26
  twisted/trial/util.py:28: redefinition of unused 'utils' from line 26
  twisted/trial/util.py:43: undefined name 'warnings'
  

• the 'undefined warnings' in FailureError is a bit frightening . Should we keep this class at all? If we keep it, we should add some tests.
• there is an error running the tests with python 2.3:

  [ERROR]: twisted.trial.test.test_reporter.TestUncleanWarningWrapperErrorReporting.testDoctestError
  Traceback (most recent call last):
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/test/test_reporter.py", line 141, in testDoctestError
      output = self.getOutput(suite)
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/test/test_reporter.py", line 102, in getOutput
      result = self.getResult(suite)
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/test/test_reporter.py", line 107, in getResult
      suite.run(self.result)
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/runner.py", line 150, in run
      test(result)
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/runner.py", line 249, in __call__
      return self._test(results)
    File "/usr/local/lib/python2.3/unittest.py", line 236, in __call__
      result.addError(self, self.__exc_info())
    File "/home/th/twisted_dev/twisted/trunk/twisted/trial/reporter.py", line 191, in addError
      if error.check(util.DirtyReactorError):
  exceptions.AttributeError: 'tuple' object has no attribute 'check'
  -------------------------------------------------------------------------------
  

• trailing whitespaces in twisted/python/util.py, twisted/trial/test/test_reporter.py, twisted/trial/test/test_deferred.py
• please use self.assertIsInstance method instead of self.assertTrue(isinstance(...))
• add docstring to test_reporter.TestUncleanWarningWrapperErrorReporting
• there is an assertWarns method on TestCase that achieve most of what getWarnings is trying to do in test_reporter. If that's the case, remove all the tests for getWarnings, if not, explain why.
• there are mention of Twisted 2.5 everywhere, they should probably be replace by 2.6 (also, mention of the 2.4 to 2.5 migration in the man page).
• when a test fails because of more than one cleanups errors, it's counted as different test failures. For example, running on twisted.mail:

  ===============================================================================
  [ERROR]: twisted.mail.test.test_imap.Timeout.testLongFetchDoesntTimeout
  Traceback (most recent call last):
  Failure: twisted.trial.util.PendingTimedCallsError: pendingTimedCalls still pending (consider setting twisted.internet.base.DelayedCall.debug = True): <DelayedCall -1219323124 [1799.99904203s] called=0 cancelled=1>
  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessWriter object at 0xb731124c>
  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb731144c>
  ===============================================================================
  [ERROR]: twisted.mail.test.test_mail.ProcessAliasTestCase.testAliasResolution
  Traceback (most recent call last):
  Failure: twisted.trial.util.DirtyReactorError: reactor left in unclean state, the following Selectables were left over: <twisted.internet.process.ProcessReader object at 0xb731162c>
  -------------------------------------------------------------------------------
  Ran 354 tests in 12.723s
  FAILED (errors=4, successes=353)
  

It should probably only report the first error.

That's it for now!

[radix]

Thanks for the review therve. I've dealt with all of your comments. Here's a few particulars:

Pyflakes errors should all be cleaned up.

I deleted the line with the undefined name warning, since it was stupid anyway (others never instantiated that class, only trial did). We don't use the class any more. In fact, there are a number of exception and warning classes which we don't use, but we can't delete them because people may still be importing them and putting them into "except" blocks. We may want to start using zope.deprecation at some point for deprecating module attributes like these, but it's not something we're doing elsewhere.

I've fixed the test failure on Python2.3; it was actually a good catch. There was a real bug in the UncleanWarning? wrapper in that it didn't deal with three-tuples. No code was exercising addError with three-tuples, except that in 2.3, doctest reported its failures as errors instead of failures. This triggered the behavior. I've added another more explicit unit test for handling three-tuples.

I've switched everything to use assertWarns.

I've changed things so that dirty reactor errors are all accumulated into one DirtyReactorAggregateError? per test, that has a very nice str that formats all the problems. While doing this I've also added a fairly thorough test suite for _Janitor!

Back to you!

[jml]

Diffstat says:

 doc/core/man/trial.1                    |   10 -
 twisted/python/util.py                  |   65 +++++-
 twisted/scripts/trial.py                |    3
 twisted/test/test_util.py               |  182 ++++++++++++++++++
 twisted/trial/itrial.py                 |  150 +++++++++++++--
 twisted/trial/reporter.py               |   71 +++++--
 twisted/trial/runner.py                 |    9
 twisted/trial/test/erroneous.py         |   24 ++
 twisted/trial/test/test_deferred.py     |    1
 twisted/trial/test/test_pyunitcompat.py |   55 ++++-
 twisted/trial/test/test_reporter.py     |  289 +++++++++++++++++++++++++++-
 twisted/trial/test/test_runner.py       |   71 ++++++-
 twisted/trial/test/test_tests.py        |   37 ++-
 twisted/trial/test/test_util.py         |  320 +++++++++++++++++++++++++++++++-
 twisted/trial/unittest.py               |   26 +-
 twisted/trial/util.py                   |  225 +++++++++++++++-------
 16 files changed, 1342 insertions(+), 196 deletions(-)

It would be nice if we didn't have branches this big. However, I understand that you've taken over from work that I was doing, and am so glad that you've got the branch to this state that I don't want to say anything negative.

twisted/python/util.py

• twisted/python/util.py:630: redefinition of fuction 'initgroups' from line 589
• Docstring for proxyForInterface
  • should say why or how you might want to use it.
  • change 'resultant' to 'resulting'.
  • Perhaps instead of saying 'will provide', say 'conform' and then remove the disclaimer in parentheses?

twisted/trial/test/test_tests.py

• I prefer test docstrings that describe behaviour, rather than using 'should' or 'check that'.

twisted/trial/test/test_util.py

• I don't really like the dedented triple-quote thing you do in DirtyReactorAggregateErrorTest. *shrug*.
• Why StubErrorReporter? Why not use a regular reporter?
• test_cleanPendingSpinsReactor should include a comment explaining that this is not the desired behaviour, otherwise it looks like a requirement.

Other than that, looks great.

[radix]

Thanks a lot, jml. I've improved proxyForInterface's docstring, fixed all the old test docstring conventions, and added some notes about how expected behavior is not a requirement.

I didn't use the regular Reporter because Reporter is eventually not going to record test objects -- this has already happened in some places.

Thanks!

[therve]

Ok I have only very few aesthetic remarks:

• remove trailing whitespaces of test_deferred
• there are still some 'Deprecated in Twisted 2.5' in reporter and ireporter that should replace by 2.6: cleanupErrors, startSuite, endSuite,
• please try to make the docstrings use all the space up to 79 characters, instead of wrapping at 70
• please use 3 blank lines between module-level objects and 2 blank lines between methods (only on the stuff you added/modified of course)

You can then merge :)

[radix]

(In [21469]) Merge unclean-errors-2091-6

Author: jml, radix Reviewers: therve, jml Fixes #2091

Global reactor state that trial detects after test runs will cause tests to fail. A new trial command line flag, --unclean-warnings, allows easy migration to the new behavior by converting these errors into warnings.