Ticket #2091 (closed enhancement: fixed )

Opened 3 years ago

Last modified 2 years ago

Make reactor unclean warnings reported errors

Reported by: jml Assigned to: radix
Type: enhancement Priority: highest
Milestone: Component: trial
Keywords: Cc: glyph, therve
Branch: Author:
Launchpad Bug:

Description

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

Attachments

getBatchOutput.patch (3.6 kB) - added by dreid 3 years ago.
fix implementations and usage of _getBatchOutput to not use iterate

Change History

  2006-09-15 00:35:25+00:00 changed by 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.

  2006-09-16 06:23:11+00:00 changed by 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.

  2006-09-16 07:49:24+00:00 changed by jml

  • keywords set to review
  • owner deleted
  • priority changed from normal to highest

Ready for review in unclean-errors-2091

  2006-09-16 08:34:04+00:00 changed by glyph

  • cc deleted
  • owner set to jml

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.

  2006-09-16 13:58:58+00:00 changed by jml

  • keywords deleted

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

  2006-09-17 06:01:28+00:00 changed by jml

  • cc set to glyph
  • status changed from new to assigned

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

  2006-09-23 18:01:37+00:00 changed by dreid

  • attachment getBatchOutput.patch added

fix implementations and usage of _getBatchOutput to not use iterate

  2006-09-24 03:17:57+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to exarkun
  • status changed from assigned to new

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

  2006-09-27 05:49:04+00:00 changed by jml

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

  2006-09-29 01:32:34+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to jml
  • 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

  2006-09-29 12:14:30+00:00 changed by 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?

  2006-09-29 12:17:20+00:00 changed by jml

Merged forward to unclean-errors-2091-3

  2006-09-30 08:08:59+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to exarkun

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.

  2006-09-30 22:55:16+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to jml

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?

  2006-09-30 23:31:40+00:00 changed by jml

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

  2006-11-16 01:31:05+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to exarkun

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.

  2006-11-17 15:47:13+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to jml

Waiting for some more changes to be committed...

  2006-11-18 12:25:12+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to exarkun

Oops.

  2006-11-18 20:48:58+00:00 changed by exarkun

  • keywords deleted
  • owner changed from exarkun to jml

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.

  2006-11-20 21:33:24+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to exarkun

Fixed the handful of tests. Back for rereview

  2006-11-20 22:31:14+00:00 changed by exarkun

  • owner changed from exarkun to radix

  2006-11-22 02:29:08+00:00 changed by jml

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

  2006-11-30 04:28:28+00:00 changed by jerub

  • keywords deleted
  • owner changed from radix to jml

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.

  2006-12-04 06:21:03+00:00 changed by jml

  • keywords set to review
  • owner changed from jml to jerub

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.

  2006-12-04 23:07:58+00:00 changed by jml

Now in unclean-errors-2091-5

  2006-12-05 11:29:20+00:00 changed by jerub

  • keywords deleted
  • owner changed from jerub to jml

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

  2006-12-05 12:42:48+00:00 changed by jml

  • status changed from new to closed
  • resolution set to fixed

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

  2006-12-05 15:56:38+00:00 changed by exarkun

  • status changed from closed to reopened
  • resolution deleted

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

  2006-12-05 15:59:24+00:00 changed by 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.

  2006-12-06 03:35:16+00:00 changed by 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.

  2006-12-08 03:35:37+00:00 changed by glyph

  • milestone deleted

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.

  2007-01-14 05:38:00+00:00 changed by 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.

  2007-02-04 02:10:10+00:00 changed by jml

  • status changed from reopened to new

  2007-02-04 02:11:16+00:00 changed by jml

  • status changed from new to assigned

  2007-02-10 01:37:19+00:00 changed by 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.

  2007-05-12 04:00:53+00:00 changed by jml

  • priority changed from highest to high

  2007-10-26 20:20:25+00:00 changed by radix

  • keywords set to review
  • owner changed from jml to therve
  • status changed from assigned to new

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.

  2007-10-26 20:37:16+00:00 changed by radix

btw the branch is unclean-errors-2091-6

  2007-10-27 09:18:54+00:00 changed by therve

  • cc changed from glyph to glyph, therve
  • keywords deleted
  • owner changed from therve to radix
  • 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!

  2007-10-27 19:50:48+00:00 changed by radix

  • keywords set to review
  • owner changed from radix to therve

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!

  2007-10-28 07:54:24+00:00 changed by jml

  • keywords deleted
  • owner changed from therve to radix

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.

  2007-10-28 14:45:05+00:00 changed by radix

  • keywords set to review
  • owner changed from radix to therve

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!

  2007-10-28 15:09:40+00:00 changed by therve

  • keywords deleted
  • owner changed from therve to radix
  • priority changed from high to highest

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 :)

  2007-10-28 16:09:28+00:00 changed by radix

  • status changed from new to closed
  • resolution set to fixed

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

Note: See TracTickets for help on using tickets.