Opened 16 years ago

Closed 12 years ago

#1870 defect closed wontfix (wontfix)

TestCase.setUpClass and TestCase.tearDownClass are undocumented and have confusing behavior

Reported by: Jean-Paul Calderone Owned by:
Priority: high Milestone:
Component: trial Keywords:
Cc: spiv, therve Branch:
Author:

Description

I used to know how these worked but I don't anymore and I can't figure it out.

I would like someone to explain the current semantics and then discuss possible changes to be made to them so that these features once again make sense and stop surprising me every time I try to use them.

Change History (20)

comment:1 Changed 16 years ago by spiv

Cc: spiv added

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

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

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

#2428 is probably related to this.

comment:5 Changed 15 years ago by therve

Cc: therve added

I have a suggestion for this, because I think we waste a lot of time on this issue. Why setUpClass and tearDownClass aren't class methods? We could deprecate current behavior (having setUpClass/tearDownClass as instance method), probably we can have both behavior at the same time. Implicit shared state is really horrible, and I don't think it brings something here.

Of course, I hope this way we can solve related problems like #2303 and #2428.

comment:6 Changed 15 years ago by Jonathan Lange

Implicit shared state is really horrible.

I tried making setUpClass and tearDownClass class methods once before.

Some things:

  1. It's really hard to change an instance method to a class method in a backwards-compatible way.
  2. I think that the solution in setUpClass-2303 is more correct.
  3. The more interesting question is "what should be responsible for running setUpClass and tearDownClass?".
  4. SomeTestCase('test_method').run(TestResult()) DTRT
  5. Trial tests must be usable in other test runners.

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

It seems unlikely that setUp/tearDownClass will be resolved in a satisfactory manner. Why don't we deprecate and remove them?

Regarding the 5th point, could you clarify what you mean by "Trial tests"?

comment:8 Changed 15 years ago by Jonathan Lange

+1 for deprecating and removing

Things that subclass t.trial.unittest.TestCase -- in particular, Deferred-returning tests.

comment:9 Changed 15 years ago by therve

(FWIW, there are 20 setUpClass and 11 tearDownClass in all twisted outside trial).

comment:10 Changed 15 years ago by therve

And, I'm also for deprecating it. It seems like a very minimal optimization to me.

comment:11 Changed 15 years ago by jknight

+1 for removing it if it can't be properly fixed, -1 for removing it without there being an alternative replacement.

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

Given:

def expensiveOperation():
    ....

class FooTests(TestCase):
    def setUpClass(self):
        self.x = expensiveOperation()

I'd suggest this as an alternative:

def expensiveOperation():
    ....
expensiveOperation = memoize(expensiveOperation)

class FooTests(TestCase):
    def setUpClass(self):
        self.x = expensiveOperation()

comment:13 Changed 15 years ago by therve

You probably meant setUp the second time.

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

I probably did.

comment:15 Changed 15 years ago by Jonathan Lange

I guess it depends on what you mean by "effective replacement". And I guess that depends on the use cases.

e.g. Setting something up once isn't particularly interesting. The interesting thing is tearing it down when it's no longer needed.

comment:16 in reply to:  6 Changed 15 years ago by Glyph

Replying to jml:

Trial tests must be usable in other test runners.

As you well know, I don't have this requirement personally, and it irks me that it always seems to be raised, with little additional detail, in the context of preventing any improvements to trial's architecture. In this particular context, it's not even clear to me what you are responding to. Nobody else previously said "Let's break trial for other test runners!", and you didn't explain which change would break it with other test runners.

I've opened #2739 for more discussion of this issue, because I believe this is an important requirement (I tend to hear it from the heaviest users of trial outside of Divmod) but I want to have enough information to understand what's required when an objection is raised on the basis of it.

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

As therve pointed out earlier, there are 20 setUpClasses and 11 tearDownClasses. So, going just by the numbers, setUpClass seems to be more interesting. ;)

And if we actually examine each tearDownClass, we find that not one of them does something that cannot be done equally well by setUpClass. So it seems Twisted itself doesn't really benefit from the ability to tear things down when no longer needed.

There are a lot of other things about this part of trial which I could write, but I'm not sure it's necessary at this point (it seems doing so would only serve to confuse the matter). If it's still not clear to anyone else that we should drop this feature, I can try to marshal the various ideas into a coherent form, though.

Regardless, I'm going to create some tickets for converting all of the tearDownClass uses in Twisted outside of trial into uses of tearDown instead.

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

And if we actually examine each tearDownClass, we find that not one of them does something that cannot be done equally well by setUpClass.

Make that equally well by tearDown. Sigh.

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

Resolution: wontfix
Status: newclosed

We decided to deprecate these APIs a while ago (#2903) and I'm about to delete them, so there's nothing left to do here.

comment:20 Changed 11 years ago by <automation>

Owner: Jonathan Lange deleted
Note: See TracTickets for help on using tickets.