Monkeypatch.delitem/delattr does not keep track of non-existing properties

[joaoe]

Hi.

The issue

So if I pass an object that lacks a property or dict that lacks an item to delattr and delitem respectively, by default those methods fail with an Exception.
So by setting raising=False, the exception is skipped. But the monkeypatch object does not record that the object originally lacked the property.
If after the unit test adds the attribute or item, monkeypatch will not restore the object original state, by deleted the attribute or item added by the test.

Here's code examples that show the issue in detail.

# 1. Not an issue

obj = {1:2}
with pytest.Monkeypatch.context() as mp:
    mp.delitem(obj, 1)
    obj[1] = 3

assert obj[1] == 2 # PASS ! the delitem operation was reversed and the original value was restored

# 2. This is an issue

obj = {}
with pytest.Monkeypatch.context() as mp:
    mp.delitem(obj, 1, raising=False)
    obj[1] = 3

# FAIL ! the delitem operation was not reversed since the original object did not have the property
# and the mp object did not record a NOTSET value
assert 1 not in obj

This applies equally to attributes, e.g. obj = types.SimpleNamespace() and change obj[1] to obj.prop.

The solution

diff --git a/monkeypatch.py b/monkeypatch.py
index 1285e57..92bf43e 100644
--- a/monkeypatch.py
+++ b/monkeypatch.py
@@ -280,6 +280,7 @@ def delattr(self, target: object | str, name: str | Notset = notset, raising: bool = True) -> None:
         if not hasattr(target, name):
             if raising:
                 raise AttributeError(name)
+            self._setattr.append((target, name, notset))
         else:
             oldval = getattr(target, name, notset)
             # Avoid class descriptors like staticmethod/classmethod.
@@ -303,6 +304,7 @@ def delitem(self, dic: Mapping[K, V], name: K, raising: bool = True) -> None:
         if name not in dic:
             if raising:
                 raise KeyError(name)
+            self._setitem.append((dic, name, notset))
         else:
             self._setitem.append((dic, name, dic.get(name, notset)))
             # Not all Mapping types support indexing, but MutableMapping doesn't support TypedDict

Environment

Any OS, issue present in the current code
https://github.com/pytest-dev/pytest/blob/main/src/_pytest/monkeypatch.py#L250

[The-Compiler]

I think this is working as expected. Monkeypatch is reversing the operations it did, not anything you did afterwards. Use monkeypatch.setitem() instead.

[joaoe]

Monkeypatch is reversing the operations it did

Hi, thank you for your comment.

That's one way to interpret it what Monkeypatch does.

The other use case of the Monkeypatch object is that it pins the object's original state, so the object then can be manipulated by the test and when it ends, the original state is restored. This is something I rely often on.

Regardless, the function has this quirk, which forces the test code to if-check the object for some specific state even though it might not be an object local to the test with a known state.
Currently, my workaround is to do mp.setitem(obj, prop, None);mp.delitem(obj, prop, raising=False) to achieve the same behavior.

Examples for the use case:

1. I might have a test that sets up some env vars using mp.setenv, but before I need to run mp.delenv to make sure the initial state is clean. If mp.delenv is a no-op because the variables are never defined, then the values set by the test will leak.

2. Likewise point 1. but when testing stuff with singletons, e.g. global configuration objects.

3, Another case that happens now and then, is when unit-testing module loading, if a module has branches given how the local environment looks, e.g. OS differences and optional dependencies installed. I want to start a test with mp.delitem("sys.modules", "my.module.name"), do some setup/mocking, then do importlib.import_module(...) for each branch I want to test.