From 670842b7c1b6128d3165a72f9d9a4875019b790d Mon Sep 17 00:00:00 2001 From: Giovana Malaia Date: Sat, 31 Oct 2026 01:53:51 -0300 Subject: [PATCH] improve error message when plugins register conflicting options --- AUTHORS | 1 + changelog/11514.improvement.rst | 1 + src/_pytest/config/__init__.py | 16 +- src/_pytest/config/argparsing.py | 58 ++++++- testing/test_parseopt.py | 260 +++++++++++++++++++++++++++++++ 5 files changed, 332 insertions(+), 4 deletions(-) create mode 100644 changelog/11514.improvement.rst diff --git a/AUTHORS b/AUTHORS index 136ffd6c5ee..4dce7d96d2f 100644 --- a/AUTHORS +++ b/AUTHORS @@ -197,6 +197,7 @@ Gene Wood George Kussumoto Georgy Dyuldin Gergely Kalmár +Giovana Malaia Gleb Nikonorov Graeme Smecher Graham Horler diff --git a/changelog/11514.improvement.rst b/changelog/11514.improvement.rst new file mode 100644 index 00000000000..534522618d1 --- /dev/null +++ b/changelog/11514.improvement.rst @@ -0,0 +1 @@ +The error raised when two plugins register the same command-line option now names the plugins involved when they can be identified, for example ``option names {'--browser'} already added by plugins: ['plugin1', 'plugin2']``, and mentions an unidentified source otherwise. diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index 60ba5f54a89..e1689785ad2 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -529,6 +529,9 @@ def __init__(self) -> None: # just stored here to be used later. self.skipped_plugins: list[tuple[str, str]] = [] + # Parser receiving the options of plugins as they are registered; set by Config. + self._option_parser: Parser | None = None + self.add_hookspecs(_pytest.hookspec) self.register(self) if os.environ.get("PYTEST_DEBUG"): @@ -601,7 +604,17 @@ def register(self, plugin: _PluggyPlugin, name: str | None = None) -> str | None ) ) return None - plugin_name = super().register(plugin, name) + # Nameless plugins without ``__name__`` stay unattributed instead of using + # pluggy's id()-based fallback name. + option_owner = name or getattr(plugin, "__name__", None) + parser = self._option_parser + scope = ( + parser._plugin_scope(option_owner) + if parser is not None + else contextlib.nullcontext() + ) + with scope: + plugin_name = super().register(plugin, name) if plugin_name is not None: self.hook.pytest_plugin_registered.call_historic( kwargs=dict( @@ -1248,6 +1261,7 @@ def __init__( :type: PytestPluginManager """ + pluginmanager._option_parser = self._parser self.stash = Stash() """A place where plugins can store information on the config for their diff --git a/src/_pytest/config/argparsing.py b/src/_pytest/config/argparsing.py index 7180cde2c4c..424e4df7f4c 100644 --- a/src/_pytest/config/argparsing.py +++ b/src/_pytest/config/argparsing.py @@ -3,7 +3,10 @@ import argparse from collections.abc import Callable +from collections.abc import Iterable +from collections.abc import Iterator from collections.abc import Sequence +import contextlib import dataclasses import os import re @@ -138,6 +141,10 @@ def __init__( self._groups = [self._anonymous] # Maps option strings -> dest, e.g. "-V" and "--version" to "version". self._opt2dest: dict[str, str] = {} + # Maps option strings -> name of the plugin that registered them, None if unknown. + self._opt2plugin: dict[str, str | None] = {} + # Name of the plugin whose options are being registered, None if unknown. + self._current_plugin: str | None = None file_or_dir_arg = self.optparser.add_argument(FILE_OR_DIR, nargs="*") file_or_dir_arg.completer = filescompleter # type: ignore @@ -158,6 +165,37 @@ def processoption(self, option: Argument) -> None: if option.dest: self._processopt(option) + @contextlib.contextmanager + def _plugin_scope(self, plugin_name: str | None) -> Iterator[None]: + """Attribute the options added within the block to ``plugin_name``.""" + previous, self._current_plugin = self._current_plugin, plugin_name + try: + yield + finally: + self._current_plugin = previous + + def _conflict_origin( + self, opts: Iterable[str], *, identified_only: bool = False + ) -> str | None: + """Describe who registered ``opts`` and the plugin now conflicting with them. + + Returns None if none of ``opts`` is known to this parser, or, with + ``identified_only``, if no plugin name is known. + """ + wanted = set(opts) + # Dict order is registration order. + owners = [plugin for opt, plugin in self._opt2plugin.items() if opt in wanted] + if not owners: + return None + current = self._current_plugin + names = list(dict.fromkeys(n for n in (*owners, current) if n is not None)) + if not names: + return None if identified_only else "by an unidentified source" + origin = f"by plugins: {names}" + if None in owners or current is None: + origin += " and an unidentified source" + return origin + def getgroup( self, name: str, description: str = "", after: str | None = None ) -> OptionGroup: @@ -455,18 +493,24 @@ def addoption(self, *opts: str, **attrs: Any) -> None: name for opt in self.options for name in opt.names() ) if conflict: - raise ValueError(f"option names {conflict} already added") + origin = self.parser._conflict_origin(conflict) if self.parser else None + suffix = f" {origin}" if origin else "" + raise ValueError(f"option names {conflict} already added{suffix}") if self.parser and "dest" not in attrs: dest = _get_argparse_dest(opts) for group in self.parser._groups: for option in group.options: if option.dest == dest: + origin = self.parser._conflict_origin( + option.names(), identified_only=True + ) + suffix = f" (registered {origin})" if origin else "" raise ValueError( f"option dest {dest!r} already used by " f"{option.names()!r} (this is the option that maps to " f"dest {dest!r}); pass dest={dest!r} explicitly " - "to share the destination" + f"to share the destination{suffix}" ) self._addoption_inner(opts, attrs, allow_reserved=False) @@ -483,12 +527,20 @@ def _addoption_inner( if len(opt) >= 2 and opt[0] == "-" and opt[1].islower(): raise ValueError("lowercase short options are reserved") - action = self._arggroup.add_argument(*opts, **attrs) + try: + action = self._arggroup.add_argument(*opts, **attrs) + except argparse.ArgumentError as exc: + origin = self.parser._conflict_origin(opts) if self.parser else None + if origin is not None: + # Keep the original exception (type, argument_name) and extend it. + exc.message = f"{exc.message} added {origin}" + raise option = Argument(action) self.options.append(option) if self.parser: for name in option.names(): self.parser._opt2dest[name] = option.dest + self.parser._opt2plugin[name] = self.parser._current_plugin self.parser.processoption(option) diff --git a/testing/test_parseopt.py b/testing/test_parseopt.py index 8457cc6e6ba..fae69784970 100644 --- a/testing/test_parseopt.py +++ b/testing/test_parseopt.py @@ -2,6 +2,9 @@ from __future__ import annotations import argparse +import dataclasses +import importlib +import importlib.metadata import locale import os from pathlib import Path @@ -10,6 +13,10 @@ import sys from _pytest.config import argparsing as parseopt +from _pytest.config import Config +from _pytest.config import get_config +from _pytest.config import PluginImportFailure +from _pytest.config import PytestPluginManager from _pytest.config.exceptions import UsageError from _pytest.monkeypatch import MonkeyPatch from _pytest.pytester import Pytester @@ -341,6 +348,259 @@ def test_multiple_metavar_help(self, parser: parseopt.Parser) -> None: assert "--preferences=value1 value2 value3" in help +class TestConflictingOptionOrigin: + """Conflict errors name the plugins that registered the conflicting options.""" + + @staticmethod + def _parseconfig(pytester: Pytester, *plugins: str): + pytester.syspathinsert() + args: list[str] = [] + for plugin in plugins: + args += ["-p", plugin] + return pytester.parseconfig(*args) + + def test_same_group_conflict_names_plugins(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b") + assert str(err.value) == ( + "option names {'--browser'} already added " + "by plugins: ['plugin_a', 'plugin_b']" + ) + + def test_cross_group_conflict_names_plugins(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.getgroup('one').addoption('--browser', dest='one', action='store')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.getgroup('two').addoption('--browser', dest='two', action='store')\n", + ) + with pytest.raises(argparse.ArgumentError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b") + assert str(err.value) == ( + "argument --browser: conflicting option string: --browser " + "added by plugins: ['plugin_a', 'plugin_b']" + ) + + def test_plugin_registering_other_options_is_not_named( + self, pytester: Pytester + ) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.addoption('--other', action='store')\n", + plugin_c="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b", "plugin_c") + assert str(err.value) == ( + "option names {'--browser'} already added " + "by plugins: ['plugin_a', 'plugin_c']" + ) + + def test_alias_conflict_names_plugins(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.addoption('-E', '--example', action='store_true')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.addoption('--example', action='store_true')\n", + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b") + assert str(err.value) == ( + "option names {'--example'} already added " + "by plugins: ['plugin_a', 'plugin_b']" + ) + + def test_conflict_follows_registration_order(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store')\n", + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_b", "plugin_a") + assert str(err.value).endswith("by plugins: ['plugin_b', 'plugin_a']") + + def test_no_conflict_registers_normally(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a="def pytest_addoption(parser):\n" + " parser.addoption('--browser', action='store', default='x')\n", + plugin_b="def pytest_addoption(parser):\n" + " parser.addoption('--other', action='store', default='y')\n", + ) + config = self._parseconfig(pytester, "plugin_a", "plugin_b") + assert config.getoption("browser") == "x" + assert config.getoption("other") == "y" + + def test_unknown_origin_gets_generic_message(self) -> None: + parser = parseopt.Parser(_ispytest=True) + group = parser.getgroup("custom") + group.addoption("--browser", action="store") + with pytest.raises(ValueError) as err: + group.addoption("--browser", action="store") + assert str(err.value) == ( + "option names {'--browser'} already added by an unidentified source" + ) + + @staticmethod + def _plugin_source(*opts: str, group: str | None = None, **attrs: object) -> str: + target = f"parser.getgroup({group!r})" if group else "parser" + return ( + "def pytest_addoption(parser):\n" + f" {target}.addoption(*{opts!r}, **{attrs!r})\n" + ) + + def test_conflict_with_options_owned_by_different_plugins( + self, pytester: Pytester + ) -> None: + pytester.makepyfile( + plugin_a=self._plugin_source("-E", action="store_true"), + plugin_b=self._plugin_source("--example", action="store_true"), + plugin_c=self._plugin_source("-E", "--example", action="store_true"), + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b", "plugin_c") + assert str(err.value).endswith( + "already added by plugins: ['plugin_a', 'plugin_b', 'plugin_c']" + ) + + def test_registration_stops_at_first_conflict(self, pytester: Pytester) -> None: + pytester.makepyfile( + plugin_a=self._plugin_source("--browser"), + plugin_b=self._plugin_source("--browser"), + plugin_c=self._plugin_source("--browser"), + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b", "plugin_c") + assert str(err.value).endswith("by plugins: ['plugin_a', 'plugin_b']") + + def test_cross_group_conflict_keeps_argparse_error_details( + self, pytester: Pytester + ) -> None: + pytester.makepyfile( + plugin_a=self._plugin_source("--browser", group="one", dest="one"), + plugin_b=self._plugin_source("--browser", group="two", dest="two"), + ) + with pytest.raises(argparse.ArgumentError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b") + assert err.value.argument_name == "--browser" + assert err.value.message.startswith("conflicting option string: --browser") + assert str(err.value) == f"argument --browser: {err.value.message}" + assert err.value.message.endswith("added by plugins: ['plugin_a', 'plugin_b']") + + def test_implicit_dest_conflict_across_groups_names_plugins( + self, pytester: Pytester + ) -> None: + pytester.makepyfile( + plugin_a=self._plugin_source("--browser", group="one"), + plugin_b=self._plugin_source("--browser", group="two"), + ) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a", "plugin_b") + message = str(err.value) + assert message.startswith("option dest 'browser' already used by ['--browser']") + assert message.endswith("(registered by plugins: ['plugin_a', 'plugin_b'])") + + def test_conftest_conflict_names_conftest_path(self, pytester: Pytester) -> None: + pytester.makepyfile(plugin_a=self._plugin_source("--browser")) + conftest = pytester.makeconftest(self._plugin_source("--browser")) + with pytest.raises(ValueError) as err: + self._parseconfig(pytester, "plugin_a") + assert str(err.value).endswith(f"by plugins: ['plugin_a', {str(conftest)!r}]") + + def test_entry_point_conflict_names_entry_point( + self, pytester: Pytester, monkeypatch: MonkeyPatch + ) -> None: + @dataclasses.dataclass + class DummyEntryPoint: + name: str + module: str + group: str = "pytest11" + + def load(self): + return importlib.import_module(self.module) + + @dataclasses.dataclass + class DummyDist: + entry_points: object + files: object = () + version: str = "1.0" + + @property + def metadata(self): + return {"name": "dummy"} + + pytester.makepyfile( + plugin_a=self._plugin_source("--browser"), + plugin_b=self._plugin_source("--browser"), + ) + pytester.syspathinsert() + monkeypatch.setattr( + importlib.metadata, + "distributions", + lambda: [DummyDist([DummyEntryPoint("ep_b", "plugin_b")])], + ) + monkeypatch.delenv("PYTEST_DISABLE_PLUGIN_AUTOLOAD", raising=False) + with pytest.raises(PluginImportFailure) as err: + pytester.parseconfig("-p", "plugin_a") + # Entry point load errors are wrapped; the original error is the cause. + assert str(err.value.__cause__).endswith("by plugins: ['plugin_a', 'ep_b']") + + def test_failed_registration_does_not_leak_plugin_name( + self, pytester: Pytester + ) -> None: + pytester.makepyfile( + plugin_a=self._plugin_source("--browser"), + plugin_b=self._plugin_source("--browser"), + ) + pytester.syspathinsert() + config = get_config() + config.pluginmanager.import_plugin("plugin_a") + with pytest.raises(ValueError): + config.pluginmanager.import_plugin("plugin_b") + with pytest.raises(ValueError) as err: + config._parser.addoption("--browser") + assert str(err.value).endswith( + "by plugins: ['plugin_a'] and an unidentified source" + ) + + def test_plugin_registered_before_config_is_not_misattributed(self) -> None: + class Early: + def pytest_addoption(self, parser: parseopt.Parser) -> None: + parser.addoption("--browser") + + pluginmanager = PytestPluginManager() + pluginmanager.register(Early(), "early") + Config(pluginmanager) + with pytest.raises(ValueError) as err: + pluginmanager.register(Early(), "late") + assert str(err.value).endswith( + "by plugins: ['late'] and an unidentified source" + ) + + def test_unnamed_plugin_instance_is_not_given_an_invented_name(self) -> None: + class Plugin: + def pytest_addoption(self, parser: parseopt.Parser) -> None: + parser.addoption("--browser") + + pluginmanager = get_config().pluginmanager + pluginmanager.register(Plugin(), "named") + with pytest.raises(ValueError) as err: + pluginmanager.register(Plugin()) + assert str(err.value).endswith( + "by plugins: ['named'] and an unidentified source" + ) + + def test_argcomplete(pytester: Pytester, monkeypatch: MonkeyPatch) -> None: if sys.version_info >= (3, 11): # New in Python 3.11, ignores utf-8 mode