[TIP] Test discovery for unittest

[Michael Foord]

Robert Collins wrote:
> On Mon, 2009-04-06 at 11:52 +0100, Michael Foord wrote:
>   
>> Robert Collins wrote:
>> Hmm... in our framework we have a custom TestResult, but also a 
>> TestRunner that overrides _makeResult.
>>
>> Our call to actually run tests looks like:
>>
>>     PublishingTestRunner(verbosity=2).run(suite)
>>
>> This pushes results (and tracebacks) into the database as the tests run. 
>> If we want the TextTestRunner to use our custom result objects then as 
>> far as I can tell we have to do this?
>>     
>
> With the current TextTestRunner yes you do; what I mean to say is that
> TextTestRunner currently does things that are IMO properly the
> responsibility of the Result object.
>   

So a job for another day. :-)
> [snip...]
>   
>>> The load_tests hook, which I added to bzr, is an attempt to reduce
>>> duplication from the test_suite hook, while adding in support for custom
>>> loaders and so on.
>>>
>>> Compare:
>>> def test_suite():
>>>     loader = unittest.TestLoader()
>>>     names = [__name__] + [__name__ +  ".tests.test_" + name for name in
>>>         [
>>>         "bar",
>>>         "foo",
>>>         "gam",
>>>         ]]
>>>     tests = loader.loadTestsFromNames(names)
>>>     # do things to tests here
>>>     return tests
>>>
>>>   
>>>       
>> So if a module provides 'test_suite' it is a callable that provides the 
>> tests for that module.
>>
>> The code above implies that it loads the tests for the whole package - 
>> so if test_suite is found in a package (the __init__.py) then we stop 
>> recursing into the package. If test_suite is found in a module then we 
>> use it for that module but continue searching the package / sub-packages.
>>
>> (Note: I see below that you suggest controlling whether we recurse into 
>> packages by having load_tests return two values. I think this is 
>> starting to get overly complex for an initial version. See below for my 
>> suggestions.)
>>     
>
> Well, the problem with standardising is that there is immediate pressure
> to 'get it right' because upgrading a protocol is harder. (e.g. you need
> a new hook point with a slightly different name, docs about the
> differences etc).
>   

Which is why I am suggesting we start with only one hook point.
> That said there are many spellings that could be done - e.g. one hook
> for 'get the suite' and another for 'control recursion'. 
>
>   

Initially I suggest that if a package (not a module) provides test_suite 
then discovery doesn't recurse into the package.

Another mechanism could be added later to control recursion. If a 
package wants to provide test_suite *and* have discovery continue into 
the package (why?) then it can do its own discovery.

>>> with
>>> def load_tests(standard_tests, module, loader):
>>>     names = ["tests.test_" + name or name in 
>>> 	[
>>> 	"bar",
>>>         "foo",
>>>         "gam",
>>> 	]]
>>>     standard_tests.addTest(loader.loadTestsFromNames(
>>>         names, module=module))
>>>     # Do things to standard_tests here
>>>     return standard_tests
>>>
>>> The second one removes all the machinery for choosing a loader (allows
>>> custom loader) 
>>>       
>> Why is test_suite not enough? test_suite allows you to use a custom 
>> loader and anything you do in load_tests to reduce duplication could 
>> also be done in test_suite.
>>     
>
> bugs in test_suite:
>  - cannot control the loader from outside the hook (which means that
>    when you have a better loader you have to change all your test_suite
>    hooks manually to use it, or monkeypatch unittest.TestLoader. Yes, 
>    I've seen that done).
>   

Or just instantiate your custom TestLoader inside test_suite. If you 
switch to using a custom one from not using a custom one then you need 
to update all of your custom hooks irregardless of whether it is spelled 
test_suite or load_tests. I don't see how providing an additional hook 
makes this any simpler.

>  - about 80% of all test_suite methods look the same; package scope ones
>    are a list of imports and recursive returns, module ones construct
>    a loader and use it.
>   

Why is that a problem? It sounds like an argument in favour of 
test_suite - typically they are simple and all look the same.

>  - errors are reported poorly and typically result in the entire test
>    suite not running.
>   

How does load_tests alleviate this problem?

I agree that one problem with test discovery is that if it doesn't 
behave how you expect you can have tests not run at all. You can solve 
this at the TestRunner / TestResult point by collecting information 
about all tests run and (for example) comparing the total number to 
previous runs. i.e. it can be solved outside the discovery mechanism itself.

Other suggestions welcomed.

>  - cannot customise the TestSuite returned (but loaders know how to make
>    a suite, so customising the loader customises the suite as well).
>
>   

Why can you not customise the suite returned? test_suite is responsible 
for creating the suite and has complete control over which loader it uses.

We should allow test_suite to return None by the way - meaning no tests 
for the module / package.
>> My initial goal is to provide a simple system that works. Having the 
>> test_suite protocol seems like it should be enough to allow for 
>> customisation of module / package loading. Why not have test_suite take 
>> a loader argument and the implementation of test_suite is free to use it 
>> or ignore it.
>>     
>
> Well, that makes it closer to 'load_tests'. Indeed, for a load_tests
> hook you can ignore the loader object if you want to. But often you
> don't need to, and thats the nice thing about it :). The reason
> load_tests is not called 'test_suite' is simply API compatibility, bzr's
> test loader has supported test_suite for a long time, when we added
> load_tests we didn't want to cause any incompatibility.
>
> ..
>   

So lets pass the loader into test_suite. I don't understand the purpose 
of the other two arguments though.

>   
>> Here you are changing the suggested protocol above so that load_tests 
>> allows individual *packages* to customize how discovery is done inside 
>> them (it doesn't seem like that would make sense for a module as you 
>> could then get multiple different contradictory rulesets within a single 
>> package).
>>     
>
> Well, it might make sense for a module if the package didn't customise
> anything.
>   

I don't think modules should be able to change discovery rules. In fact 
I don't think we need to support changing discovery rules at all at this 
stage - let packages control how tests are loaded from them using their 
own DiscoveringTestSuite and their own rules if they want.
>  
>   
>> Anyway, at this point we're getting beyond what I want to achieve with 
>> an initial version. The test_suite protocol seems to allow modules and 
>> packages to completely customize how tests are loaded from them. Don't 
>> forget that they can also use their own DiscoveringTestSuite if they 
>> want to change the filter rules.
>>
>> So in summary +1 on the test_suite protocol (a callable taking a loader 
>> as an argument) and -1 on load_tests and the more complex rules.
>>
>> We can always add load_tests as a next step, whereas if we add it now we 
>> are tied to forever remaining compatible with the API signature we choose.
>>     
>
> So test_suite, if you want to be compatible with the folk using it is
> just 'def test_suite():'. If you're not worried about that, I'd spell it
> as
> def test_suite(standard_tests, module, loader):
>     "Return a TestSuite for this module."""
>
> Avoiding all the concerns about controlling recursion, and just disable
> recursion when you encounter one of these. This is broadly what you
> describe above - the only modification I'm making is to pass in the
> module and loader (and then because I've found them both extremely
> useful to pass in).
>   

The loader yes.

What is the use case for passing in the module? Can't test_suite just do 
'__import__(__name__)' if it wants a reference to its containing module?

At the point at which test_suite is called standard_tests doesn't exist 
as we are delegating to test_suite to *create* the tests. Unless you 
mean pass in the top level test suite instance that represents all tests 
collected so far. If so, what is the use case?

Alternatively are you suggesting that we collect tests for the module 
separately and then pass these in as standard_tests for test_suite to 
augment or ignore? test_suite can trivially get that itself surely? (All 
TestCase classes in its global namespace.)

> FWIW, in bzr we don't have test discovery, so the whole issue of
> controlling recursion never came up.
>   

I quite like the ability of a package to completely control how its 
tests are loaded. It seems a natural extension of allowing modules to 
control how their tests are loaded.

The discovery mechanism as currently implemented completely ignores the 
top level package files (it explicitly skips the __init__.py).

Michael

> -Rob
>
>   


-- 
http://www.ironpythoninaction.com/
http://www.voidspace.org.uk/blog