Ticket #2091 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

Make reactor unclean warnings reported errors

Reported by: jml Owned by: radix
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 Download (3.6 KB) - added by dreid 4 years ago.
fix implementations and usage of _getBatchOutput to not use iterate

Change History

Changed 4 years ago 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.

Changed 4 years ago 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.

Changed 4 years ago by jml

  • priority changed from normal to highest
  • owner jml deleted
  • keywords review added

Ready for review in unclean-errors-2091

Changed 4 years ago by glyph

  • cc warner removed
  • 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.

Changed 4 years ago by jml

  • keywords review removed

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

Changed 4 years ago by jml

  • cc glyph added
  • 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.

Changed 4 years ago by dreid

fix implementations and usage of _getBatchOutput to not use iterate

Changed 4 years ago by jml

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

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

Changed 4 years ago by jml

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

Changed 4 years ago by exarkun

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

Changed 4 years ago 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?

Changed 4 years ago by jml

Merged forward to unclean-errors-2091-3

Changed 4 years ago by jml

  • owner changed from jml to exarkun
  • keywords review added

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.

Changed 4 years ago by exarkun

  • keywords review removed
  • 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?

Changed 4 years ago by jml

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

Changed 4 years ago by jml

  • owner changed from jml to exarkun
  • keywords review added

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.

Changed 4 years ago by exarkun

  • keywords review removed
  • owner changed from exarkun to jml

Waiting for some more changes to be committed...

Changed 4 years ago by jml

  • keywords review added
  • owner changed from jml to exarkun

Oops.

Changed 4 years ago by exarkun

  • keywords review removed
  • 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.

Changed 4 years ago by jml

  • keywords review added
  • owner changed from jml to exarkun

Fixed the handful of tests. Back for rereview

Changed 4 years ago by exarkun

  • owner changed from exarkun to radix

Changed 4 years ago by jml

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

Changed 4 years ago by jerub

  • keywords review removed
  • 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.

Changed 4 years ago by jml

  • keywords review added
  • 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.

Changed 4 years ago by jml

Now in unclean-errors-2091-5

Changed 4 years ago by jerub

  • owner changed from jerub to jml
  • keywords review removed

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

Changed 4 years ago 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.

Changed 4 years ago by exarkun

  • status changed from closed to reopened
  • resolution fixed 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.
-------------------------------------------------------------------------------

Changed 4 years ago 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.

Changed 4 years ago 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.

Changed 4 years ago by glyph

  • milestone Twisted-2.5 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.

Changed 4 years ago 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.

Changed 4 years ago by jml

  • status changed from reopened to new

Changed 4 years ago by jml

  • status changed from new to assigned

Changed 4 years ago 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.

Changed 3 years ago by jml

  • priority changed from highest to high

Changed 3 years ago by radix

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

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.

Changed 3 years ago by radix

btw the branch is unclean-errors-2091-6

Changed 3 years ago by therve

  • cc therve added
  • owner changed from therve to radix
  • keywords review removed
  • 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!

Changed 3 years ago by radix

  • owner changed from radix to therve
  • keywords review added

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!

Changed 3 years ago by jml

  • owner changed from therve to radix
  • keywords review removed

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.

Changed 3 years ago by radix

  • owner changed from radix to therve
  • keywords review added

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!

Changed 3 years ago by therve

  • priority changed from high to highest
  • owner changed from therve to radix
  • keywords review removed

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

Changed 3 years ago 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.