Opened 15 years ago

Closed 14 years ago

#2303 defect closed fixed (fixed)

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

Reported by: jknight Owned by:
Priority: highest Milestone:
Component: trial Keywords:
Cc: spiv, therve, Jean-Paul Calderone Branch: branches/deprecate-setUpClass-2303
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

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.

Attachments (1)

test_setupclass.py (1.1 KB) - added by jknight 15 years ago.

Download all attachments as: .zip

Change History (51)

Changed 15 years ago by jknight

Attachment: test_setupclass.py added

comment:1 Changed 15 years ago by Jonathan Lange

Cc: spiv added
Keywords: review added
Owner: Jonathan Lange deleted
Priority: normalhighest

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.

comment:2 Changed 15 years ago by 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.

comment:3 Changed 15 years ago by 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?

comment:4 Changed 15 years ago by 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.

comment:5 Changed 15 years ago by Jonathan Lange

Owner: set to radix

comment:6 Changed 15 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from radix to Jonathan Lange

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)

comment:7 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Stephen Thorne

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

comment:8 Changed 15 years ago by radix

Keywords: review removed
Owner: changed from Stephen Thorne to Jonathan Lange

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

comment:9 Changed 15 years ago by radix

Keywords: review added
Owner: changed from Jonathan Lange to radix
Status: newassigned

comment:10 Changed 15 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from radix to Jonathan Lange
Status: assignednew

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.

comment:11 Changed 15 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

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

comment:12 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

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

comment:13 Changed 15 years ago by Jean-Paul Calderone

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

comment:14 Changed 15 years ago by 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.

comment:15 Changed 15 years ago by Jonathan Lange

Status: reopenednew

comment:16 Changed 15 years ago by Jonathan Lange

Status: newassigned

comment:17 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

Ready for re-review in setUpClass-2303-2

comment:18 Changed 15 years ago by therve

Cc: therve added
Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange

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

comment:19 Changed 15 years ago by Jonathan Lange

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

comment:20 Changed 15 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

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

comment:21 Changed 15 years ago by 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.

comment:22 Changed 15 years ago by 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.

comment:23 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

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.

comment:24 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Lange

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

comment:25 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

Fixed the failures in the gtk2reactor.

comment:26 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Lange

the branch conflicts with trunk

comment:27 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

Ready for review in setUpClass-2303-3

comment:28 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Lange

Not really working, lots of test failures

comment:29 Changed 15 years ago by 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.

comment:30 Changed 15 years ago by Jonathan Lange

Branch: branches/setUpClass-2303-3

comment:31 Changed 15 years ago by Jonathan Lange

author: jml
Branch: branches/setUpClass-2303-3branches/setUpClass-2303-4

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

comment:32 Changed 15 years ago by 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.

comment:33 Changed 15 years ago by therve

Summary: setUpClass isn't run on subclasses if it already ran on superclassDeprecate setUpClass and tearDownClass and, if possible, fix the subclassing behaviour.

comment:34 Changed 15 years ago by therve

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

comment:35 Changed 14 years ago by Jonathan Lange

Branch: branches/setUpClass-2303-4branches/deprecate-setUpClass-2303

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

comment:36 Changed 14 years ago by Jean-Paul Calderone

author: jmlexarkun

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

comment:37 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jonathan Lange deleted

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

comment:38 Changed 14 years ago by Jonathan Lange

Owner: set to Jonathan Lange
Status: newassigned

comment:39 Changed 14 years ago by 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)
.

comment:40 Changed 14 years ago by radix

Keywords: review removed
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

comment:41 Changed 14 years ago by Jean-Paul Calderone

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

refs #2303

comment:42 Changed 14 years ago by Jean-Paul Calderone

(In [24099]) Remove commented out wildcard import

refs #2303

comment:43 Changed 14 years ago by Jean-Paul Calderone

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

refs #2303

comment:44 Changed 14 years ago by Jean-Paul Calderone

(In [24101]) Remove unused import

refs #2303

comment:45 Changed 14 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

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

comment:46 Changed 14 years ago by Jean-Paul Calderone

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

refs #2303

comment:47 Changed 14 years ago by radix

author: exarkun
Owner: set to Jean-Paul Calderone

Assuming buildbot passed with that change, +1.

comment:48 Changed 14 years ago by radix

author: exarkun
Keywords: review removed

comment:49 Changed 14 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

comment:50 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.