pattern=None when following documentation for load_tests and unittest.main()

[gagern]

BPO	11218
Nosy	@mdickinson, @ezio-melotti, @merwok, @voidspace
Files	tester.py: demonstrating issue issue11218_patternNone.patch: Treat pattern=None like ommitted pattern

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/voidspace'
closed_at = None
created_at = <Date 2011-02-15.08:33:26.840>
labels = ['type-bug', 'library']
title = 'pattern=None when following documentation for load_tests and unittest.main()'
updated_at = <Date 2020-03-24.09:43:01.752>
user = 'https://bugs.python.org/gagern'

bugs.python.org fields:

activity = <Date 2020-03-24.09:43:01.752>
actor = 'Zbynek.Winkler'
assignee = 'michael.foord'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2011-02-15.08:33:26.840>
creator = 'gagern'
dependencies = []
files = ['20763', '25208']
hgrepos = []
issue_num = 11218
keywords = ['patch']
message_count = 10.0
messages = ['128579', '157891', '157935', '158219', '158232', '158234', '158235', '158236', '172463', '348881']
nosy_count = 9.0
nosy_names = ['mark.dickinson', 'gagern', 'vila', 'ezio.melotti', 'eric.araujo', 'michael.foord', 'Zbynek.Winkler', 'rik.poggi', 'Stefan Sullivan']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue11218'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

[gagern]

If I follow the documentation at http://docs.python.org/library/unittest.html#unittest.main by putting the following two snippets of code in my module file:

def load_tests(loader, standard_tests, pattern='test*.py'):
    # top level directory cached on loader instance
    this_dir = os.path.dirname(__file__)
    package_tests = loader.discover(start_dir=this_dir, pattern=pattern)
    standard_tests.addTests(package_tests)
    return standard_tests

if __name__ == "__main__":
    unittest.main()

then the application will fail with an obscure error message:

======================================================================
ERROR: __main__ (unittest.loader.LoadTestsFailure)
----------------------------------------------------------------------
TypeError: object of type 'NoneType' has no len()

----------------------------------------------------------------------
Ran 1 test in 0.000s

Monkeypatching unittest.loader._make_failed_load_tests to display a stack trace, I got this:

Traceback (most recent call last):
  File "/usr/lib64/python2.7/unittest/loader.py", line 71, in loadTestsFromModule
    return load_tests(self, tests, None)
  File "tester.py", line 15, in load_tests
    package_tests = loader.discover(start_dir=this_dir, pattern=pattern)
  File "/usr/lib64/python2.7/unittest/loader.py", line 204, in discover
    tests = list(self._find_tests(start_dir, pattern))
  File "/usr/lib64/python2.7/unittest/loader.py", line 247, in _find_tests
    if not self._match_path(path, full_path, pattern):
  File "/usr/lib64/python2.7/unittest/loader.py", line 235, in _match_path
    return fnmatch(path, pattern)
  File "/usr/lib64/python2.7/fnmatch.py", line 43, in fnmatch
    return fnmatchcase(name, pat)
  File "/usr/lib64/python2.7/fnmatch.py", line 75, in fnmatchcase
    res = translate(pat)
  File "/usr/lib64/python2.7/fnmatch.py", line 87, in translate
    i, n = 0, len(pat)
TypeError: object of type 'NoneType' has no len()

The error is due to the fact that pattern is passed as None to load_tests, but apparently loader.discover doesn't loke a None pattern.

I would suggest that
a) discover internally translates None to the default of 'test*.py' or
b) the documentation is changed to cater for this common use case, i.e. by including a "pattern is None" case distinction in its load_tests snippet.

In case b) is implemented but not a), it would be nice to have a more expressive error message by catching the error somewhat sooner.

[rikpoggi]

I think the doc should be improved (http://docs.python.org/library/unittest.html#load-tests-protocol), it's not clear how pattern in the example (last one) could not be None.

Changing the discover signature doesn't seem to be an option since the TestLoader.discover doc http://docs.python.org/library/unittest.html#unittest.TestLoader.discover says:

The pattern is deliberately not stored as a loader attribute so that
packages can continue discovery themselves.

[gagern]

Rik, I don't follow your argument on not changing discover. Currently, if code calls discover with pattern=None, there will be an exception. So there cannot be any working code out there which passes pattern=None. Therefore, it should be all right for the implementation to detect the None and replace it by a default. No currently working code should be affected, and new code could be shorter that way. The pattern still wouldn't be stored inside the loader, so the doc there still holds. Only the fallback for None would be stored.

By the way, what's the rationale behind passing the pattern to the load_tests function? Shouldn't each package decide which of its files constitute the test suite, independent of what the parent module used as a filter?

[rikpoggi]

I wasn't trying to make any argument, just thinking that such particular signature was intentional.

Also notice that there might be code that doesn't pass the pattern argument, and fall back on the default value. So a signature change will break their code. I think that the best solution would be to provide a better documentation.

I don't know what's the rational behind all that.

[gagern]

I'm attaching a patch to better explain what I'm suggesting. As you can see, this patch doesn't change the signature of discover, nor does it change the semantics for any code that doesn't pass pattern, or that passes some pattern other than None.

The only change is that now, passing pattern=None is the same as not passing pattern at all. As a result, load_tests might now pass pattern=pattern as the documentation suggests, and still be called with pattern=None without raising an exception.

[voidspace]

So the logic of the "pattern" argument to "load_tests" is that it should not be None when test discovery is loading the __init__.py module of a test package. However, because most patterns will actually *prevent* __init__.py from being loaded by test discovery - this turns out to not be very useful and in all practical cases this argument will be None.

I don't think there are any backward compatibility issues with the real pattern being passed in.

[voidspace]

Also the patch to allow the pattern to be None (and revert to the default pattern in this case) looks good.

[gagern]

Michael wrote: "[…] the real pattern being passed in".
I wonder, what would be "the real pattern"? In the code I originally pasted, the load_tests function would be invoked by loadTestsFromModule (for module __main__). There is nothing file-based about this, so although you could pass a default pattern, it wouldn't be any more or less "real" than passing None. It might be more useful, though.

"most patterns will actually *prevent* __init__.py from being loaded by test discovery - this turns out to not be very useful and in all practical cases this argument will be None."

Not sure I follow there. For the root of the test suite, yes, it will always be None. But for child packages it will be the pattern that led to the discovery of the __init__.py of that package. In all practical cases this will be a string different from both None and the default of 'test*.py', as it has to match the directory name. Most likely it will be what the load_tests function of the parent package passed to its invocation of discover.

[voidspace]

Changing the docs to the following fixes the original reported issue:

def load_tests(loader, standard_tests, pattern):
    # top level directory cached on loader instance
    this_dir = os.path.dirname(__file__)
    pattern = pattern or "test_*.py"
    package_tests = loader.discover(start_dir=this_dir, pattern=pattern)
    standard_tests.addTests(package_tests)
    return standard_tests

(Suggested by Antoine in bpo-16171.)

I think that load_tests not working as documented is a bug and calling discover with pattern=None should work.

[StefanSullivan]

This seems like it directly contradicts the documentation for the discover method:

"If a package (a directory containing a file named __init__.py) is found, the package will be checked for a load_tests function. If this exists then it will be called package.load_tests(loader, tests, pattern)."

I believe this _is_ a functional bug. I have a use case where I have provided a root script with contents along the lines of

    for d in includes:
        test_dir = os.path.join(search_dir, d)
        if test_dir not in excludes:
            test_suite.addTests(test_loader.discover(test_dir, '*.py', search_dir))

and a load_tests function in one of those directory's __init__.py. However, since test discovery does not pass on the pattern, I cannot possibly discover what the pattern should have been.

Furthermore, if I instead invoke test discovery on the command line using:

python -m unittest discover -s /path/to/code -t /path/to/code -p *.py

I again will see a load_tests invoked with pattern=None.

What is the situation where load_tests will be invoked with a non-None pattern?

Is there a workaround for how I'm supposed to get this pattern? Is there a convention or design pattern where I'm supposed to return everything and let the caller filter out the extra tests?

As far as I can tell, the load_tests protocol is only relevant to packages (not modules) by virtue of its placement in the __init__.py file. However, the only way to load tests from a package is via discover, because there is no loadTestsFromPackage method (like there is for loadTestsFromModule). Have I misunderstood the load_tests protocol? Is there some special way to get a pattern?

I feel that if the user really wanted to use their own pattern, being passed a string won't hurt anything, but converse is not true (at least for my use case)