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.

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

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.

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.

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.

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.

				

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

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.

Logged In: YES 
user_id=764593

(Responding to the patch, not directly to the conversation)

(1)  (around lines 234) Do you really want two test cases 
to compare equal because they have the same name, even if 
that name is the default runTest?  

(2)  Have you made sense of the UnboundMethodType case?  As 
nearly as I can figure, the old behavior was to run call 
the test case with a method name (which by default tried to 
run it, using that string as a TestResult?!?, and then 
returned None), and the new behavior just wraps this None 
in a TestSuite.

(3)  After patch, the callable case has a "return test" 
which is dead code, because the other branches either 
raised or returned on their own.

(4)  Changing the raise from ValueError to TypeError would 
make sense if not for backwards compatibility.  Could you 
use a custom error that inherits from both?

Please don't let these comments get you down; this is code 
that needs cleaning.  (The changes to not special-case 
jython and to not rewalk the mro are particularly good -- 
because they don't do anything, having them in the code is 
misleading.)

Logged In: YES 
user_id=11375

Some bits of this patch (the URL change, using True/False
and sys.exc_info()) are uncontroversial.  Shall I go ahead
and try to apply those bits, or can the debate on the
changes converge to a decision?

Logged In: YES 
user_id=764593

Doing the obvious parts first makes sense to me, because it 
leaves a cleaner slate for comparisons.  On the other hand, the 
job still won't be done, so if Collin wants it all at once, that 
would be up to him.

Logged In: YES 
user_id=1344176

I'd be perfectly happy to have the obvious parts applied as
soon as possible. I'm hoping to get back to working on
Python -- including this patch -- soon; three successive
computer failures have thrown a bit of a kink in my
free-time coding.

Committed most of the patch as part of rev. 54199. Not backporting to 2.5.

I am a little bit concerned with the new __eq__:

    def __eq__(self, other):
        if type(self) is not type(other):
            return False
        return self._tests == other._tests

Why did you use "self._tests == other._tests" instead of "self is other" ?

After I upgraded to 2.6 I started to have some issues with a breadth
first iterator for a TestSuite and I tracked them down to this eq method.

At a guess, because equality is not the same as identity.  (If identity
was the requirement for equality for this class, the method body would
_just_ be self is other).

You should open a new bug report explaining the problem you are seeing
in more detail, with a reference to this one in the body of your message.

I keep trying to repro this on a smaller scale TestSuite but without any
success. The whole test framework has around 300 tests and I can't
really upload it.

In the mean time I changed the TestSuite's __eq__ back to identity.

I will attach the _breadth_first() method that parses the TestSuites.