[Patches] [ python-Patches-1550273 ] Fix numerous bugs in unittest

[SourceForge.net]

Patches item #1550273, was opened at 2006-08-31 22:44
Message generated for change (Comment added) made by collinwinter
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1550273&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Library (Lib)
Group: Python 2.5
Status: Open
Resolution: None
Priority: 5
Submitted By: Collin Winter (collinwinter)
Assigned to: Nobody/Anonymous (nobody)
Summary: Fix numerous bugs in unittest

Initial Comment:
Following from the test suite for unittest that I
uploaded as SF #1550272, this patch fixes the bugs that
were uncovered while writing the tests.

1) TestLoader.loadTestsFromName() failed to return a
suite when resolving a name to a callable that returns
a TestCase instance.

2) Fix a bug in both TestSuite.addTest() and
TestSuite.addTests() concerning a lack of input
checking on the input test case(s)/suite(s).

3) Fix a bug in both TestLoader.loadTestsFromName() and
TestLoader.loadTestsFromNames() that had ValueError
being raised instead of TypeError. The problem occured
when the given name resolved to a callable and the
callable returned something of the wrong type.

4) When a name resolves to a method on a TestCase
subclass, TestLoader.loadTestsFromName() did not return
a suite as promised.

5) TestLoader.loadTestsFromName() would raise a
ValueError (rather than a TypeError) if a name resolved
to an invalid object. This has been fixed so that a
TypeError is raised.

6) TestResult.shouldStop was being initialised to 0 in
TestResult.__init__. Since this attribute is always
used in a boolean context, it's better to use the False
spelling.

In addition, all uses of the name PyUnit were changed
to unittest.

With these changes, the uploaded test suite passes. The
Python test suite still passes all tests, as do all the
unittest-based test suites I tested the module against.

----------------------------------------------------------------------

>Comment By: Collin Winter (collinwinter)
Date: 2006-09-01 12:40

Message:
Logged In: YES 
user_id=1344176

> Consistency within the module is not the important thing
> here. TestLoader and TestSuite are separate components.
> The type checking in TestLoader is only there to *find*
> the tests. 
>
> Just because TestLoader is inherently limited doesn't mean
> the limitations should be forced down to TestSuite.

The important thing is that unittest presents a unified view
of what is and what is not a test case. Having one component
take a much wider view of test-case-ness than another
component would be confusing.

>> Given that the docs for TestLoader.loadTestsFrom*() have
>> begun with "Return a suite of all test cases" since
>> r20345
>> -- that is, for the last _five years_ -- I'd say this is
>> a long-standing bug in the code, not the documentation.
>
> If the documentation has been wrong for five years, then
> the correct thing to do is fix the documentation, not the
> code.

Why do you assume that it's the documentation that's wrong?
Of all the code paths through loadTestsFromName(), only one
returns something other than a suite. Your position seems to
be "the code works as written; who cares what the intention
was".

You said that "[t]he core idea behind the TestLoader
methods is to return something that can be run()"; ignoring
the fact that there's no basis in the documentation or code
for this assertion, the addition of __iter__ to TestSuite
has enlarged this imaginary guarantee of "run()-able-ness"
to "run()-able and iter()-able". I ran across this issue
when code like ``list(loader.loadTestsFromName("foo.bar"))''
raised unexpected TypeErrors, complaining that the return
value from loadTestsFromName() wasn't iterable. TestCase
does not conform to the expectations set up by the use of
the word "suite" in the docs.

----------------------------------------------------------------------

Comment By: Jonathan Lange (mumak)
Date: 2006-09-01 00:39

Message:
Logged In: YES 
user_id=602096

> I added these checks in order to be consistent with the rest
> of the module, where inheritance from TestCase is required
> (TestLoader.loadTestsFromModule(),
> TestLoader.loadTestsFromName(),
> TestLoader.loadTestsFromNames()). This policy should be
> enforced throughout the module, not piecemeal.

Consistency within the module is not the important thing
here. TestLoader and TestSuite are separate components. The
type checking in TestLoader is only there to *find* the tests. 

Just because TestLoader is inherently limited doesn't mean
the limitations should be forced down to TestSuite.


> Given that the docs for TestLoader.loadTestsFrom*() have
> begun with "Return a suite of all test cases" since
> r20345
> -- that is, for the last _five years_ -- I'd say this is a
> long-standing bug in the code, not the documentation.

If the documentation has been wrong for five years, then the
correct thing to do is fix the documentation, not the code.

As I said, it doesn't change behaviour significantly, so I
lack concern.

----------------------------------------------------------------------

Comment By: Collin Winter (collinwinter)
Date: 2006-09-01 00:19

Message:
Logged In: YES 
user_id=1344176

Jonathan,

> I don't think addTest should check the type of test --
> duck typing is sufficient here. Other testing frameworks 
> should not have to subclass unittest.TestCase in order to be
> able to add their test cases to a unittest.TestSuite.

I added these checks in order to be consistent with the rest
of the module, where inheritance from TestCase is required
(TestLoader.loadTestsFromModule(),
TestLoader.loadTestsFromName(),
TestLoader.loadTestsFromNames()). This policy should be
enforced throughout the module, not piecemeal.

> In loadTestsFromName, it's not strictly necessary to
> return TestSuite([test]). The core idea behind the
> TestLoader methods is to return something that can be
> run(). Also, it's generally not a good idea to change
> long-standing behaviour to match documentation. It should
> be the other way around.

Given that the docs for TestLoader.loadTestsFrom*() have
begun with "Return a suite of all test cases" since r20345
-- that is, for the last _five years_ -- I'd say this is a
long-standing bug in the code, not the documentation.

----------------------------------------------------------------------

Comment By: Jonathan Lange (mumak)
Date: 2006-08-31 23:40

Message:
Logged In: YES 
user_id=602096

G'day Collin,

I've just had a look at the patch -- looks pretty good.

A couple of things though:

I don't think addTest should check the type of test -- duck
typing is sufficient here. Other testing frameworks should
not have to subclass unittest.TestCase in order to be able
to add their test cases to a unittest.TestSuite.

In loadTestsFromName, it's not strictly necessary to return
TestSuite([test]). The core idea behind the TestLoader
methods is to return something that can be run(). Also, it's
generally not a good idea to change long-standing behaviour
to match documentation. It should be the other way around.

It's really good to see unittest finally getting some love
-- thanks Collin.

jml

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1550273&group_id=5470