From b1b4469ae6bccf4f714a7477ba245290def48346 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Sun, 8 Jan 2023 00:00:00 -0800 Subject: [PATCH 1/3] Test for MonkeyPatch.setattr/delattr polluting vars(target) --- testing/test_monkeypatch.py | 64 +++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index b32e67bd7..612c5b356 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -5,12 +5,30 @@ import textwrap from pathlib import Path from typing import Dict from typing import Generator +from typing import Optional +from typing import overload from typing import Type +from typing import TypeVar import pytest from _pytest.monkeypatch import MonkeyPatch from _pytest.pytester import Pytester +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 + @pytest.fixture def mp() -> Generator[MonkeyPatch, None, None]: @@ -22,15 +40,21 @@ def mp() -> Generator[MonkeyPatch, None, None]: def test_setattr() -> None: + import inspect + class A: x = 1 + y = SomeDescriptor() 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) @@ -44,8 +68,23 @@ 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) + + # Test that class/instance descriptors don't get bound and written into the target's + # dictionary. + for obj in (A, A()): + 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) class TestSetattrWithImportPath: @@ -96,8 +135,11 @@ class TestSetattrWithImportPath: def test_delattr() -> None: + import inspect + class A: x = 1 + y = SomeDescriptor() monkeypatch = MonkeyPatch() monkeypatch.delattr(A, "x") @@ -106,14 +148,22 @@ 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) + def test_setitem() -> None: d = {"x": 1} From 054b335366b2e939f1d619473adbcd76ffeca639 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Sun, 8 Jan 2023 00:00:00 -0800 Subject: [PATCH 2/3] Fix MonkeyPatch.setattr polluting vars(target) --- changelog/10644.bugfix.rst | 3 +++ src/_pytest/monkeypatch.py | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 changelog/10644.bugfix.rst 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/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index c6e29ac76..fcf8f6fdb 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -223,7 +223,6 @@ class MonkeyPatch: applies to ``monkeypatch.setattr`` as well. """ __tracebackhide__ = True - import inspect if isinstance(value, Notset): if not isinstance(target, str): @@ -246,9 +245,10 @@ class MonkeyPatch: if raising and oldval is notset: 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) From a43a7adddb37cfdc6fde23e7c2aa7b092a5baa5d Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Sun, 8 Jan 2023 00:00:00 -0800 Subject: [PATCH 3/3] Add Ganden Schaffner to AUTHORS --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index a4ca99267..a1753a994 100644 --- a/AUTHORS +++ b/AUTHORS @@ -134,6 +134,7 @@ Florian Dahlitz Floris Bruynooghe Gabriel Landau Gabriel Reis +Ganden Schaffner Garvit Shubham Gene Wood George Kussumoto