fix(MonkeyPatch): register undo entry only after a successful mutation (delattr/setitem/delitem) (#14909)

changelog/14909.bugfix.rst

@@ -0,0 +1 @@
+Fixed :meth:`MonkeyPatch.delattr <pytest.MonkeyPatch.delattr>`, :meth:`MonkeyPatch.setitem <pytest.MonkeyPatch.setitem>` and :meth:`MonkeyPatch.delitem <pytest.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 <pytest.MonkeyPatch.setattr>`.

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``.

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):