diff --git a/changelog/14909.bugfix.rst b/changelog/14909.bugfix.rst new file mode 100644 index 00000000000..32dced31386 --- /dev/null +++ b/changelog/14909.bugfix.rst @@ -0,0 +1 @@ +Fixed :meth:`MonkeyPatch.delattr `, :meth:`MonkeyPatch.setitem ` and :meth:`MonkeyPatch.delitem ` recording a stale undo entry when the underlying mutation failed, which could cause spurious errors during teardown. The undo entry is now recorded only after the mutation succeeds, matching the behavior of :meth:`MonkeyPatch.setattr `. diff --git a/src/_pytest/monkeypatch.py b/src/_pytest/monkeypatch.py index d6db72455a8..9ae9e2ba9c8 100644 --- a/src/_pytest/monkeypatch.py +++ b/src/_pytest/monkeypatch.py @@ -282,14 +282,15 @@ def delattr( # Avoid class descriptors like staticmethod/classmethod. if inspect.isclass(target): oldval = target.__dict__.get(name, NOTSET) - self._setattr.append((target, name, oldval)) delattr(target, name) + self._setattr.append((target, name, oldval)) def setitem(self, dic: Mapping[K, V], name: K, value: V) -> None: """Set dictionary entry ``name`` to value.""" - self._setitem.append((dic, name, dic.get(name, NOTSET))) + oldval = dic.get(name, NOTSET) # Not all Mapping types support indexing, but MutableMapping doesn't support TypedDict dic[name] = value # type: ignore[index] + self._setitem.append((dic, name, oldval)) def delitem(self, dic: Mapping[K, V], name: K, raising: bool = True) -> None: """Delete ``name`` from dict. @@ -301,9 +302,10 @@ def delitem(self, dic: Mapping[K, V], name: K, raising: bool = True) -> None: if raising: raise KeyError(name) else: - self._setitem.append((dic, name, dic.get(name, NOTSET))) + oldval = dic.get(name, NOTSET) # Not all Mapping types support indexing, but MutableMapping doesn't support TypedDict del dic[name] # type: ignore[attr-defined] + self._setitem.append((dic, name, oldval)) def setenv(self, name: str, value: str, prepend: str | None = None) -> None: """Set environment variable ``name`` to ``value``. diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index 04b16a1e8c2..63e145c66d4 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -7,6 +7,7 @@ import re import sys import textwrap +from types import MappingProxyType import warnings from _pytest.monkeypatch import MonkeyPatch @@ -196,6 +197,57 @@ def test_delitem() -> None: assert d == {"hello": "world", "x": 1} +def test_failed_delattr(monkeypatch: MonkeyPatch) -> None: + """If delattr() raises, no stale undo entry should be recorded (#14909).""" + + class A: + __slots__ = () + x = 1 + + a = A() + with pytest.raises(AttributeError): + monkeypatch.delattr(a, "x") + assert a.x == 1 + # undo() must not raise — no entry should be on the undo stack. + monkeypatch.undo() + + +def test_failed_setitem(monkeypatch: MonkeyPatch) -> None: + """If setitem() raises, no stale undo entry should be recorded (#14909).""" + mapping = MappingProxyType({"x": 1}) + with pytest.raises(TypeError): + monkeypatch.setitem(mapping, "x", 2) + assert mapping["x"] == 1 + # undo() must not raise — no entry should be on the undo stack. + monkeypatch.undo() + + +def test_failed_delitem(monkeypatch: MonkeyPatch) -> None: + """If delitem() raises, no stale undo entry should be recorded (#14909).""" + mapping = MappingProxyType({"x": 1}) + with pytest.raises(TypeError): + monkeypatch.delitem(mapping, "x") + assert mapping["x"] == 1 + # undo() must not raise — no entry should be on the undo stack. + monkeypatch.undo() + + +def test_setitem_delitem_oldval_captured_before_mutation( + monkeypatch: MonkeyPatch, +) -> None: + """For setitem/delitem the old value must be captured *before* the + mutation so undo() restores the correct value (#14909). This case + exercises both the capture-before line and the append-after line in + the success path (no exception), so codecov patch coverage for the + new lines stays 100% even when the surrounding pytest suite changes. + """ + inner: dict[str, int] = {"x": 1, "y": 2} + monkeypatch.setitem(inner, "x", 99) + monkeypatch.delitem(inner, "y") + monkeypatch.undo() + assert inner == {"x": 1, "y": 2} + + def test_setenv() -> None: monkeypatch = MonkeyPatch() with pytest.warns(pytest.PytestWarning):