Questions on unittest behaviour

[Collin Winter]

While working on a test suite for unittest these past few weeks, I've
run across some behaviours that, while not obviously wrong, don't
strike me as quite right, either. Submitted for your consideration:

1) TestCase.tearDown() is only run if TestCase.setUp() succeeded. It
seems to me that tearDown() should always be run, regardless of any
failures in setUp() or the test method itself.

The case I'm considering is something like this, ie, a multi-part setUp():

def setUp(self)
lock_file(testfile) # open_socket(), connect_to_database(), etc

something_that_raises_an_exception()

def tearDown(self):
if file_is_locked(testfile):
unlock_file(testfile)

In this pseudo-code example, the file won't be unlocked if some later
operation in setUp() raises an exception. I propose that
TestCase.run() be changed to always run tearDown(), even if setUp()
raise an exception.

I'm undecided if this is a new feature (so it should go in for 2.6) or
a bug fix; I'm leaning toward the latter.

2) The TestLoader.testMethodPrefix attribute currently allows anything
to be assigned to it, including invalid objects and the empty string.
While the former will cause errors to be raised when one of
TestLoader's loadTestsFrom*() methods is called, the empty string will
raise no exception; rather, the loadTestsFrom*() methods will
interpret every possible attribute as being a test method, e.g.,
meaning you get things like assertEqual(), failUnlessEqual(), etc,
when TestLoader.loadTestsFromTestCase() is run.

I propose protecting testMethodPrefix with a property that validates
the assigned value, restricting input to non-empty instances of str. I
see this as a bug fix that should go in before 2.5-final.

3) TestLoader.loadTestsFromTestCase() accepts objects that are not
test cases and will happily look for appropriately-named methods on
any object you give it. This flexibility should be documented, or
proper input validation should be done (a bug fix for 2.5).

4) TestLoader.loadTestsFromName() (and by extension,
loadTestsFromNames(), too) raises an AttributeError if the name is the
empty string because -- as it correctly asserts -- the object does not
contain an attribute named ''. I recommend that this be tested for and
ValueError be raised (bug fix for 2.5).

This of course leads into the question of how much input validation
should be done on these names. Should loadTestsFrom{Name,Names}() make
sure the names are valid attribute names, or is this overkill?

5) When TestLoader.loadTestsFrom{Name,Names}() are given a name that
resolves to a classmethod on a TestCase subclass, the method is not
invoked. From the docs:

The specifier name is a ``dotted name'' that may resolve either to a module, a test
case class, a TestSuite instance, a test method within a test case class, or a
callable object which returns a TestCase or TestSuite instance.

It is not documented which of these tests takes priority: is the
classmethod "a test method within a test case class" or is it a
callable? The same issue applies to staticmethods as well.


Once I get answers to these questions, I can finish off the last few
bits of the test suite and have it ready for 2.5-final.

Thanks,
Collin Winter

 

[Sybren Stuvel]

Collin Winter enlightened us with:

1) TestCase.tearDown() is only run if TestCase.setUp() succeeded.

I agree with you there. If it's unwanted to have tearDown() always
called, there should be another function that's always called.

2) The TestLoader.testMethodPrefix attribute currently allows
anything to be assigned to it, including invalid objects and the
empty string.

I agree with you - a property is easily created, and might prevent a
lot of hairpulling.

Sybren

 

[Roy Smith]

While working on a test suite for unittest these past few weeks, I've
run across some behaviours that, while not obviously wrong, don't
strike me as quite right, either. Submitted for your consideration:

1) TestCase.tearDown() is only run if TestCase.setUp() succeeded. It
seems to me that tearDown() should always be run, regardless of any
failures in setUp() or the test method itself.

I look at setUp() as a constructor. It's the constructor's job to either
completely construct an object, or clean up after itself. With your
example:

It would be cleaner to just do:

def setUp(self):
lock_file()
try:
something_that_raises_an_exception()
except
unlock_file()
raise

In this pseudo-code example, the file won't be unlocked if some later
operation in setUp() raises an exception. I propose that
TestCase.run() be changed to always run tearDown(), even if setUp()
raise an exception.

While this might simplify setUp() methods, it complicates tearDown()s,
because now they can't be sure what environment they're running in. One
way or the other, somebody has to deal with exceptions in setUp(). Keeping
the exception handling code as close to the source of the exception seems
like the right thing to do.

 

[John Roth]

While working on a test suite for unittest these past few weeks, I've
run across some behaviours that, while not obviously wrong, don't
strike me as quite right, either. Submitted for your consideration:

1) TestCase.tearDown() is only run if TestCase.setUp() succeeded. It
seems to me that tearDown() should always be run, regardless of any
failures in setUp() or the test method itself.

I'm undecided if this is a new feature (so it should go in for 2.6) or
a bug fix; I'm leaning toward the latter.

I strongly disagree. First, this is the documented behavior
for all xUnit ports (see jUnit), second, it's the responsibility
of a routine to clean up for itself.

2) The TestLoader.testMethodPrefix attribute currently allows anything
to be assigned to it, including invalid objects and the empty string.
While the former will cause errors to be raised when one of
TestLoader's loadTestsFrom*() methods is called, the empty string will
raise no exception; rather, the loadTestsFrom*() methods will
interpret every possible attribute as being a test method, e.g.,
meaning you get things like assertEqual(), failUnlessEqual(), etc,
when TestLoader.loadTestsFromTestCase() is run.

I propose protecting testMethodPrefix with a property that validates
the assigned value, restricting input to non-empty instances of str. I
see this as a bug fix that should go in before 2.5-final.

Several points. First, since it's been this way since day one and
nobody has complained, it's not a bug. That means it's a feature,
and 2.5 is in a complete, absolute, utter, total and thorough
feature freeze. This is not the place to get Guido's attention. As
far as I'm aware, he doesn't read this newsgroup.

On the other hand, you might be working with the crew on
pythondev; I only read the summaries. If you are, then you
already know what I mentioned above. If you aren't, your
odds of getting a new test suite into 2.5 final are somewhere
on the far side of zero.

3) TestLoader.loadTestsFromTestCase() accepts objects that are not
test cases and will happily look for appropriately-named methods on
any object you give it. This flexibility should be documented, or
proper input validation should be done (a bug fix for 2.5).

I'm pretty sure I use this. Making it go away would cause
me to have to find another way around the misbegotten
prefix selection mechanism. There's a lot of discussion
around something that's called "behavior driven development"
which is neither here nor there other than that group of people
considers the whole notion of a name with a fixed element to
be a problem with constructing really meaningful test names.

4) TestLoader.loadTestsFromName() (and by extension,
loadTestsFromNames(), too) raises an AttributeError if the name is the
empty string because -- as it correctly asserts -- the object does not
contain an attribute named ''. I recommend that this be tested for and
ValueError be raised (bug fix for 2.5).

Prior point about 2.5 being in a freeze. Take it to the pythondev
mailing list if you really want action. As far as I'm concerned,
this is a "so what." There are much deeper issues with unittest
than this.

This of course leads into the question of how much input validation
should be done on these names. Should loadTestsFrom{Name,Names}() make
sure the names are valid attribute names, or is this overkill?

What _I_ want is a method of selecting tests from a class
that doesn't depend on pattern matching the name.
I've got it rigged to not do that, and I feel that my names
are now significantly better.

5) When TestLoader.loadTestsFrom{Name,Names}() are given a name that
resolves to a classmethod on a TestCase subclass, the method is not
invoked. From the docs:


It is not documented which of these tests takes priority: is the
classmethod "a test method within a test case class" or is it a
callable? The same issue applies to staticmethods as well.

These are all new since pyUnit (renamed to unittest for inclusion
into the standard library) was written. My viewpoint is that it's a
feature that's looking for a use case.

As I intimated above, my current practice is to not use the supplied
mechanism for locating tests. I don't use it for locating test classes
either.

Once I get answers to these questions, I can finish off the last few
bits of the test suite and have it ready for 2.5-final.

Answers? On this list? I'm not even sure you can get
coherent opinions here.

John Roth