diff --git a/changelog/762.bugfix.rst b/changelog/762.bugfix.rst new file mode 100644 index 00000000..83607a41 --- /dev/null +++ b/changelog/762.bugfix.rst @@ -0,0 +1,4 @@ +``PluginManager.unregister()`` now raises ``TypeError`` when neither a plugin nor a +name is given, and ``ValueError`` when an unregistered plugin is given without a +name, including under ``python -O``. Code catching ``AssertionError`` for these +cases must catch the new exception types instead. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 4994c867..2276b15a 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -283,12 +283,19 @@ def unregister( The plugin can be specified either by the plugin object or the plugin name. If both are specified, they must agree. - Returns the unregistered plugin, or ``None`` if not found. + Returns the unregistered plugin. When only a name is given, returns + ``None`` if the name is not found or is blocked. + + Raises :exc:`TypeError` if neither argument is given, and + :exc:`ValueError` if only a plugin object is given but it is not + registered, or if the plugin object and name do not agree. """ if name is None: - assert plugin is not None, "one of name or plugin needs to be specified" + if plugin is None: + raise TypeError("one of name or plugin needs to be specified") name = self.get_name(plugin) - assert name is not None, "plugin is not registered" + if name is None: + raise ValueError("plugin is not registered") if plugin is None: plugin = self.get_plugin(name) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 4be3dc9a..0d98242b 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -3,6 +3,9 @@ """ import importlib.metadata +import subprocess +import sys +import textwrap import types from typing import Any from typing import cast @@ -474,10 +477,58 @@ def he_method1(self): ... # pragma: no cover assert not pm.is_registered(my) assert pm.get_plugins() == {my2} - with pytest.raises(AssertionError, match=r"not registered"): + with pytest.raises(ValueError, match=r"not registered"): pm.unregister(my) +@pytest.mark.parametrize( + "kwargs", [{}, {"plugin": None}, {"name": None}, {"plugin": None, "name": None}] +) +def test_unregister_requires_argument( + pm: PluginManager, kwargs: dict[str, Any] +) -> None: + with pytest.raises(TypeError, match="one of name or plugin needs to be specified"): + pm.unregister(**kwargs) + assert pm.list_name_plugin() == [] + + +@pytest.mark.parametrize("optimization", [0, 1, 2]) +def test_unregister_validation_with_optimization(optimization: int) -> None: + result = subprocess.run( + [ + sys.executable, + *["-O"] * optimization, + "-c", + textwrap.dedent( + """\ + from pluggy import PluginManager + + pm = PluginManager("example") + for kwargs in ({}, {"plugin": object()}): + try: + pm.unregister(**kwargs) + except (TypeError, ValueError) as exc: + print(f"{type(exc).__name__}: {exc}") + """ + ), + ], + check=True, + capture_output=True, + text=True, + ) + assert result.stdout.splitlines() == [ + "TypeError: one of name or plugin needs to be specified", + "ValueError: plugin is not registered", + ] + + +def test_unregister_unknown_name(pm: PluginManager) -> None: + assert pm.unregister(name="unknown") is None + plugin = object() + assert pm.unregister(plugin, name="unknown") is plugin + assert pm.list_name_plugin() == [] + + def test_unregister_blocked(pm: PluginManager) -> None: class Plugin: pass @@ -485,9 +536,11 @@ class Plugin: p = Plugin() pm.set_blocked("error") pm.register(p, "error") - # bloked plugins can be unregistred many times atm - pm.unregister(p, "error") - pm.unregister(p, "error") + # Blocked plugins can be unregistered many times. + assert pm.unregister(p, "error") is p + assert pm.unregister(p, "error") is p + assert pm.unregister(name="error") is None + assert pm.is_blocked("error") def test_register_unknown_hooks(pm: PluginManager) -> None: