load_tests chaining into discover from non-discover entry point gets top_level_dir wrong

[rbtcollins]

BPO	22763
Nosy	@rbtcollins, @bitdancer

^{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 = None
closed_at = None
created_at = <Date 2014-10-30.03:13:07.277>
labels = ['tests']
title = 'load_tests chaining into discover from non-discover entry point gets top_level_dir wrong'
updated_at = <Date 2014-11-01.01:32:48.588>
user = 'https://github.com/rbtcollins'

bugs.python.org fields:

activity = <Date 2014-11-01.01:32:48.588>
actor = 'terry.reedy'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation = <Date 2014-10-30.03:13:07.277>
creator = 'rbcollins'
dependencies = []
files = []
hgrepos = []
issue_num = 22763
keywords = []
message_count = 2.0
messages = ['230253', '230268']
nosy_count = 2.0
nosy_names = ['rbcollins', 'r.david.murray']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'test needed'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue22763'
versions = ['Python 3.5']

[rbtcollins]

The following trivial chain-into-discover load_tests:
def load_tests(loader, tests, pattern):
import os.path
# top level directory cached on loader instance
this_dir = os.path.dirname(file)
return loader.discover(start_dir=this_dir, pattern=pattern)

Will work fine when that module was loaded by discover in the first place, but if the entry point was e.g. loader.loadTestsFromModule then it will fail because _top_level_dir starts None and is, if not supplied, inferred from start_dir.

One possible way to improve this would be to check for start_dir being a child of rather than just an exact match of, the elements of sys.path. This isn't complete of course, because individual packages can have their own path. And we also compare with top_level_dir to assess importability - but perhaps we can deprecate/ get rid of the top_level_dir concept entirely - it only has two functional uses: a) making it possible to import things when the path to be discovered isn't on sys.path yet, which could be done by the unittest front end, leaving the core API less complicated, and b) to map from paths back to modules which is complicated by the need to handle namespaces and other packages with multiple directories providing modules. (Because load_tests can chain into other directories that aren't under top_level_dir this same bug can show up in the inverse situation).

Another possibility would be to make a new api 'discover from a package' and split discover's internals into 'find packages' and then a chain call to this new api. I'm in favour of this latter approach because I think it will have less compromises and work better. It might also make it easier for us in fixing the rather ugly thing at the moment where discover('foo.bar') adds foo.bar to sys.path before figuring out that its a module dotted path, not a filesystem path.

[bitdancer]

+1 on the refactoring. I think it makes more conceptual sense.