ref: replace monkeypatch with unittest.mock (#96714)

the monkeypatch fixture has not-well-defined scope compared to context
managers. it also can produce unexpected results when repeatedly called
on the same target potentially leading to test pollution

getsentry portion: getsentry/getsentry#18043

<!-- Describe your PR here. -->

Author: asottile-sentry

src/sentry/testutils/pytest/metrics.py

@@ -1,5 +1,7 @@
+import contextlib
 import dataclasses
 import functools
+from unittest import mock
 
 import pytest
 from snuba_sdk import Entity, Join
@@ -21,104 +23,119 @@
 
 
 @pytest.fixture(autouse=True)
-def control_metrics_access(monkeypatch, request, set_sentry_option):
+def control_metrics_access(request, set_sentry_option):
     from snuba_sdk import MetricsQuery
 
     from sentry.sentry_metrics import indexer
     from sentry.sentry_metrics.indexer.mock import MockIndexer
     from sentry.snuba import tasks
     from sentry.utils import snuba
 
-    if "sentry_metrics" in {mark.name for mark in request.node.iter_markers()}:
-        mock_indexer = MockIndexer()
-        monkeypatch.setattr("sentry.sentry_metrics.indexer.backend", mock_indexer)
-        monkeypatch.setattr("sentry.sentry_metrics.indexer.bulk_record", mock_indexer.bulk_record)
-        monkeypatch.setattr("sentry.sentry_metrics.indexer.record", mock_indexer.record)
-        monkeypatch.setattr("sentry.sentry_metrics.indexer.resolve", mock_indexer.resolve)
-        monkeypatch.setattr(
-            "sentry.sentry_metrics.indexer.reverse_resolve", mock_indexer.reverse_resolve
-        )
-        monkeypatch.setattr(
-            "sentry.sentry_metrics.indexer.bulk_reverse_resolve", mock_indexer.bulk_reverse_resolve
-        )
-
-        old_resolve = indexer.resolve
-
-        def new_resolve(use_case_id, org_id, string):
-            if (
-                use_case_id == UseCaseID.TRANSACTIONS
-                and string in STRINGS_THAT_LOOK_LIKE_TAG_VALUES
-            ):
-                pytest.fail(
-                    f"stop right there, thief! you're about to resolve the string {string!r}. that looks like a tag value, but in this test mode, tag values are stored in clickhouse. the indexer might not have the value!"
+    with contextlib.ExitStack() as ctx:
+        if "sentry_metrics" in {mark.name for mark in request.node.iter_markers()}:
+            mock_indexer = MockIndexer()
+
+            ctx.enter_context(
+                mock.patch.multiple(
+                    indexer,
+                    backend=mock_indexer,
+                    bulk_record=mock_indexer.bulk_record,
+                    record=mock_indexer.record,
+                    resolve=mock_indexer.resolve,
+                    reverse_resolve=mock_indexer.reverse_resolve,
+                    bulk_reverse_resolve=mock_indexer.bulk_reverse_resolve,
                 )
-            return old_resolve(use_case_id, org_id, string)
-
-        monkeypatch.setattr(indexer, "resolve", new_resolve)
+            )
 
-        old_build_results = snuba._apply_cache_and_build_results
+            old_resolve = indexer.resolve
+
+            def new_resolve(use_case_id, org_id, string):
+                if (
+                    use_case_id == UseCaseID.TRANSACTIONS
+                    and string in STRINGS_THAT_LOOK_LIKE_TAG_VALUES
+                ):
+                    pytest.fail(
+                        f"stop right there, thief! you're about to resolve the string {string!r}. that looks like a tag value, but in this test mode, tag values are stored in clickhouse. the indexer might not have the value!"
+                    )
+                return old_resolve(use_case_id, org_id, string)
+
+            ctx.enter_context(mock.patch.object(indexer, "resolve", new_resolve))
+
+            old_build_results = snuba._apply_cache_and_build_results
+
+            def new_build_results(*args, **kwargs):
+                if isinstance(args[0][0].request, dict):
+                    # We only support snql queries, and metrics only go through snql
+                    return old_build_results(*args, **kwargs)
+                query = args[0][0].request.query
+                is_performance_metrics = False
+                is_metrics = False
+                if not isinstance(query, MetricsQuery) and not isinstance(query.match, Join):
+                    is_performance_metrics = query.match.name.startswith("generic")
+                    is_metrics = "metrics" in query.match.name
+
+                if is_performance_metrics:
+                    _validate_query(query, True)
+                elif is_metrics:
+                    _validate_query(query, False)
 
-        def new_build_results(*args, **kwargs):
-            if isinstance(args[0][0].request, dict):
-                # We only support snql queries, and metrics only go through snql
                 return old_build_results(*args, **kwargs)
-            query = args[0][0].request.query
-            is_performance_metrics = False
-            is_metrics = False
-            if not isinstance(query, MetricsQuery) and not isinstance(query.match, Join):
-                is_performance_metrics = query.match.name.startswith("generic")
-                is_metrics = "metrics" in query.match.name
-
-            if is_performance_metrics:
-                _validate_query(query, True)
-            elif is_metrics:
-                _validate_query(query, False)
-
-            return old_build_results(*args, **kwargs)
-
-        monkeypatch.setattr(snuba, "_apply_cache_and_build_results", new_build_results)
-
-        old_create_snql_in_snuba = tasks._create_snql_in_snuba
-
-        def new_create_snql_in_snuba(subscription, snuba_query, snql_query, entity_subscription):
-            query = snql_query.query
-            is_performance_metrics = False
-            is_metrics = False
-            if isinstance(query.match, Entity):
-                is_performance_metrics = query.match.name.startswith("generic")
-                is_metrics = "metrics" in query.match.name
-
-            if is_performance_metrics:
-                _validate_query(query, True)
-            elif is_metrics:
-                _validate_query(query, False)
-
-            return old_create_snql_in_snuba(
-                subscription, snuba_query, snql_query, entity_subscription
+
+            ctx.enter_context(
+                mock.patch.object(snuba, "_apply_cache_and_build_results", new_build_results)
             )
 
-        monkeypatch.setattr(tasks, "_create_snql_in_snuba", new_create_snql_in_snuba)
-        yield
-    else:
-        should_fail = False
+            old_create_snql_in_snuba = tasks._create_snql_in_snuba
 
-        def fail(old_fn, *args, **kwargs):
-            nonlocal should_fail
-            should_fail = True
-            return old_fn(*args, **kwargs)
+            def new_create_snql_in_snuba(
+                subscription, snuba_query, snql_query, entity_subscription
+            ):
+                query = snql_query.query
+                is_performance_metrics = False
+                is_metrics = False
+                if isinstance(query.match, Entity):
+                    is_performance_metrics = query.match.name.startswith("generic")
+                    is_metrics = "metrics" in query.match.name
+
+                if is_performance_metrics:
+                    _validate_query(query, True)
+                elif is_metrics:
+                    _validate_query(query, False)
+
+                return old_create_snql_in_snuba(
+                    subscription, snuba_query, snql_query, entity_subscription
+                )
 
-        monkeypatch.setattr(indexer, "resolve", functools.partial(fail, indexer.resolve))
-        monkeypatch.setattr(indexer, "bulk_record", functools.partial(fail, indexer.bulk_record))
+            ctx.enter_context(
+                mock.patch.object(tasks, "_create_snql_in_snuba", new_create_snql_in_snuba)
+            )
+            yield
+        else:
+            should_fail = False
 
-        yield
+            def fail(old_fn, *args, **kwargs):
+                nonlocal should_fail
+                should_fail = True
+                return old_fn(*args, **kwargs)
 
-        if should_fail:
-            pytest.fail(
-                "Your test accesses sentry metrics without declaring it in "
-                "metadata. Add this to your testfile:\n\n"
-                "pytestmark = pytest.mark.sentry_metrics"
+            ctx.enter_context(
+                mock.patch.object(indexer, "resolve", functools.partial(fail, indexer.resolve))
+            )
+            ctx.enter_context(
+                mock.patch.object(
+                    indexer, "bulk_record", functools.partial(fail, indexer.bulk_record)
+                )
             )
 
+            yield
+
+            if should_fail:
+                pytest.fail(
+                    "Your test accesses sentry metrics without declaring it in "
+                    "metadata. Add this to your testfile:\n\n"
+                    "pytestmark = pytest.mark.sentry_metrics"
+                )
+
 
 def _validate_query(query, tag_values_are_strings):
     def _walk(node):

src/sentry/testutils/pytest/stale_database_reads.py

@@ -1,6 +1,7 @@
 import dataclasses
 from threading import local
 from typing import Any
+from unittest import mock
 
 import pytest
 from celery.app.task import Task
@@ -88,7 +89,7 @@ def test_very_special(stale_database_reads):
 
 
 @pytest.fixture(autouse=True)
-def stale_database_reads(monkeypatch):
+def stale_database_reads():
     _state = local()
 
     old_send = ModelSignal.send
@@ -100,8 +101,6 @@ def send(self, sender, **named):
         finally:
             _state.in_signal_sender = False
 
-    monkeypatch.setattr(ModelSignal, "send", send)
-
     old_on_commit = transaction.on_commit
 
     # Needs to be hooked for autocommit
@@ -112,8 +111,6 @@ def on_commit(*args, **kwargs):
         finally:
             _state.in_on_commit = False
 
-    monkeypatch.setattr(transaction, "on_commit", on_commit)
-
     old_atomic = transaction.atomic
 
     def atomic(*args, **kwargs):
@@ -123,8 +120,6 @@ def atomic(*args, **kwargs):
         finally:
             _state.in_atomic = False
 
-    monkeypatch.setattr(transaction, "atomic", atomic)
-
     old_run_and_clear_commit_hooks = BaseDatabaseWrapper.run_and_clear_commit_hooks
 
     # Needs to be hooked for running commit hooks inside of TransactionTestCase
@@ -135,10 +130,6 @@ def run_and_clear_commit_hooks(*args, **kwargs):
         finally:
             _state.in_run_and_clear_commit_hooks = False
 
-    monkeypatch.setattr(
-        BaseDatabaseWrapper, "run_and_clear_commit_hooks", run_and_clear_commit_hooks
-    )
-
     reports = StaleDatabaseReads(model_signal_handlers=[], transaction_blocks=[])
 
     old_apply_async = Task.apply_async
@@ -156,8 +147,15 @@ def apply_async(self, args=(), kwargs=(), **options):
 
         return old_apply_async(self, args, kwargs, **options)
 
-    monkeypatch.setattr(Task, "apply_async", apply_async)
-
-    yield reports
+    with (
+        mock.patch.object(ModelSignal, "send", send),
+        mock.patch.object(transaction, "on_commit", on_commit),
+        mock.patch.object(transaction, "atomic", atomic),
+        mock.patch.object(
+            BaseDatabaseWrapper, "run_and_clear_commit_hooks", run_and_clear_commit_hooks
+        ),
+        mock.patch.object(Task, "apply_async", apply_async),
+    ):
+        yield reports
 
     _raise_reports(reports)

tests/sentry/api/endpoints/test_relay_projectconfigs.py

@@ -100,11 +100,12 @@ def add_org_key(default_organization, relay):
 
 
 @pytest.fixture
-def no_internal_networks(monkeypatch):
+def no_internal_networks():
     """
     Disable is_internal_ip functionality (make all requests appear to be from external networks)
     """
-    monkeypatch.setattr("sentry.auth.system.INTERNAL_NETWORKS", ())
+    with patch("sentry.auth.system.INTERNAL_NETWORKS", ()):
+        yield
 
 
 @django_db_all
@@ -205,10 +206,10 @@ def test_untrusted_external_relays_should_not_receive_configs(call_endpoint, no_
 
 
 @pytest.fixture
-def projectconfig_cache_set(monkeypatch):
+def projectconfig_cache_set():
     calls: list[dict[str, Any]] = []
-    monkeypatch.setattr("sentry.relay.projectconfig_cache.backend.set_many", calls.append)
-    return calls
+    with patch("sentry.relay.projectconfig_cache.backend.set_many", calls.append):
+        yield calls
 
 
 @django_db_all

tests/sentry/api/endpoints/test_relay_projectconfigs_v2.py

@@ -55,11 +55,12 @@ def add_org_key(default_organization, relay):
 
 
 @pytest.fixture
-def no_internal_networks(monkeypatch):
+def no_internal_networks():
     """
     Disable is_internal_ip functionality (make all requests appear to be from external networks)
     """
-    monkeypatch.setattr("sentry.auth.system.INTERNAL_NETWORKS", ())
+    with patch("sentry.auth.system.INTERNAL_NETWORKS", ()):
+        yield
 
 
 @django_db_all
@@ -166,10 +167,10 @@ def test_untrusted_external_relays_should_not_receive_configs(call_endpoint, no_
 
 
 @pytest.fixture
-def projectconfig_cache_set(monkeypatch):
+def projectconfig_cache_set():
     calls: list[dict[str, Any]] = []
-    monkeypatch.setattr("sentry.relay.projectconfig_cache.backend.set_many", calls.append)
-    return calls
+    with patch("sentry.relay.projectconfig_cache.backend.set_many", calls.append):
+        yield calls
 
 
 @django_db_all

tests/sentry/api/endpoints/test_relay_projectconfigs_v3.py

@@ -45,37 +45,38 @@ def inner(public_keys=None, global_=False):
 
 
 @pytest.fixture
-def projectconfig_cache_get_mock_config(monkeypatch):
-    monkeypatch.setattr(
+def projectconfig_cache_get_mock_config():
+    with patch(
         "sentry.relay.projectconfig_cache.backend.get",
-        lambda *args, **kwargs: {"is_mock_config": True},
-    )
+        return_value={"is_mock_config": True},
+    ):
+        yield
 
 
 @pytest.fixture
-def single_mock_proj_cached(monkeypatch):
+def single_mock_proj_cached():
     def cache_get(*args, **kwargs):
         if args[0] == "must_exist":
             return {"is_mock_config": True}
         return None
 
-    monkeypatch.setattr("sentry.relay.projectconfig_cache.backend.get", cache_get)
+    with patch("sentry.relay.projectconfig_cache.backend.get", cache_get):
+        yield
 
 
 @pytest.fixture
-def projectconfig_debounced_cache(monkeypatch):
-    monkeypatch.setattr(
-        "sentry.relay.projectconfig_debounce_cache.backend.is_debounced",
-        lambda *args, **kargs: True,
-    )
+def projectconfig_debounced_cache():
+    with patch("sentry.relay.projectconfig_debounce_cache.backend.is_debounced", return_value=True):
+        yield
 
 
 @pytest.fixture
-def project_config_get_mock(monkeypatch):
-    monkeypatch.setattr(
+def project_config_get_mock():
+    with patch(
         "sentry.relay.config.get_project_config",
-        lambda *args, **kwargs: ProjectConfig(sentinel.mock_project, is_mock_config=True),
-    )
+        return_value=ProjectConfig(sentinel.mock_project, is_mock_config=True),
+    ):
+        yield
 
 
 @django_db_all