warn on ambiguous -p plugin module usage#14270
warn on ambiguous -p plugin module usage#14270kartikdp wants to merge 6 commits intopytest-dev:mainfrom
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
kartikdp
force-pushed
the
bugfix/pluginarg-invalid-entrypoint
branch
from
4c67d0b to
f0192b9
Compare
| if self._plugin_has_pytest_hooks(mod): | ||
| return |
There was a problem hiding this comment.
Let's inline this:
| if self._plugin_has_pytest_hooks(mod): | |
| return | |
| plugin_has_pytest_hooks = any( | |
| self.parse_hookimpl_opts(plugin, attr) is not None for attr in dir(plugin) | |
| ) | |
| if plugin_has_pytest_hooks: | |
| return |
| if not suggested: | ||
| return |
There was a problem hiding this comment.
I think we should still warn about the plugin not having hooks, even if we don't find a suitable suggestion.
| suggested = { | ||
| ep.name | ||
| for dist in importlib.metadata.distributions() | ||
| for ep in dist.entry_points | ||
| if ep.group == "pytest11" | ||
| and ep.name != modname | ||
| and isinstance(getattr(ep, "value", None), str) | ||
| and getattr(ep, "value").split(":", 1)[0].startswith(modname_prefix) | ||
| } |
There was a problem hiding this comment.
This is a bit dense, can we break it a bit?
ep_values = ...
suggested = ...
| if ep.group == "pytest11" | ||
| and ep.name != modname | ||
| and isinstance(getattr(ep, "value", None), str) | ||
| and getattr(ep, "value").split(":", 1)[0].startswith(modname_prefix) |
There was a problem hiding this comment.
This warrants a comment: examples of what ep.value might be and what we are trying to match with this split() call here with : (the tests use . as separator).
| w | ||
| for w in captured | ||
| if isinstance(w.message, PytestConfigWarning) | ||
| and "contains no pytest hooks" in str(w.message) |
There was a problem hiding this comment.
This check is a bit brittle in case we tweak the message later. Could we instead just check that there are no warnings at all?
| ) | ||
|
|
||
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||
| assert not [ |
There was a problem hiding this comment.
Same comment as the other test.
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||
|
|
||
|
|
||
| def test_warn_about_submodule_entrypoint_plugin_direct(pytester: Pytester, monkeypatch): |
There was a problem hiding this comment.
| def test_warn_about_submodule_entrypoint_plugin_direct(pytester: Pytester, monkeypatch): | |
| def test_warn_about_submodule_entrypoint_plugin_direct(pytester: Pytester, monkeypatch: MonkeyPatch) -> None: |
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||
|
|
||
|
|
||
| def test_warn_about_submodule_entrypoint_plugin_direct(pytester: Pytester, monkeypatch): |
There was a problem hiding this comment.
Not sure I follow the intent behind this test, could you add a docstring please?
2 participants
Summary
-p <name>can silently do the wrong thing with--disable-plugin-autoloadwhen
<name>is an importable top-level module that has no pytest hooks,while the real plugin is exposed via a
pytest11entry-point submodule.This change adds a focused warning in that case:
pytest11entry-point exists in one of its submodulesPytestConfigWarningsuggesting the entry-point name (forexample
-p recording)closes #14135
Impact
This improves correctness and UX for plugin selection, especially in autoload-
disabled runs, by surfacing likely misconfiguration instead of failing
silently.
How tested
testing/ test_config.py::test_disable_plugin_autoload_warns_for_submodule_entrypointuv run -m pytest testing/test_config.py -k "disable_plugin_autoload_warns_for_submodule_entrypoint or test_disable_plugin_autoload"uv run -m pytest testing/test_pluginmanager.py -k "import_plugin_importname or import_plugin_dotted_name"uv run --with pre-commit pre-commit run --files src/_pytest/config/ __init__.py testing/test_config.py changelog/14135.bugfix.rstChecklist
no new feature)
closes #XYZWto PR description and/or commits.Co-authored-bycommittrailers.
changelogdirectory.AUTHORSin alphabetical order. (Not needed for thischange)