From b1b4469ae6bccf4f714a7477ba245290def48346 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Sun, 8 Jan 2023 00:00:00 -0800 Subject: [PATCH 1/6] 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/6] 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/6] 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 From 1b53b0083b2dadb1b5d69a8ab3c2e389886f253f Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 9 Jan 2023 00:00:00 -0800 Subject: [PATCH 4/6] Sync MonkeyPatch.delattr and setattr AttributeError messages --- src/_pytest/monkeypatch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index fcf8f6fdb..dc255357e 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -281,7 +281,7 @@ class MonkeyPatch: if not hasattr(target, name): if raising: - raise AttributeError(name) + raise AttributeError(f"{target!r} has no attribute {name!r}") else: oldval = getattr(target, name, notset) # Avoid class descriptors like staticmethod/classmethod. From d5f04b526bfbe7ae630c00bbbc808bd108bd6ae5 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 9 Jan 2023 00:00:00 -0800 Subject: [PATCH 5/6] Test MonkeyPatch.setattr/delattr descriptor checking and short-circuiting --- testing/test_monkeypatch.py | 40 +++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index 612c5b356..dd78d5acf 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -5,6 +5,7 @@ import textwrap from pathlib import Path from typing import Dict from typing import Generator +from typing import NoReturn from typing import Optional from typing import overload from typing import Type @@ -30,6 +31,21 @@ class SomeDescriptor: 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]: cwd = os.getcwd() @@ -45,6 +61,7 @@ def test_setattr() -> None: class A: x = 1 y = SomeDescriptor() + z = RaisingDescriptor() monkeypatch = MonkeyPatch() pytest.raises(AttributeError, monkeypatch.setattr, A, "notexists", 2) @@ -77,15 +94,24 @@ def test_setattr() -> None: 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()): + # 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: def test_string_expression(self, monkeypatch: MonkeyPatch) -> None: @@ -140,6 +166,7 @@ def test_delattr() -> None: class A: x = 1 y = SomeDescriptor() + z = RaisingDescriptor() monkeypatch = MonkeyPatch() monkeypatch.delattr(A, "x") @@ -164,6 +191,15 @@ def test_delattr() -> None: 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} From 82557504ba98439e5c2991ec894e115c65888345 Mon Sep 17 00:00:00 2001 From: Ganden Schaffner Date: Mon, 9 Jan 2023 00:00:00 -0800 Subject: [PATCH 6/6] Fix MonkeyPatch.setattr/delattr binding descriptors when raising=False --- changelog/10646.bugfix.rst | 3 +++ src/_pytest/monkeypatch.py | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) create mode 100644 changelog/10646.bugfix.rst 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 dc255357e..47fb4351f 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -241,8 +241,7 @@ 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}") # Prevent `undo` from polluting `vars(target)` with an object that was not in it @@ -268,7 +267,6 @@ class MonkeyPatch: ``raising`` is set to False. """ __tracebackhide__ = True - import inspect if isinstance(name, Notset): if not isinstance(target, str): @@ -279,16 +277,18 @@ class MonkeyPatch: ) name, target = derive_importpath(target, raising) - if not hasattr(target, name): - if raising: - raise AttributeError(f"{target!r} has no attribute {name!r}") - 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: MutableMapping[K, V], name: K, value: V) -> None: """Set dictionary entry ``name`` to value."""