tox and pytest runs can behave differently when "-p something.conftest" is specified

[hpk42]

Did a little bit of debugging and wanted to share my findings here:

I have an sdist muacrypt which contains a test_muacrypt package and a conftest file inside.
In another project i have a pytest.ini containing addopts = -p test_muacrypt.conftest. This makes all fixtures and command line options available in a normal pytest run. fine.

Problem: When i run tox the fixtures from that plugin are not loaded.
This is surprising from a pure user perspective (tox and pytest runs should not have such differences).

Turns out, the problematic code is in this FixtureManager method:

def pytest_plugin_registered(self, plugin):
    nodeid = None
    try:
        p = py.path.local(plugin.__file__)
    except AttributeError:
        pass
    else:
        # construct the base nodeid which is later used to check
        # what fixtures are visible for particular tests (as denoted
        # by their test id)
        if p.basename.startswith("conftest.py"):
            nodeid = p.dirpath().relto(self.config.rootdir)
            if p.sep != nodes.SEP:
                nodeid = nodeid.replace(p.sep, nodes.SEP)
    self.parsefactories(plugin, nodeid)

in my case, test_muacrypt.conftest has a basename of conftest.py and it's path (inside .tox) is relative to the project root. So the plugin gets a "nodeid" and lookups for fixtures from that plugin fail, because the requesting side will not be a child id of that '.tox/py27/.../conftest.py`. Without tox, Plugins which reside outside a project rootdir will get an empty nodeid and are thus found properly.

Not sure how to fix this in pytest properly yet.

A work aground for me is to put the plugin into test_muacrypt.testing_plugin or so and import everything from the conftest file there.

[pytestbot]

GitMate.io thinks possibly related issues are #2777 (pytest is not running when i run it through tox -- Mac), #2783 (pytest running pyximport?), #273 (plugins entrypoint for plugins that should run before conftest), #2310 (tox needed?), and #3112 (Key errors while running pytest).

[hpk42]

Just a note, my test_muacrypt.testing_plugin workaround did not work because early plugin loading (for "-p" option) is done in such a way that it imports from the tox location because test_muacrypt is a package. When pytest goes to discover the test file in test_muacrypt it will produce ImportMismatchError because test_muacrypt was already loaded from the tox environment. Maybe that is arguably another glitch.

The proper workaround is to put the plugin into muacrypt/testing_plugin.py and -p muacrypt.testing_plugin into the pytest.ini's addopts. This works then.

The original problem is that pytest treats conftest.py files in a special way. I guess we could at least try to avoid this specialty for plugins loaded via "-p" somehow.

[nicoddemus]

I posted a question in the mailing list a few years back, and the conclusion for a workaround was the same.

[hpk42]

i wonder, however, how we could fix this properly so people don't run into this. What do you think about making "-p" force a nodeid of '', i.e. any import path you specified will be a global plugin for the test run? Probably also pytest_plugins=... definitions should behave this way.

The only reason for using -p something.conftest is if you are running tests that are not under something, because if the tests are a file under the something directory, you wouldn't need to add a plugin. If we want to remain fully compatible we could also introduce another option -- maybe a global_pyimport_plugins = import_path1, import_path2, ... setting. It would mean that pytest_plugin_registered would need to receive this enforcing flag (to always use an empty nodeid) somehow.

Maybe with a 4.0 we could see about making "-p" cause empty nodeids, and if it causes troubles, revert it in 4.0.1 (i somehow doubt it's going to cause troubles but who knows).

[nicoddemus]

The only reason for using -p something.conftest is if you are running tests that are not under something, because if the tests are a file under the something directory, you wouldn't need to add a plugin. If we want to remain fully compatible we could also introduce another option -- maybe a global_pyimport_plugins = import_path1, import_path2, ... setting. It would mean that pytest_plugin_registered would need to receive this enforcing flag (to always use an empty nodeid) somehow.

Perhaps the proper solution would be to keep track explicitly of which plugins should be based on location and which plugins should be in the global namespace, instead of relying implicitly on the name conftest.py; this way we can mark conftest.py plugins discovered through collection as "location based", and all others (including those passed through the -p option) as global plugins.

[hpk42]

yes, so far this "keeping track of which plugins are based on location" was done through checking the basename (basically conftest.py files are location dependent, all others are global).

Here is a refined suggestion for what to change user-side:

• declare "-p" and pytest_plugins to always be location-independent (global plugins)
• introduce "preload_conftest = ..." ini setting which leads to loading conftest files early -- you could also use this if you have a deeply nested conftest which defines command line options -- it would just load the conftest.py file earlier than if it is discovered through collection. The value would be a comma-separated list of relative filenames, relative to the directory of where the ini-file is. It's probably a bit difficult to also add a command line option, not sure. that would rather be a filename which is relative to the cwd of the pytest invocation. I think specifying an import path is out of the question -- nested test directories are not required to contain __init__.py files IIRC.

This way, if people used "-p" or pytest_plugins to load a nested conftest early (they could do this i think but it's probably only done rarely -- in none of my projects FWIW) they could use preload_conftest instead which expresses the intent much clearner.

Looking at the code of PytestPluginManager.register i think it's not too difficult (dragons non-withstanding). Adding a new flag to pytest_plugin_registered (..., as_conftest=False|True) or something similar should be doable without too much hassle.

[RonnyPfannschmidt]

we dont need a new flag, we can check if pytest_configure did happen instead

[RonnyPfannschmidt]

also lets please avoid bleeding internals in that way

[hpk42]

@RonnyPfannschmidt I was mostly describing things from the user perspective above. Can't make sense of your comments -- do you e.g. mean to imply with your "check if pytest_configure happened" the behaviour for plugin loading would behave differently before and afterwards? I invested a lot of words to be clear, as did Bruno, please try to be a bit more verbose so i don't have to guess/think so much of what you might mean.

[RonnyPfannschmidt]

bascially we should parse fixtures of all plugins registred outside of the collection loop for all tests, and otherwise do the normal conftest logic for now

if we want to differentiate metadata we should put the calls into more select places instead of side-channeling

[nicoddemus]

@RonnyPfannschmidt IIUC, you mean that we can already separate non-root conftest files from other plugins:

pytest/_pytest/config.py

Lines 372 to 374 in 6f95189

	if hasattr(mod, 'pytest_plugins') and self._configured:
	from _pytest.deprecated import PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST
	warnings.warn(PYTEST_PLUGINS_FROM_NON_TOP_LEVEL_CONFTEST)

So we don't need any extra flag or marker to distinguish between non-local and global conftest.py files, the config object can already track this automatically and provide a (say) _is_preloaded_conftest method which can be used instead of checking for the base name in:

pytest/_pytest/fixtures.py

Lines 997 to 1011 in 6f95189

	def pytest_plugin_registered(self, plugin):
	nodeid = None
	try:
	p = py.path.local(plugin.__file__)
	except AttributeError:
	pass
	else:
	# construct the base nodeid which is later used to check
	# what fixtures are visible for particular tests (as denoted
	# by their test id)
	if p.basename.startswith("conftest.py"):
	nodeid = p.dirpath().relto(self.config.rootdir)
	if p.sep != nodes.SEP:
	nodeid = nodeid.replace(p.sep, nodes.SEP)
	self.parsefactories(plugin, nodeid)

Is that it @RonnyPfannschmidt?

[RonnyPfannschmidt]

correct

[hpk42]

If it's possible to do things automatically then it's of course preferable! I am still a bit skeptical because i think it's unclear that if someone specifies "-p somepackage.conftest" what the precise intention/expectation is, today. It might be for adding a nested plugin to get its command line options. It might be from a different package whose fixtures should become available. At the point of looking at the "-p" option pytest_configure has not happened so i am not sure how it could be used to distinguish the intentions. Or am i missing something?

[RonnyPfannschmidt]

i would go as far as issuing a warning from the pluginmanager itself if anything named conftest is specified as normal plugin, its error prone, confusing, and extraction into a plugin is as trivial as renaming the file, and referring it in a fresh conftest

[nicoddemus]

if anything named conftest is specified as normal plugin, its error prone, confusing, and extraction into a plugin is as trivial as renaming the file, and referring it in a fresh conftest

What is error prone and confusing about having a plugin named conftest.py file, can you please elaborate? When we started using pytest more prominently at work it was very common for people to put their fixtures in conftest.py files and assume referencing that file in a separate project using pytest_plugins = someproject.conftest would work (myself included). I have lost count how many times I had to explain "well you just can't reference conftest.py files that way" and I didn't know the reason until now TBH.

[RonnyPfannschmidt]

conftests are path bound by initial design, plugins aren't

as such we should keep it at that and warn peole when they use the bad pattern

[nicoddemus]

I see, thanks. TBH I'm not entirely convinced, can't we just tell people if they are using -p or pytest_plugins to load contest.py files from other projects then fixtures and hooks there are no longer path bound?

If the answer is no, then I think we should issue a deprecation warning and remove support for -p and pytest_plugins with conftest.py files, otherwise people will just get confused.

[RonnyPfannschmidt]

@nicoddemus we could support it, but to me it seems anything making the logic for that would end up as a fragile mess

although we do have the option to go for changing the registration pattern from being implemented as a hook to being implemented in consider_* methods of pluginmanager

personally i'd like ensure that conftests are local plugins, its relatively easy to provide plugin files after all

and in case of something not wanting to do that, its stilll trivial to import in a local conftest instead of linking it as a plain plugin

lets keep in mind that a large amount of todays maintenance issues are due to not keeping separate concerns separate

[nicoddemus]

we could support it, but to me it seems anything making the logic for that would end up as a fragile mess

I see, thanks. I may have a shallow view, but currently conftest files are special-cased in pytest_plugin_registered by basename only, we could check that the plugin has been loaded as a "global" plugin through the config object, and call self.parsefactories(plugin, None). But we would have to experiment with it.