Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #3751 from nicoddemus/collect-file-bug
Workaround for #3742
  • Loading branch information
RonnyPfannschmidt committed Aug 2, 2018
2 parents 7e92930 + 8c9efd8 commit ca04769
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 2 deletions.
4 changes: 3 additions & 1 deletion src/_pytest/main.py
Expand Up @@ -482,6 +482,8 @@ def collect(self):
self.trace.root.indent -= 1

def _collect(self, arg):
from _pytest.python import Package

names = self._parsearg(arg)
argpath = names.pop(0)
paths = []
Expand All @@ -503,7 +505,7 @@ def _collect(self, arg):
root = self._node_cache[pkginit]
else:
col = root._collectfile(pkginit)
if col:
if col and isinstance(col, Package):

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 9, 2018

The return value is actually the return value of pytest_collect_file hook which is always going to be a list (i.e. firstresult == False). That's basically equivalent to "if False:"

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 9, 2018

Member

Oh you are right. This is certainly incorrect, and only "fixed" the bug by disabling package-scoped fixtures it seems.

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 10, 2018

No, only a corner case won't work anymore (which I realized there's no unittest for).
This should work, although it's still ugly as the one above:

                        if col:
                            if isinstance(col[0], Package):
                                root = col[0]
                            self._node_cache[root.fspath] = root

In other words, if the collected file is not a Package (as expected) because some plugin returned their own item type, then leave the root set to "Session".

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 10, 2018

I think the bigger issue here is that there's no separate hook for packages (e.g. pytest_pycollect_makepackage).

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 10, 2018

Member

I agree, see #3771 (comment). That hook is basically sitting there unused in the code, it is just called during collection but the return value is never used. I was wondering about this while debugging this issue, but sadly have not had any time since then to look further into this.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 23, 2018

Member

While investigating #3854, I realized that pytest_collect_file actually receives both files and directories, so it can call pytest_pycollect_makemodule to return a package when it finds a directory, instead of the __init__.py:

def pytest_collect_file(path, parent):
    ext = path.ext
    if path.check(dir=1) and path.join('__init__.py').check(file=1):
        ihook = parent.session.gethookproxy(path)
        return ihook.pytest_pycollect_makemodule(path=path, parent=parent)
    if ext == ".py":
        if not parent.session.isinitpath(path):
            for pat in parent.config.getini("python_files") + ["__init__.py"]:
                if path.fnmatch(pat):
                    break
            else:
                return
        ihook = parent.session.gethookproxy(path)
        return ihook.pytest_pycollect_makemodule(path=path, parent=parent)


def pytest_pycollect_makemodule(path, parent):
    if path.check(dir=1) and path.join('__init__.py').check(file=1):
        return Package(path, parent)
    else:
        return Module(path, parent)

This warrants a bit of exploring, I didn't go further than that because I'm out of time right now, but what do you think @jonozzz?

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 23, 2018

I don't see any directories passed down as the path argument. I think pytest_ignore_collect is the one receiving both files and dirs.

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 24, 2018

Member

argh revisiting this I noticed I had some other changes in the same file which probably affected this. Nevermind, sorry for the noise. 😁

root = col[0]
self._node_cache[root.fspath] = root

Expand Down
10 changes: 10 additions & 0 deletions testing/example_scripts/fixtures/custom_item/conftest.py
@@ -0,0 +1,10 @@
import pytest


class CustomItem(pytest.Item, pytest.File):
def runtest(self):
pass


def pytest_collect_file(path, parent):
return CustomItem(path, parent)
Empty file.
2 changes: 2 additions & 0 deletions testing/example_scripts/fixtures/custom_item/foo/test_foo.py
@@ -0,0 +1,2 @@
def test():
pass
5 changes: 5 additions & 0 deletions testing/python/fixture.py
Expand Up @@ -1618,6 +1618,11 @@ def test_package(one):
reprec = testdir.inline_run()
reprec.assertoutcome(passed=2)

def test_collect_custom_items(self, testdir):
testdir.copy_example("fixtures/custom_item")
result = testdir.runpytest("foo")
result.stdout.fnmatch_lines("*passed*")


class TestAutouseDiscovery(object):
@pytest.fixture
Expand Down
2 changes: 1 addition & 1 deletion testing/test_collection.py
Expand Up @@ -647,7 +647,7 @@ def test_pkgfile(self, testdir):
col = testdir.getnode(config, x)
assert isinstance(col, pytest.Module)
assert col.name == "x.py"
assert col.parent.parent.parent is None
assert col.parent.parent is None
for col in col.listchain():
assert col.config is config

Expand Down

0 comments on commit ca04769

Please sign in to comment.