From a88e91bacaa98b68fe80e07a138ce40994bd8797 Mon Sep 17 00:00:00 2001 From: sharonyao1127 Date: Wed, 19 Aug 2026 23:45:03 +0800 Subject: [PATCH 1/2] fix(MonkeyPatch): register undo entry only after a successful mutation (delattr/setitem/delitem) (#14909) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> --- changelog/14909.bugfix.rst | 1 + src/_pytest/monkeypatch.py | 8 +++--- testing/test_monkeypatch.py | 53 +++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 changelog/14909.bugfix.rst 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..7dc01eb3874 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,58 @@ 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() + + +@pytest.mark.parametrize("make_mapping", [dict]) +def test_setitem_delitem_oldval_captured_before_mutation( + monkeypatch: MonkeyPatch, make_mapping +) -> 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): From 2802b66bb562342be2c0f9bfb3920ed761569ff6 Mon Sep 17 00:00:00 2001 From: sharonyao1127 Date: Tue, 25 Aug 2026 15:01:16 +0800 Subject: [PATCH 2/2] test(monkeypatch): simplify old-value regression test --- testing/test_monkeypatch.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/testing/test_monkeypatch.py b/testing/test_monkeypatch.py index 7dc01eb3874..63e145c66d4 100644 --- a/testing/test_monkeypatch.py +++ b/testing/test_monkeypatch.py @@ -232,9 +232,8 @@ def test_failed_delitem(monkeypatch: MonkeyPatch) -> None: monkeypatch.undo() -@pytest.mark.parametrize("make_mapping", [dict]) def test_setitem_delitem_oldval_captured_before_mutation( - monkeypatch: MonkeyPatch, make_mapping + monkeypatch: MonkeyPatch, ) -> None: """For setitem/delitem the old value must be captured *before* the mutation so undo() restores the correct value (#14909). This case