| msg51042 - (view) |
Author: Collin Winter (collinwinter) *  |
Date: 2006-09-01 02:44 |
| 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. |
|
|
| msg51043 - (view) |
Author: Jonathan Lange (mumak) |
Date: 2006-09-01 03:40 |
| 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 |
|
|
| msg51044 - (view) |
Author: Collin Winter (collinwinter) * |
Date: 2006-09-01 04:19 |
| 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. |
|
|
| msg51045 - (view) |
Author: Jonathan Lange (mumak) |
Date: 2006-09-01 04:39 |
| 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. |
|
|
| msg51046 - (view) |
Author: Collin Winter (collinwinter) * |
Date: 2006-09-01 16:40 |
| 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. |
|
|
| msg51047 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2006-09-01 21:08 |
| 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. |
|
|
| msg51048 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2006-09-01 21:18 |
| 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.) |
|
|
| msg51049 - (view) |
Author: Collin Winter (collinwinter) * |
Date: 2006-09-01 21:39 |
| 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. |
|
|
| msg51050 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2006-09-01 22:31 |
| 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.) |
|
|
| msg51051 - (view) |
Author: A.M. Kuchling (akuchling) * |
Date: 2006-11-09 21:25 |
| 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? |
|
|
| msg51052 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2006-11-11 00:27 |
| 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. |
|
|
| msg51053 - (view) |
Author: Collin Winter (collinwinter) * |
Date: 2006-11-11 21:49 |
| 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. |
|
|
| msg51054 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2007-03-07 09:12 |
| Committed most of the patch as part of rev. 54199. Not backporting to 2.5. |
|
|
| msg91675 - (view) |
Author: Ionut Turturica (jonozzz) |
Date: 2009-08-18 01:10 |
| 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. |
|
|
| msg91676 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2009-08-18 01:32 |
| 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. |
|
|
| msg91747 - (view) |
Author: Ionut Turturica (jonozzz) |
Date: 2009-08-19 21:21 |
| 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. |
|
|