deprecate pytest_plugins in non-top-level conftest #3230
Conversation
_pytest/config.py
Outdated
| @@ -364,6 +364,9 @@ def _importconftest(self, conftestpath): | |||
| _ensure_removed_sysmodule(conftestpath.purebasename) | |||
| try: | |||
| mod = conftestpath.pyimport() | |||
| if hasattr(mod, 'pytest_plugins') and len(self._conftest_plugins) > 0: | |||
| from _pytest.deprecated import PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST | |||
| self._warn(PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST) | |||
There was a problem hiding this comment.
we should include the relative module location in the error message
i'd appreciate if warnings.warn was used there, but i can understand leaving as is if it seems hard/very time consuming to enable it, i dont what to turn this into a time sink for you
There was a problem hiding this comment.
not a problem at all, I was just worried about its visibility
_pytest/deprecated.py
Outdated
| @@ -50,3 +50,9 @@ class RemovedInPytest4Warning(DeprecationWarning): | |||
| "Metafunc.addcall is deprecated and scheduled to be removed in pytest 4.0.\n" | |||
| "Please use Metafunc.parametrize instead." | |||
| ) | |||
|
|
|||
| PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST = ( | |||
There was a problem hiding this comment.
in case its viable to use warnings.warn, lets please make this a warning object.
otherwise lets have a small todo comment
_pytest/config.py
Outdated
| @@ -364,6 +364,9 @@ def _importconftest(self, conftestpath): | |||
| _ensure_removed_sysmodule(conftestpath.purebasename) | |||
| try: | |||
| mod = conftestpath.pyimport() | |||
| if hasattr(mod, 'pytest_plugins') and len(self._conftest_plugins) > 0: | |||
There was a problem hiding this comment.
@RonnyPfannschmidt is using len(self._conftest_plugins) > 0 to identify the root conftest the correct approach, regardless where the collection started? Just making sure.
There was a problem hiding this comment.
its not, i'll take a while to identify the right approach for the initial contests
There was a problem hiding this comment.
Can you give an example of a situation in which the top level conftest won't be the first collected? I wasn't able to find one.
There was a problem hiding this comment.
@brianmaissy the situation i wondering about is where there is no toplevel conftest to begin with
There was a problem hiding this comment.
Ah I see. Any conftest whose directory is not an ancestor of all collected tests. That would involve keeping track of which conftests used pytest_plugins during collection, and then determining it after collection is finished. Seems complicated.
Or we could say that only a conftest located in the rootdir can use pytest_plugins. But that might be too restrictive.
There was a problem hiding this comment.
So for sure my implementation yields false negatives. You could also say it yields false positives if you consider this directory structure valid:
a/
conftest.py (defining pytest_plugins)
b/
conftest.py (defining pytest_plugins)
test_it.py
There was a problem hiding this comment.
in order to sort it out i believe a check if pytest is configured fits
early conftests are imported and used while the pytest_configure step did not yet happen,
later conftests are imported as part of collection after pytest_configure did happen
brianmaissy
force-pushed
the
features
branch
5 times, most recently
from
8d8bcac
to
6c25af4
Compare
|
Haha finally got that failing test! Today I learned pypy is picky about source file encoding |
There was a problem hiding this comment.
Sorry but I just realized that we need to add a more detailed explanation to the docs, one of the first things users will do when they see this warning is to search the docs for an explanation. Perhaps a note in the section Requiring/Loading plugins in a test module or conftest file of writing_plugins.rst would be best?
|
Yeah sounds good |
|
done, sorry for the delay |
|
Thanks @brianmaissy for following up with the PR! I fixed a small typo, |
|
@RonnyPfannschmidt gentle ping. 😁 |
Fixes #3084