-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
warn on ambiguous -p plugin module usage #14270
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
base: main
Are you sure you want to change the base?
Changes from all commits
f0192b9
6d288ce
9fb25c8
c92d067
c19bda1
7dd52bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Pytest now warns when ``-p`` loads a module with no pytest hooks but a ``pytest11`` entry-point exists in one of its submodules, helping catch wrong plugin names when plugin autoloading is disabled. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -878,8 +878,9 @@ def import_plugin(self, modname: str, consider_entry_points: bool = False) -> No | |
| importspec = "_pytest." + modname if modname in builtin_plugins else modname | ||
| self.rewrite_hook.mark_rewrite(importspec) | ||
|
|
||
| loaded = False | ||
| if consider_entry_points: | ||
| loaded = self.load_setuptools_entrypoints("pytest11", name=modname) | ||
| loaded = bool(self.load_setuptools_entrypoints("pytest11", name=modname)) | ||
| if loaded: | ||
| return | ||
|
|
||
|
|
@@ -900,6 +901,44 @@ def import_plugin(self, modname: str, consider_entry_points: bool = False) -> No | |
| self.skipped_plugins.append((modname, e.msg or "")) | ||
| else: | ||
| self.register(mod, modname) | ||
| if consider_entry_points and not loaded: | ||
| self._warn_about_submodule_entrypoint_plugin(modname, mod) | ||
|
|
||
| def _warn_about_submodule_entrypoint_plugin( | ||
| self, modname: str, mod: _PluggyPlugin | ||
| ) -> None: | ||
| if self._plugin_has_pytest_hooks(mod): | ||
| return | ||
|
|
||
| modname_prefix = f"{modname}." | ||
| 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This warrants a comment: examples of what |
||
| } | ||
|
Comment on lines
+914
to
+922
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit dense, can we break it a bit? |
||
| if not suggested: | ||
| return | ||
|
Comment on lines
+923
to
+924
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should still warn about the plugin not having hooks, even if we don't find a suitable suggestion. |
||
|
|
||
| suggestion = ( | ||
| f"-p {sorted(suggested)[0]}" | ||
| if len(suggested) == 1 | ||
| else "one of: " + ", ".join(f"-p {name}" for name in sorted(suggested)) | ||
| ) | ||
| warnings.warn( | ||
| PytestConfigWarning( | ||
| f'Plugin "{modname}" contains no pytest hooks. Did you mean to use {suggestion}?' | ||
| ), | ||
| stacklevel=3, | ||
| ) | ||
|
|
||
| def _plugin_has_pytest_hooks(self, plugin: _PluggyPlugin) -> bool: | ||
| return any( | ||
| self.parse_hookimpl_opts(plugin, attr) is not None for attr in dir(plugin) | ||
| ) | ||
|
|
||
|
|
||
| def _get_plugin_specs_as_list( | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,9 @@ | |||||
| import re | ||||||
| import sys | ||||||
| import textwrap | ||||||
| import types | ||||||
| from typing import Any | ||||||
| import warnings | ||||||
|
|
||||||
| import _pytest._code | ||||||
| from _pytest.config import _get_plugin_specs_as_list | ||||||
|
|
@@ -30,6 +32,7 @@ | |||||
| from _pytest.monkeypatch import MonkeyPatch | ||||||
| from _pytest.pathlib import absolutepath | ||||||
| from _pytest.pytester import Pytester | ||||||
| from _pytest.warning_types import PytestConfigWarning | ||||||
| from _pytest.warning_types import PytestDeprecationWarning | ||||||
| import pytest | ||||||
|
|
||||||
|
|
@@ -1690,6 +1693,196 @@ def distributions(): | |||||
| # __spec__ is present when testing locally on pypy, but not in CI ???? | ||||||
|
|
||||||
|
|
||||||
| def test_disable_plugin_autoload_warns_for_submodule_entrypoint( | ||||||
| pytester: Pytester, monkeypatch: MonkeyPatch | ||||||
| ) -> None: | ||||||
| class DummyEntryPoint: | ||||||
| project_name = "pytest-recording" | ||||||
| name = "recording" | ||||||
| group = "pytest11" | ||||||
| version = "1.0" | ||||||
| value = "pytest_recording.plugin" | ||||||
|
|
||||||
| class Distribution: | ||||||
| metadata = {"name": "pytest-recording"} | ||||||
| entry_points = (DummyEntryPoint(),) | ||||||
| files = () | ||||||
|
|
||||||
| def distributions(): | ||||||
| return (Distribution(),) | ||||||
|
|
||||||
| top_level_plugin = types.ModuleType("pytest_recording") | ||||||
| submodule_plugin = types.ModuleType("pytest_recording.plugin") | ||||||
| setattr(submodule_plugin, "pytest_addoption", lambda parser: None) | ||||||
|
|
||||||
| monkeypatch.setattr(importlib.metadata, "distributions", distributions) | ||||||
| monkeypatch.setitem(sys.modules, "pytest_recording", top_level_plugin) | ||||||
| monkeypatch.setitem(sys.modules, "pytest_recording.plugin", submodule_plugin) | ||||||
|
|
||||||
| with pytest.warns( | ||||||
| PytestConfigWarning, | ||||||
| match=r'Plugin "pytest_recording" contains no pytest hooks\. Did you mean to use -p recording\?', | ||||||
| ): | ||||||
| config = pytester.parseconfig( | ||||||
| "--disable-plugin-autoload", "-p", "pytest_recording" | ||||||
| ) | ||||||
|
|
||||||
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||||||
|
|
||||||
|
|
||||||
| def test_disable_plugin_autoload_does_not_warn_when_module_has_hooks( | ||||||
| pytester: Pytester, monkeypatch: MonkeyPatch | ||||||
| ) -> None: | ||||||
| class DummyEntryPoint: | ||||||
| project_name = "pytest-recording" | ||||||
| name = "recording" | ||||||
| group = "pytest11" | ||||||
| version = "1.0" | ||||||
| value = "pytest_recording.plugin" | ||||||
|
|
||||||
| class Distribution: | ||||||
| metadata = {"name": "pytest-recording"} | ||||||
| entry_points = (DummyEntryPoint(),) | ||||||
| files = () | ||||||
|
|
||||||
| def distributions(): | ||||||
| return (Distribution(),) | ||||||
|
|
||||||
| plugin_with_hooks = types.ModuleType("pytest_recording") | ||||||
|
|
||||||
| def pytest_addoption(parser): | ||||||
| parser.addoption("--block-network") | ||||||
|
|
||||||
| setattr(plugin_with_hooks, "pytest_addoption", pytest_addoption) | ||||||
|
|
||||||
| monkeypatch.setattr(importlib.metadata, "distributions", distributions) | ||||||
| monkeypatch.setitem(sys.modules, "pytest_recording", plugin_with_hooks) | ||||||
|
|
||||||
| with warnings.catch_warnings(record=True) as captured: | ||||||
| warnings.simplefilter("always") | ||||||
| config = pytester.parseconfig( | ||||||
| "--disable-plugin-autoload", "-p", "pytest_recording" | ||||||
| ) | ||||||
|
|
||||||
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||||||
| assert not [ | ||||||
| w | ||||||
| for w in captured | ||||||
| if isinstance(w.message, PytestConfigWarning) | ||||||
| and "contains no pytest hooks" in str(w.message) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||||||
| ] | ||||||
|
|
||||||
|
|
||||||
| def test_disable_plugin_autoload_does_not_warn_when_no_submodule_entrypoint( | ||||||
| pytester: Pytester, monkeypatch: MonkeyPatch | ||||||
| ) -> None: | ||||||
| class DummyEntryPoint: | ||||||
| project_name = "pytest-recording" | ||||||
| name = "recording" | ||||||
| group = "pytest11" | ||||||
| version = "1.0" | ||||||
| value = "other_plugin.plugin" | ||||||
|
|
||||||
| class Distribution: | ||||||
| metadata = {"name": "pytest-recording"} | ||||||
| entry_points = (DummyEntryPoint(),) | ||||||
| files = () | ||||||
|
|
||||||
| def distributions(): | ||||||
| return (Distribution(),) | ||||||
|
|
||||||
| monkeypatch.setattr(importlib.metadata, "distributions", distributions) | ||||||
| monkeypatch.setitem( | ||||||
| sys.modules, "pytest_recording", types.ModuleType("pytest_recording") | ||||||
| ) | ||||||
|
|
||||||
| with warnings.catch_warnings(record=True) as captured: | ||||||
| warnings.simplefilter("always") | ||||||
| config = pytester.parseconfig( | ||||||
| "--disable-plugin-autoload", "-p", "pytest_recording" | ||||||
| ) | ||||||
|
|
||||||
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||||||
| assert not [ | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as the other test. |
||||||
| w | ||||||
| for w in captured | ||||||
| if isinstance(w.message, PytestConfigWarning) | ||||||
| and "contains no pytest hooks" in str(w.message) | ||||||
| ] | ||||||
|
|
||||||
|
|
||||||
| def test_disable_plugin_autoload_warns_for_multiple_submodule_entrypoints( | ||||||
| pytester: Pytester, monkeypatch: MonkeyPatch | ||||||
| ) -> None: | ||||||
| class DummyEntryPoint: | ||||||
| project_name = "pytest-recording" | ||||||
| group = "pytest11" | ||||||
| version = "1.0" | ||||||
|
|
||||||
| def __init__(self, name: str, value: str) -> None: | ||||||
| self.name = name | ||||||
| self.value = value | ||||||
|
|
||||||
| class Distribution: | ||||||
| metadata = {"name": "pytest-recording"} | ||||||
| entry_points = ( | ||||||
| DummyEntryPoint("recording", "pytest_recording.plugin"), | ||||||
| DummyEntryPoint("recording_alt", "pytest_recording.alt"), | ||||||
| ) | ||||||
| files = () | ||||||
|
|
||||||
| def distributions(): | ||||||
| return (Distribution(),) | ||||||
|
|
||||||
| monkeypatch.setattr(importlib.metadata, "distributions", distributions) | ||||||
| monkeypatch.setitem( | ||||||
| sys.modules, "pytest_recording", types.ModuleType("pytest_recording") | ||||||
| ) | ||||||
|
|
||||||
| with pytest.warns( | ||||||
| PytestConfigWarning, | ||||||
| match=( | ||||||
| r'Plugin "pytest_recording" contains no pytest hooks\. ' | ||||||
| r"Did you mean to use one of: -p recording, -p recording_alt\?" | ||||||
| ), | ||||||
| ): | ||||||
| config = pytester.parseconfig( | ||||||
| "--disable-plugin-autoload", "-p", "pytest_recording" | ||||||
| ) | ||||||
|
|
||||||
| assert config.pluginmanager.get_plugin("pytest_recording") is not None | ||||||
|
|
||||||
|
|
||||||
| def test_warn_about_submodule_entrypoint_plugin_direct(pytester: Pytester, monkeypatch): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow the intent behind this test, could you add a docstring please? |
||||||
| class DummyEntryPoint: | ||||||
| group = "pytest11" | ||||||
| version = "1.0" | ||||||
|
|
||||||
| def __init__(self, name: str, value: str): | ||||||
| self.name = name | ||||||
| self.value = value | ||||||
|
|
||||||
| class Distribution: | ||||||
| metadata = {"name": "pytest-recording"} | ||||||
| entry_points = (DummyEntryPoint("recording", "pytest_recording.plugin"),) | ||||||
| files = () | ||||||
|
|
||||||
| def distributions(): | ||||||
| return (Distribution(),) | ||||||
|
|
||||||
| monkeypatch.setattr(importlib.metadata, "distributions", distributions) | ||||||
| config = pytester.parseconfig() | ||||||
| plugin = types.ModuleType("pytest_recording") | ||||||
|
|
||||||
| with pytest.warns( | ||||||
| PytestConfigWarning, | ||||||
| match=r'Plugin "pytest_recording" contains no pytest hooks\. Did you mean to use -p recording\?', | ||||||
| ): | ||||||
| config.pluginmanager._warn_about_submodule_entrypoint_plugin( | ||||||
| "pytest_recording", plugin | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def test_plugin_loading_order(pytester: Pytester) -> None: | ||||||
| """Test order of plugin loading with `-p`.""" | ||||||
| p1 = pytester.makepyfile( | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's inline this: