diff --git a/AUTHORS b/AUTHORS index 748b9bae2..136d10f7e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -159,6 +159,7 @@ Floris Bruynooghe Fraser Stark Gabriel Landau Gabriel Reis +Ganden Schaffner Garvit Shubham Gene Wood George Kussumoto diff --git a/changelog/10644.bugfix.rst b/changelog/10644.bugfix.rst new file mode 100644 index 000000000..15b4019d3 --- /dev/null +++ b/changelog/10644.bugfix.rst @@ -0,0 +1,3 @@ +`monkeypatch.setattr` no longer leaves a leftover item in the target's dictionary after cleanup when patching an inherited attribute of a non-class object. This fixes a bug where a `__get__` descriptor's dynamic lookup was blocked and replaced by a cached static lookup after `monkeypatch` teardown. + +Previously `monkeypatch.setattr` avoided leaving leftover items in the target's dictionary when patching class objects but not when patching non-class objects. diff --git a/changelog/10646.bugfix.rst b/changelog/10646.bugfix.rst new file mode 100644 index 000000000..bfc27bb7c --- /dev/null +++ b/changelog/10646.bugfix.rst @@ -0,0 +1,3 @@ +Fix bug where `monkeypatch.setattr` and `monkeypatch.delattr` bind descriptors (possibly causing unwanted side-effects) to implement their `hasattr` checks even when their checks are disabled by `raising=False`. + +Note that they still must bind descriptors to implement their checks if `raising=True`. diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index 3f398df76..e50021d35 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -222,7 +222,6 @@ class MonkeyPatch: applies to ``monkeypatch.setattr`` as well. """ __tracebackhide__ = True - import inspect if isinstance(value, Notset): if not isinstance(target, str): @@ -241,13 +240,13 @@ class MonkeyPatch: "import string" ) - oldval = getattr(target, name, notset) - if raising and oldval is notset: + if raising and not hasattr(target, name): raise AttributeError(f"{target!r} has no attribute {name!r}") - # avoid class descriptors like staticmethod/classmethod - if inspect.isclass(target): - oldval = target.__dict__.get(name, notset) + # Prevent `undo` from polluting `vars(target)` with an object that was not in it + # before monkeypatching, such as inherited attributes or the results of + # descriptor binding. + oldval = vars(target).get(name, notset) self._setattr.append((target, name, oldval)) setattr(target, name, value) @@ -267,7 +266,6 @@ class MonkeyPatch: ``raising`` is set to False. """ __tracebackhide__ = True - import inspect if isinstance(name, Notset): if not isinstance(target, str): @@ -278,16 +276,18 @@ class MonkeyPatch: ) name, target = derive_importpath(target, raising) - if not hasattr(target, name): - if raising: - raise AttributeError(name) - else: - oldval = getattr(target, name, notset) - # Avoid class descriptors like staticmethod/classmethod. - if inspect.isclass(target): - oldval = target.__dict__.get(name, notset) - self._setattr.append((target, name, oldval)) + if raising and not hasattr(target, name): + raise AttributeError(f"{target!r} has no attribute {name!r}") + + # Prevent `undo` from overwriting class descriptors (like + # staticmethod/classmethod) with the results of descriptor binding. + oldval = vars(target).get(name, notset) + try: delattr(target, name) + except AttributeError: + pass + else: + self._setattr.append((target, name, oldval)) def setitem(self, dic: Mapping[K, V], name: K, value: V) -> None: """Set dictionary entry ``name`` to value.""" diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index 2ad3ccc4d..6a7d61b08 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -6,12 +6,46 @@ import sys import textwrap from typing import Dict from typing import Generator +from typing import NoReturn +from typing import Optional +from typing import overload from typing import Type +from typing import TypeVar from _pytest.monkeypatch import MonkeyPatch from _pytest.pytester import Pytester import pytest +T = TypeVar("T") + + +class SomeDescriptor: + @overload + def __get__(self, instance: None, owner: type) -> int: + ... + + @overload + def __get__(self, instance: T, owner: Optional[Type[T]] = ...) -> int: + ... + + def __get__(self, instance: Optional[T], owner: Optional[Type[T]] = None) -> int: + return 1 + + +class RaisingDescriptor: + @overload + def __get__(self, instance: None, owner: type) -> NoReturn: + ... + + @overload + def __get__(self, instance: T, owner: Optional[Type[T]] = ...) -> NoReturn: + ... + + def __get__( + self, instance: Optional[T], owner: Optional[Type[T]] = None + ) -> NoReturn: + assert False, "descriptor was bound" + @pytest.fixture def mp() -> Generator[MonkeyPatch, None, None]: @@ -23,15 +57,22 @@ def mp() -> Generator[MonkeyPatch, None, None]: def test_setattr() -> None: + import inspect + class A: x = 1 + y = SomeDescriptor() + z = RaisingDescriptor() monkeypatch = MonkeyPatch() pytest.raises(AttributeError, monkeypatch.setattr, A, "notexists", 2) - monkeypatch.setattr(A, "y", 2, raising=False) - assert A.y == 2 # type: ignore + monkeypatch.setattr(A, "w", 2, raising=False) + assert A.w == 2 # type: ignore monkeypatch.undo() - assert not hasattr(A, "y") + assert not hasattr(A, "w") + + with pytest.raises(TypeError): + monkeypatch.setattr(A, "w") # type: ignore[call-overload] monkeypatch = MonkeyPatch() monkeypatch.setattr(A, "x", 2) @@ -45,8 +86,32 @@ def test_setattr() -> None: monkeypatch.undo() # double-undo makes no modification assert A.x == 5 - with pytest.raises(TypeError): - monkeypatch.setattr(A, "y") # type: ignore[call-overload] + # Test that inherited attributes don't get written into the target's instance + # dictionary. + a = A() + monkeypatch = MonkeyPatch() + assert "x" not in vars(a) + monkeypatch.setattr(a, "x", 2) + monkeypatch.undo() + assert "x" not in vars(a) + + for obj in (A, A()): + # Test that class/instance descriptors don't get bound and written into the + # target's dictionary. + monkeypatch = MonkeyPatch() + assert isinstance(inspect.getattr_static(obj, "y"), SomeDescriptor) + monkeypatch.setattr(obj, "y", 2) + monkeypatch.undo() + assert isinstance(inspect.getattr_static(obj, "y"), SomeDescriptor) + + # Test that the `raising=True` check binds descriptors (to check if they raise + # AttributeError). + monkeypatch = MonkeyPatch() + with pytest.raises(AssertionError, match="descriptor was bound"): + monkeypatch.setattr(obj, "z", 2) + # Test that descriptors don't get bound if `raising=False`. + monkeypatch.setattr(obj, "z", 2, raising=False) + monkeypatch.undo() class TestSetattrWithImportPath: @@ -97,8 +162,12 @@ class TestSetattrWithImportPath: def test_delattr() -> None: + import inspect + class A: x = 1 + y = SomeDescriptor() + z = RaisingDescriptor() monkeypatch = MonkeyPatch() monkeypatch.delattr(A, "x") @@ -107,14 +176,31 @@ def test_delattr() -> None: assert A.x == 1 monkeypatch = MonkeyPatch() + pytest.raises(AttributeError, monkeypatch.delattr, A, "w") + monkeypatch.delattr(A, "w", raising=False) monkeypatch.delattr(A, "x") - pytest.raises(AttributeError, monkeypatch.delattr, A, "y") - monkeypatch.delattr(A, "y", raising=False) monkeypatch.setattr(A, "x", 5, raising=False) assert A.x == 5 monkeypatch.undo() assert A.x == 1 + # Test that (non-inherited) class descriptors don't get bound and written into the + # target's dictionary. + monkeypatch = MonkeyPatch() + assert isinstance(inspect.getattr_static(A, "y"), SomeDescriptor) + monkeypatch.delattr(A, "y") + monkeypatch.undo() + assert isinstance(inspect.getattr_static(A, "y"), SomeDescriptor) + + # Test that the `raising=True` check binds descriptors (to check if they raise + # AttributeError). + monkeypatch = MonkeyPatch() + with pytest.raises(AssertionError, match="descriptor was bound"): + monkeypatch.delattr(A, "z") + # Test that descriptor's don't get bound if `raising=False`. + monkeypatch.delattr(A, "z", raising=False) + monkeypatch.undo() + def test_setitem() -> None: d = {"x": 1}