[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 17:39

Message:
Logged In: YES 
user_id=1344176

Jim:

> (Is there something they could do to the TestCase() that
> would start to fail if you wrap it in a TestSuite()?
> Remember that they might have local standards saying "only
> one test class per file" or something, so they might never
> have gotten a real suite back in the past.)

Off the top of my head:

- TestSuite.run() requires a TestResult instance to be
passed in where TestCase.run()'s result parameter is optional.

- TestSuite doesn't have TestCase's shortDescription() or
id() method.

- Assignment to a TestCase instance's failureException
attribute will affect the object's run() method, whereas the
same assignment on a TestSuite will have no effect.

IMHO none of these is significant enough to block that
particular section of the patch. Remember that we're talking
about a behaviour that's only triggered when the given name
resolves to a callable and that callable returns a TestCase
instance when invoked.

I don't like the idea of keeping around long-standing minor
bugs just because someone, somewhere might be using them.
Can anyone produce a real example where someone is relying
on this behaviour? I'll send a message to c.l.p on Monday
and see if anyone in that wider audience can come up with
concrete cases where these behaviours are mission-critical.

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

Comment By: Jim Jewett (jimjjewett)
Date: 2006-09-01 17:18

Message:
Logged In: YES 
user_id=764593

To clarify:  It would be wrong to put in checks that refuse 
to run an invalid test that would have run today.

But wrapping TestCase in a TestSuite before returning it is 
probably OK, since it fixes the list(x) bug you mentioned; 
you just need to be sure that it won't break something 
else.  Unfortunately, it might.  (Is there something they 
could do to the TestCase() that would start to fail if you 
wrap it in a TestSuite()?  Remember that they might have 
local standards saying "only one test class per file" or 
something, so they might never have gotten a real suite 
back in the past.)

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

Comment By: Jim Jewett (jimjjewett)
Date: 2006-09-01 17:08

Message:
Logged In: YES 
user_id=764593

>> The type checking in TestLoader is only 
>> there to *find* the tests. ... 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.

Why?  If you were designing from scratch, I would agree, 
because it would make understanding the module easier.  But 
once the module has been deployed this long -- particularly 
with the various confusing aspects -- people have gotten 
used to treating it as a black box.  If they go through the 
full procedure, then it won't matter that the runner could 
have handled something else too.  If they go around the 
loader, then this change will break their regression 
testing.  At a minimum, it needs to be something that can 
be shut off again easily (such as a strict option), but in 
that case ... why bother to enforce it at all?

>> ... docs ... for the last _five years_ 
>> -- I'd say this is a long-standing bug
>> in the code, not the documentation.

The disagreement between them is a long-standing bug.  But 
this can be resolved by changing either.  A feature is a 
bug with seniority.  After this long, even a bad decision 
needs to be respected for backwards compatibility.  (It 
could certainly be changed for Py3K, though.)

> Your position seems to be "the code
> works as written; who cares what the
> intention was".

Close.  The code is _in_use_ as written, and is in use by 
people who don't fully understand the intent.  Honoring the 
developers' original intent would break things for users.

> ... core idea behind the TestLoader
> methods is to return something that can be run()";
> ... the addition of __iter__ to TestSuite
> ... ``list(loader.loadTestsFromName("foo.bar"))''
> raised unexpected TypeErrors, complaining that 
> the return value from loadTestsFromName() wasn't 
> iterable.

So the change to TestSuite wasn't as compatible as 
expected, nor as tested and documented as desired.  :{

The docs should definately be changed to mention this 
requirement, and the code should probably be changed to 
make your code work.  That could mean adding iteration to 
TestCase, or it could mean fixing loadTests... to wrap 
individual cases in a suite, or, for safety, it could mean 
both.

				

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

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