Fix infinite recursion collection bug with pytest_ignore_collect hook

[nicoddemus]

Adding an example for a bug we found at work. This blows up with:

.env36\lib\site-packages\pluggy\hooks.py:258: in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
.env36\lib\site-packages\pluggy\manager.py:67: in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
.env36\lib\site-packages\pluggy\manager.py:61: in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
src\_pytest\python.py:210: in pytest_collect_file
    return ihook.pytest_pycollect_makemodule(path=path, parent=parent)
.env36\lib\site-packages\pluggy\hooks.py:258: in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
.env36\lib\site-packages\pluggy\manager.py:67: in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
.env36\lib\site-packages\pluggy\manager.py:61: in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
src\_pytest\python.py:215: in pytest_pycollect_makemodule
    return Package(path, parent)
src\_pytest\python.py:552: in __init__
    self, fspath, parent=parent, config=config, session=session, nodeid=nodeid
src\_pytest\nodes.py:351: in __init__
    fspath = py.path.local(fspath)  # xxx only for test_resultlog.py?
.env36\lib\site-packages\py\_path\local.py:158: in __init__
    self.strpath = abspath(path)
w:\Miniconda\envs\py36\lib\ntpath.py:552: in abspath
    return normpath(path)
w:\Miniconda\envs\py36\lib\ntpath.py:494: in normpath
    prefix, path = splitdrive(path)
w:\Miniconda\envs\py36\lib\ntpath.py:142: in splitdrive
    if len(p) >= 2:
E   RecursionError: maximum recursion depth exceeded in comparison
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!

The trigger is the pytest_ignore_collect hook returning False. If it returns None, everything works.

This is a regression because returning False from the hook worked up until 3.7.0.

cc @kfasolin

[RonnyPfannschmidt]

blanket approve, please add a reference to the issue using a mark and mark it as xfail

[nicoddemus]

I plan to tackle it soon. If we get to the point where we decide to merge it, then sure I will mark it as xfail and create a reference issue. 👍

[nicoddemus]

I'm studying a fix for this, and I realize that the current package-scoped fixture should be collecting directories, not files. Currently it collects __init__.py files as Package objects:

pytest/src/_pytest/python.py

Lines 213 to 216 in a76cc8f

	def pytest_pycollect_makemodule(path, parent):
	if path.basename == "__init__.py":
	return Package(path, parent)
	return Module(path, parent)

I believe this is the cause for this bug and the one reported in #3773.

I think we can overhaul pytest_collect_directory (#571) to properly support collecting directories, but this would require some refactoring in the current code.

For the bug in demonstrated in this PR, I have a fix which seems to work but I believe the long term solution is to allow proper collection of directories.

cc @jonozzz

[nicoddemus]

Just pushed the workaround for this PR.

[nicoddemus]

This doesn't have anything to do with the fix, just thought I would keep the return value consistent.

[nicoddemus]

This is the actual fix. I did not understand what this had to do with pytest_ignore_collect though.

[jonozzz]

visit() calls back the self._recurse(path) which in turn calls the pytest_ignore_collect

[coveralls]

Coverage increased (+0.05%) to 92.518% when pulling fe0a76e on nicoddemus:package-infinite-recursion-bug into a76cc8f on pytest-dev:master.

[nicoddemus]

@RonnyPfannschmidt I know you already approved it, but could you take a look at the new commits and tell what you think?

Would appreciate some input from @jonozzz as well

[jonozzz]

Looks good. Thanks for fixing this.

[kfasolin]

Thanks for the fix @nicoddemus =P

[drdavella]

Was this fix released in 3.7.1? This looks very similar to, but not exactly the same as, the issue I reported in scientific-python/pytest-doctestplus#22. Perhaps its the same bug but in a different hook?

[nicoddemus]

@drdavella not yet, this will be released in 3.7.2. Can you try the current master meanwhile?

[jonozzz]

@nicoddemus I didn't even know pytest_collect_directory existed! That would've simplified things a lot... Packages are basically directories with a special init.py file.

[nicoddemus]

@jonozzz if you see the code currently you will notice that pytest_collect_directory cannot do much because its contents are discarded; we need to re-purpose it to be part of the collection. But my gut feeling is the same as yours, it would greatly simplify things.

[nicoddemus]

On second thought, we probably don't want to re-purpose an existing hook, it might be better to deprecate it and add a new hook with the semantics we want to collect a directory.

[nicoddemus]

Got curious and decided to dig a little (although I should be in bed by now), it seems pytest_collect_directory was used at some point in the past to collect Directory nodes: 4a78daf

[drdavella]

@nicoddemus yes the fix works for me on master, thanks.