New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pytest_ignore_collect applies to all directories collected afterwards? #2016

Closed
d-b-w opened this Issue Oct 20, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@d-b-w

d-b-w commented Oct 20, 2016

I put a pytest_ignore_collect() in a conftest.py. From the documentation, I had expected it to apply to all subdirectories of the one in which the conftest.py resides. However, it also prevented collection of files that were not in subdirectories!

I have this directory structure:

demo_parent/childa/conftest.py
demo_parent/childa/demo_test.py
demo_parent/childb/demo_test.py

demo_test.py are both def test(): pass. When I add def pytest_ignore_collect(path, config): return True to the conftest.py, neither of the tests are executed! I would have expected childb/demo_test.py to run.

platform darwin -- Python 2.7.10, pytest-2.8.7, py-1.4.31, pluggy-0.3.1
plugins: hypothesis-3.4.0, cov-2.2.1, faulthandler-1.3.0, xdist-1.14

also:
execnet==1.4.1
hypothesis==3.4.0
coverage==4.0.3

I've observed this on Linux, and Mac.

@d-b-w

This comment has been minimized.

d-b-w commented Oct 20, 2016

In my experience, other pytest hooks apply to subdirectories only, not um sibling directories?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Oct 20, 2016

From the documentation, I had expected it to apply to all subdirectories of the one in which the conftest.py resides.

The docs for pytest_ignore_collect() say:

    return True to prevent considering this path for collection.
    This hook is consulted for all files and directories prior to calling
    more specific hooks.

IMHO nothing there hints that it only applies for subdirectories of conftest.py files.

In my experience, other pytest hooks apply to subdirectories only, not um sibling directories?

Could you point other hooks which exhibit this behavior?

TBH I'm not sure it is worth changing this because it would break existing plugins and that can be circumvented easily on the hook's code.

@d-b-w

This comment has been minimized.

d-b-w commented Oct 20, 2016

Thanks, Bruno-

The docs say for conftest say "local conftest.py plugins contain directory-specific hook implementations", which implies to me that conftest.py hooks operate on a specific directory, rather than the whole py.test process. ( http://pytest.org/2.2.4/plugins.html#conftest-py-local-per-directory-plugins).

However, I see this now: "Session and test running activities will invoke all hooks defined in conftest.py files closer to the root of the filesystem". I suppose that this implies that other hooks are invoked differently? Maybe as they are discovered?

Could you point other hooks which exhibit this behavior?

pytest_collect_file and pytest_itemcollected both operate on subdirectories, not sibling directories.

Another facet of this oddity - If the directory looks like this-

demo_parent/child0/demo_test.py
demo_parent/childa/conftest.py
demo_parent/childa/demo_test.py
demo_parent/childb/demo_test.py

The tests in child0 are run, but those in childa and childb are not. I feel like either pytest_ignore_collect() should operate on all directories, or it should operate on the subdirectories. (I totally understand that my instinct may not match broader needs, and it may not be technically feasible, I just wanted to mention what I would see as consistent behavior).

@d-b-w

This comment has been minimized.

d-b-w commented Nov 16, 2016

Actually, I think that this may be a bug. I think that the cache here:

self._fs2hookproxy[fspath] = proxy
is problematic. I'll do some more testing and then report back.

@d-b-w

This comment has been minimized.

d-b-w commented Nov 17, 2016

When I disabled the cache this issue was fixed and no tests failed. Furthermore, pytest tests took the same amount of time!

I see that the cache was added in commit 3de715ec13a7c80b2, but there is no linked issue so I don't know if there was a specific performance target that was intended.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 21, 2016

Hi @d-b-w, sorry for the silence on this.

The docs say for conftest say "local conftest.py plugins contain directory-specific hook implementations", which implies to me that conftest.py hooks operate on a specific directory, rather than the whole py.test process.

I think the wording here can be improved. What pytest does is that contains for each path a list of active conftest.py files, which are searched upwards in the FS hierarchy until reaching the root of the session. So your assumption that your pytest_ignore_collect should only prevent tests from below it to be collected is right and it shouldn't call that hook for the sibling directory.

I created a similar structure:

demo0/test_foo1.py

demoa/test_foo2.py
demoa/conftest.py  # contains pytest_ignore_collect() -> True

demob/test_foo3.py

If I execute py.test, it only executes test_foo1. If I comment the cache you mention then it correctly executes test_foo1 and test_foo3.

I will continue investigating... thanks for the patience on this!

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 21, 2016

I think I now understand what's going on.

You may have noticed that one of the things gethookproxy() does is to cache an "ihook" object for a given path. The "ihook" object can be either the "complete one" with all hooks of all plugins and conftests, or a subset for the cases of subdirectories with their own conftest.py files. The latter is handled by creating a temporary object which only calls hooks from conftests belonging to that directory or upwards. It does so by removing the plugins that should not participate in calls for that path:

            remove_mods = pm._conftest_plugins.difference(my_conftestmodules)
            if remove_mods:
                # one or more conftests are not in use at this fspath
                proxy = FSHookProxy(fspath, pm, remove_mods)
            else:
                # all plugis are active for this fspath
                proxy = self.config.hook            

The problem lies with the "complete proxy" of the else: clause.

Using the example similar to the above:

root/demo-0/test_foo1.py

root/demo-a/test_foo2.py
root/demo-a/conftest.py  # contains pytest_ignore_collect() -> True

root/demo-b/test_foo3.py
  1. When root starts collection, it calls hook = gethookproxy("root") in order to call pytest_ignore_collect() and other hooks.
  2. Since this is the very first object, it gets the "complete proxy", which is cached.
  3. The call to pytest_ignore_collect is done initially using the path of the parent, and not of the path being collected; this is by design so an entire directory can be skipped on a single call:
  def _recurse(self, path):
      ihook = self.gethookproxy(path.dirpath())
      if ihook.pytest_ignore_collect(path=path, config=self.config):
          return
      for pat in self._norecursepatterns:
          if path.check(fnmatch=pat):
              return False
      ihook = self.gethookproxy(path)
      ihook.pytest_collect_directory(path=path, parent=self)
      return True
  1. Then it tries to collect root/demo-0 by using self.gethookproxy('root') and loads the test in there normally (at this point we don't have our hook loaded yet).
  2. Then it collects root/demo-a and now our new conftest is loaded, and accessible from the "complete proxy" which is the same one cached.
  3. And now we have a problem for every path from now one that is collectible from 'root', because it contains our dummy hook implementation which ignores all directories.

I think the solution to this is the one you proposed on the mailing list:

Another option would be to clear the session's cache each time a new conftest is added

This is simple to implement and I already have a working prototype.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Nov 21, 2016

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Nov 22, 2016

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Dec 2, 2016

This was referenced Mar 5, 2018

mozillazg pushed a commit to mozillazg/pypy that referenced this issue Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment