Deprecate setUpClass and tearDownClass and, if possible, fix the subclassing behaviour.

[jknight]

See attached code. It fails if run in alpha order, but succeeds if run with --random 4, as then the subclasses get run first. And if the subclasses are run first, then their setUpClass methods *do* get run.

[Jonathan Lange]

Ready for review in setUpClass-2303.

This probably will also go some way toward fixing #1870.

Notes on the impact of this change:

This branch affects unittest compatibility in two ways. The first is that tests that have setUpClass or tearDownClass must be run from within SharedClassSuite. Otherwise, they simply will not work as expected. The second is that _classCleanUp will only be called if tests are within a ClassSuite (SharedClassSuite is a subclass). This probably doesn't matter for most tests. However, it might matter for some of them.

ClassSuite doesn't particularly care whether its members are all in the same class or not. The decision to have an extra-strict cleanup at the end of every class is a fairly arbitrary one. SharedClassSuite cares deeply about the class of its members.

[radix]

• It is exceedingly difficult to review branches which reorganize code at the same time as functionality change :(
• Is the following note still necessary? + # XXX - make sure this is returned from loadClass

•         # We have to use callLater for this because the run() method
          # blocks using _wait.
          from twisted.internet import reactor
  

  I'm glad you added that note, because I was going to complain about the use of callLater(0) instead of just manually firing back a Deferred in a test case, but it's confusingly placed and the reactor import isn't used.

Ok I'm not really done reviewing this, but I need to get some sleep.

[radix]

I'm also curious about the removal of upDownError. Weren't there any reporters in twisted.trial that implemented it? Was its removal related to the fix in this branch, or just something you wanted to do?

[Jonathan Lange]

I'm sorry about that. There's not much I can do about it, particularly since glyph has jumped all over me for posting "cleanup" tickets.

As for upDownError, it's no longer used by Trial, so it can be deprecated, and removed from the interface. I think that's all I have done.

[Stephen Thorne]

Branch fails to build for me (on shiny.thorne.id.au), account available on request).

twisted.trial.test.test_class.AttributeSharing.test_shared and all of twisted.trial.test.test_class.FactoryCounting fail.

===============================================================================
[FAIL]: twisted.trial.test.test_class.AttributeSharing.test_shared

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 103, in test_shared
    
twisted.trial.unittest.FailTest: 'test_3' != 'test_2'
===============================================================================
[FAIL]: twisted.trial.test.test_class.FactoryCounting.test_runMultipleCopies

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 184, in test_runMultipleCopies
    
twisted.trial.unittest.FailTest: 0 != 1
===============================================================================
[FAIL]: twisted.trial.test.test_class.FactoryCounting.test_runTwice

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 174, in test_runTwice
    
twisted.trial.unittest.FailTest: 0 != 1
===============================================================================
[FAIL]: twisted.trial.test.test_class.FactoryCounting.test_skippingSetUpClass

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 194, in test_skippingSetUpClass
    
twisted.trial.unittest.FailTest: 0 != 1
===============================================================================
[ERROR]: twisted.trial.test.test_class.FactoryCounting.test_createAndRun

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 152, in test_createAndRun
    
exceptions.AttributeError: 'MyTestCase' object has no attribute '_isFirst'
===============================================================================
[ERROR]: twisted.trial.test.test_class.FactoryCounting.test_createTwoAndRun

Traceback (most recent call last):
  File "/Users/stephen/src/Twisted/trunk/twisted/trial/test/test_class.py", line 160, in test_createTwoAndRun
    
exceptions.AttributeError: 'MyTestCase' object has no attribute '_isFirst'
-------------------------------------------------------------------------------
Ran 340 tests in 18.769s

FAILED (failures=4, errors=2, successes=334)

[Jonathan Lange]

test_class has been removed. Please remove your .pyc files and try again.

[radix]

Current issue is that twisted.test.test_threads seems to be broken by this branch -- testCallFromThread and testCallInThread are both timing out.

[Stephen Thorne]

One pyflakes warning that should be cleaned up.

twisted/trial/test/test_tests.py:3: 'log' imported but unused

And then merge, thankyou. Sorry about the pyc thing earlier. We should really make trial notice that kind of shenanigan.

[Jonathan Lange]

(In [19155]) Handle setUpClass / tearDownClass logic outside of TestCase

• Author: jml
• Reviewers: radix, Jerub
• Fixes #2303
• Refs #1870

Trial TestCases with setUpClass or tearDownClass methods use a different logic to regular tests. Instead of running each test in a separate instance, they run all the tests from the same instance.

This branch handles that difference by wrapping setUpClass-style tests in a special suite: SharedClassSuite. This suite runs setUpClass, tearDownClass and runs all of its tests from the same instance.

This branch affects unittest compatibility in two ways. The first is that tests that have setUpClass or tearDownClass must be run from within SharedClassSuite. Otherwise, they simply will not work as expected. The second is that _classCleanUp will only be called if tests are within a ClassSuite (SharedClassSuite is a subclass). This probably doesn't matter for most tests. However, it does matter for tests in test_threads, for example.

ClassSuite doesn't particularly care whether its members are all in the same class or not. The decision to have an extra-strict cleanup at the end of every class is a fairly arbitrary one. SharedClassSuite cares deeply about the class of its members.

[Jean-Paul Calderone]

(In [19162]) Revert r19155 - test suite regression

Refs #2302 Refs #1870

twisted.trial.test.test_runner.DeferredSharedSuiteTest?.test_waitForDeferred fails with gtk reactor.

twisted.trial.test.test_runner.TestSharedClassSuite?.test_reporting fails with Python 2.5.

Some interaction seems to be taking place with the TODO feature. testTLS and testBackwardsTLS from twisted.test.test_ssl.SpammyTLSTestCase are reported as [FAIL] even though they are marked TODO (only in some configurations).

[Jean-Paul Calderone]

This ticket also seems to be a duplicate of #2168.

[Jonathan Lange]

OK. I've fixed the Python 2.5 problem. The GTK problem seems to be related to interaction with _wait, which makes me very, very sad. I guess I'll conjure the dark spirits tomorrow.

I seem to remember glossing over the TODO stuff when I first wrote the branch. It should be fairly straightforward to fix.

[Jonathan Lange]

Ready for re-review in setUpClass-2303-2

[therve]

The problem with testTLS and testBackwardsTLS is still there (see ​http://twistedmatrix.com/buildbot/osx-py2.5-select/builds/27/step-trial/2).

[Jonathan Lange]

The tests now pass, so I reckon it's definitely ready for review.

[Jean-Paul Calderone]

With gtk reactor, twisted.trial.test.test_runner.DeferredSharedSuiteTest.test_waitForDeferred seems to hang indefinitely.

[Jonathan Lange]

Here's the cause, shown as a near-minimal example:

from twisted.internet import gtkreactor
gtkreactor.install()

from twisted.internet import reactor
import gtk

def quit():
    reactor.crash()
    gtk.events_pending()

reactor.callWhenRunning(quit)
reactor.run()

gtk.events_pending() is called in GtkReactor.doIteration which is triggered by the reactor.iterate() calls in the post-class cleanup.

My hunch is that this is a bug in GTK+ 1.2.

[Jonathan Lange]

For what it's worth, it looks like tearDownClass and the post-class cleanup code isn't being run with gtkreactor in trunk.

[Jonathan Lange]

After experimenting with a couple of different approaches, I have simply disabled the pending calls cleanup when running a GTK+ reactor. This is the least bad solution that I've come across -- better suggestions are welcome.

Branch is now up for review at setUpClass-2303-2.

[Jean-Paul Calderone]

I haven't looked at the solution, but gtk2reactor now has failures.

[Jonathan Lange]

Fixed the failures in the gtk2reactor.

[Jean-Paul Calderone]

the branch conflicts with trunk

[Jonathan Lange]

Ready for review in setUpClass-2303-3

[Jean-Paul Calderone]

Not really working, lots of test failures

[Jonathan Lange]

Very unhappy.

I've waited almost a month to hear that this branch fails lots of tests. In it's last iteration, it passed all the tests on all the reactors except for the gtkreactor.

And now, if I find out what these new test failures are and fix them, I'll have to submit it for a fresh round of review. If it's like other branches, I might even have to face general negative criticism about its design.

I know the test failures are no ones fault but my own, but this is a painful way to work.

[Jonathan Lange]

(In [21947]) Branching to 'setUpClass-2303-4'

[Jonathan Lange]

I can't edit the ticket description, but if I could, here's what I'd say:

Deprecate setUpClass and tearDownClass and, if possible, fix the subclassing behaviour.

A separate ticket should be filed to move the class clean up stuff into the main post-test cleanup stuff, fixing any failing tests.

The branch that exists for this isn't very good. It should be reviewed for salvageable code.

[therve]

(I modified the title and gave you admin rights).

[Jonathan Lange]

(In [22007]) Branching to 'deprecate-setUpClass-2303'

[Jean-Paul Calderone]

(In [24003]) Branching to 'deprecate-setUpClass-2303'

[Jean-Paul Calderone]

Okay. I deprecated setUpClass and tearDownClass. I deleted all uses of them in Twisted, too. I didn't try to fix the subclassing issue.

[radix]

By the way, I hate trac formatting.

1. The warning message sucks, and should be a complete sentence including verbs directing the developer to do something, like "stop using setUpClass".
.
2. deferSetUpClass and deferTearDownClass could do with some docstrings
.
3. 
+# from twisted.web2.stream import *
.
can be removed.
.
4. 
-    def setUp(self):
+    def extraSetUp(self):
.
Why do that instead of just overriding setUp and upcalling before other stuff?
.
5. You know what's awesome? doing stuff like this:
.
self.addCleanup(sys.setcheckinterval, sys.getcheckinterval())
sys.setcheckinterval(7)
.

[Jean-Paul Calderone]

(In [24098]) Add docstrings for deferSetUpClass and deferTearDownClass

refs #2303

[Jean-Paul Calderone]

(In [24099]) Remove commented out wildcard import

refs #2303

[Jean-Paul Calderone]

(In [24100]) Simplify check interval setup/cleanup

refs #2303

[Jean-Paul Calderone]

(In [24101]) Remove unused import

refs #2303

[Jean-Paul Calderone]

Everything should be fixed, except point 4. I did it that way because there are two base classes so it's difficult to do an explicit upcall; adding a new method to both bases makes it simple for the child class to invoke it from setUp.

Build results:

​http://buildbot.twistedmatrix.com/boxes-supported?num_builds=1&branch=/branches/deprecate-setUpClass-2303

[Jean-Paul Calderone]

(In [24102]) put the sibpath import back, executable code inside strings depends on it

refs #2303

[radix]

Assuming buildbot passed with that change, +1.

[Jean-Paul Calderone]

(In [24121]) Merge deprecate-setUpClass-2303

Author: exarkun Reviewer: radix Fixes: #2303

Remove all remaining uses of setUpClass and tearDownClass from the Twisted test suite and deprecate the use of these methods by unit test authors.