diff --git a/AUTHORS b/AUTHORS index e2fad5e8364..d97fca162af 100644 --- a/AUTHORS +++ b/AUTHORS @@ -206,6 +206,7 @@ Guido Wesdorp Guoqiang Zhang Hamza Mobeen Harald Armin Massa +Hardik Kaurani Harshna Henk-Jaap Wagenaar Henry Schreiner diff --git a/changelog/15099.bugfix.rst b/changelog/15099.bugfix.rst new file mode 100644 index 00000000000..1fc71bf29f2 --- /dev/null +++ b/changelog/15099.bugfix.rst @@ -0,0 +1 @@ +``monkeypatch.setattr()`` now correctly restores attributes on objects with a custom ``__setattr__``. diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index 453c6728ee2..ca35b15b05d 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -258,13 +258,17 @@ def setattr( # avoid class descriptors like staticmethod/classmethod if inspect.isclass(target): oldval = target.__dict__.get(name, NOTSET) - elif not _is_data_descriptor(type(target), name): - # With no data descriptor in the way, the `setattr()` below writes - # into the instance `__dict__`, so `undo()` has to restore that - # `__dict__` entry. Assigning an inherited `oldval` back onto the - # instance would instead leave behind a new entry shadowing the - # class attribute, which permanently freezes descriptors that - # resolve dynamically (#10644). + elif type(target).__setattr__ is object.__setattr__ and not _is_data_descriptor( + type(target), name + ): + # With no data descriptor in the way and default `object.__setattr__`, + # the `setattr()` below writes into the instance `__dict__`, so + # `undo()` has to restore that `__dict__` entry. Assigning an + # inherited `oldval` back onto the instance would instead leave behind + # a new entry shadowing the class attribute, which permanently freezes + # descriptors that resolve dynamically (#10644). + # When `__setattr__` is overridden, attribute setting is routed through + # custom machinery which may store values elsewhere (#15099). target_dict = getattr(target, "__dict__", None) if isinstance(target_dict, Mapping): oldval = target_dict.get(name, NOTSET) @@ -434,7 +438,10 @@ def undo(self) -> None: if value is not NOTSET: setattr(obj, name, value) else: - delattr(obj, name) + try: + delattr(obj, name) + except AttributeError: + pass self._setattr[:] = [] for dictionary, key, value in reversed(self._setitem): if value is NOTSET: diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index 0d07783b05b..3e5587373c6 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -607,6 +607,98 @@ def __init__(self) -> None: assert obj.x == 1 +def test_undo_custom_setattr_without_delattr() -> None: + """An object with custom __setattr__ but NO __delattr__ should not crash on undo. + + See #15099 maintainer review. Since the object doesn't support deletion, + undo() catches AttributeError and tolerates it, though it cannot physically remove the attribute. + """ + + class ConfigNoDel: + _data: dict[str, object] + + def __init__(self) -> None: + object.__setattr__(self, "_data", {}) + + def __getattr__(self, name: str) -> object: + try: + return self._data[name] + except KeyError: + raise AttributeError(name) from None + + def __setattr__(self, name: str, value: object) -> None: + self._data[name] = value + + cfg = ConfigNoDel() + monkeypatch = MonkeyPatch() + + # Attribute initially absent + assert not hasattr(cfg, "brand_new") + + # Set missing attribute with raising=False + monkeypatch.setattr(cfg, "brand_new", True, raising=False) + assert cfg.brand_new is True + + # Undo should not raise AttributeError, even though delattr(cfg, "brand_new") fails + monkeypatch.undo() + + # Note: We CANNOT assert not hasattr(cfg, "brand_new") here, because without + # a custom __delattr__, monkeypatch has no generic way to remove the attribute + # from the custom storage (_data). The tolerance avoids the teardown crash. + assert cfg.brand_new is True + + +def test_undo_custom_setattr_with_delattr() -> None: + """An object with custom __setattr__ and __delattr__ must restore its old value on undo.""" + + class Config: + """Stores attributes in a private dict instead of __dict__.""" + + _data: dict[str, object] + + def __init__(self) -> None: + object.__setattr__(self, "_data", {"debug": False}) + + def __getattr__(self, name: str) -> object: + try: + return self._data[name] + except KeyError: + raise AttributeError(name) from None + + def __setattr__(self, name: str, value: object) -> None: + self._data[name] = value + + def __delattr__(self, name: str) -> None: + try: + del self._data[name] + except KeyError: + raise AttributeError(name) from None + + cfg = Config() + monkeypatch = MonkeyPatch() + + with pytest.raises(AttributeError, match="has no attribute 'nonexistent'"): + monkeypatch.setattr(cfg, "nonexistent", True) + + # Test setting a missing attribute with raising=False + monkeypatch.setattr(cfg, "nonexistent", True, raising=False) + assert getattr(cfg, "nonexistent") is True + + # Test setting an existing attribute + monkeypatch.setattr(cfg, "debug", True) + assert getattr(cfg, "debug") is True + + monkeypatch.undo() + assert getattr(cfg, "debug") is False + assert not hasattr(cfg, "nonexistent") + + # Test that undoing an already-deleted attribute doesn't raise (the AttributeError tolerance) + monkeypatch.setattr(cfg, "brand_new", True, raising=False) + delattr(cfg, "brand_new") + monkeypatch.undo() + assert not hasattr(cfg, "brand_new") + + def test_issue1338_name_resolving() -> None: pytest.importorskip("requests") monkeypatch = MonkeyPatch()