ref(hybrid-cloud): Mark stable a bunch of control tests (#49039)

Mark a bunch of control silo tests as stable

.vscode/launch.json

@@ -44,7 +44,10 @@
       "request": "launch",
       "module": "pytest",
       "args": ["${file}"],
-      "django": true
+      "django": true,
+      "env": {
+        "SENTRY_MODEL_MANIFEST_FILE_PATH": "./model-manifest.json"
+      }
     }
   ]
 }

scripts/silo/decorators/decorate_unit_tests.py

@@ -2,7 +2,7 @@
 
 from __future__ import annotations
 
-from scripts.silo.add_silo_decorators import SILO_KEYWORDS
+from sentry.testutils.modelmanifest import ModelManifest
 from sentry.utils.silo.decorate_unit_tests import decorate_unit_tests
 
 """Add silo mode decorators to unit test cases en masse.
@@ -15,13 +15,17 @@ business in order to apply the decorators.
 Instructions for use:
 
 From the Sentry project root, do
-    pytest --collect-only | ./scripts/decorators/silo/decorate_unit_tests.py
+    pytest --collect-only | ./scripts/silo/decorators/decorate_unit_tests.py
 
 Running `pytest` to collect unit test cases can be quite slow. To speed up
 repeated runs, you can instead do
     pytest --collect-only > pytest-collect.txt
-    ./scripts/decorators/silo/decorate_unit_tests.py < pytest-collect.txt
+    ./scripts/silo/decorators/decorate_unit_tests.py < pytest-collect.txt
 """
 
+_MODEL_MANIFEST_FILE_PATH = "./model-manifest.json"  # os.getenv("SENTRY_MODEL_MANIFEST_FILE_PATH")
+global _model_manifest
+_model_manifest = ModelManifest.open(_MODEL_MANIFEST_FILE_PATH)
+
 if __name__ == "__main__":
-    decorate_unit_tests(silo_keywords=SILO_KEYWORDS)
+    decorate_unit_tests(_model_manifest)

src/sentry/testutils/modelmanifest.py

@@ -1,34 +1,38 @@
 from __future__ import annotations
 
 import ast
+import logging
 import os
 from collections import defaultdict
 from contextlib import ExitStack, contextmanager
-from pathlib import Path
-from typing import Any, Dict, Generator, Iterable, Iterator, MutableMapping, Set, Tuple, Type, Union
+from typing import Any, Dict, Generator, Iterable, MutableMapping, Set, Tuple, Type, Union
 
 import django.apps
+from django.apps import apps
 
 from sentry.db.models import BaseManager, Model
 from sentry.db.models.manager.base import ModelManagerTriggerAction, ModelManagerTriggerCondition
+from sentry.silo.base import SiloMode
 from sentry.utils import json
 
+ROOT = os.path.dirname(os.path.abspath(__file__)) + "/../../../"
+
 
 class ModelManifest:
     """For auditing which models are touched by each test case."""
 
     file_path: str
     connections: MutableMapping[int, Set[int]]
-    model_names: MutableMapping[str, int]
-    test_names: MutableMapping[str, int]
+    model_names: MutableMapping[str, Dict[str, Any]]
+    test_names: MutableMapping[str, Dict[str, Any]]
     reverse_lookup: MutableMapping[int, str]
     next_id: int
 
-    def get_or_create_id(self, cache: MutableMapping[str, int], name: str) -> int:
+    def get_or_create_id(self, cache: MutableMapping[str, Dict[str, Any]], name: str) -> int:
         if name in cache:
-            return cache[name]
+            return int(cache[name]["id"])
         next_id = self.next_id
-        cache[name] = next_id
+        cache[name] = {"id": next_id}
         self.reverse_lookup[next_id] = name
         self.next_id += 1
         return next_id
@@ -54,6 +58,7 @@ class ModelManifest:
         self.test_names = {}
         self.reverse_lookup = {}
         self.next_id = 0
+        self.count = 0
 
     @classmethod
     def from_json_file(cls, file_path: str) -> ModelManifest:
@@ -62,15 +67,15 @@ class ModelManifest:
 
         manifest = ModelManifest(file_path)
         highest_id = 0
-        for model_name, model_id in content["model_names"].items():
-            manifest.model_names[model_name] = model_id
-            highest_id = max(model_id, highest_id)
-            manifest.reverse_lookup[model_id] = model_name
+        for model_name, m in content["model_names"].items():
+            manifest.model_names[model_name] = m
+            highest_id = max(m["id"], highest_id)
+            manifest.reverse_lookup[m["id"]] = model_name
 
-        for test_name, test_id in content["test_names"].items():
-            manifest.test_names[test_name] = test_id
-            highest_id = max(test_id, highest_id)
-            manifest.reverse_lookup[test_id] = test_name
+        for test_name, d in content["test_names"].items():
+            manifest.test_names[test_name] = d
+            highest_id = max(d["id"], highest_id)
+            manifest.reverse_lookup[d["id"]] = test_name
 
         for id, connections in content["connections"].items():
             for connection in connections:
@@ -118,12 +123,19 @@ class ModelManifest:
                     action = entry.create_trigger_action(condition)
                     stack.enter_context(model_manager.register_trigger(condition, action))
 
+            # Overwrite the entire test in place, in case it used to touch a
+            # model and doesn't anymore
+            test_id = self.get_or_create_id(self.test_names, test_name)
+            hc_test = self.hybrid_cloud_test(test_name)
+            self.test_names[test_name] = {
+                "id": test_id,
+                "stable": hc_test.decorator_was_stable or False,
+                "annotated": hc_test.decorator_match_line is not None,
+            }
+
             try:
                 yield
             finally:
-                # Overwrite the entire test in place, in case it used to touch a
-                # model and doesn't anymore
-                test_id = self.get_or_create_id(self.test_names, test_name)
                 self.connections[test_id] = set()
                 for key in list(self.connections.keys()):
                     if test_id in self.connections[key]:
@@ -135,19 +147,48 @@ class ModelManifest:
                         self.connections[test_id].add(model_id)
                         self.connections[model_id].add(test_id)
 
-    def each_hybrid_cloud_test(
-        self, path_refix: Path
-    ) -> Iterator[Tuple[int, HybridCloudTestVisitor]]:
-        for test_node_name, test_id in self.test_names.items():
-            test_file_path: str
-            test_case_name: str
-            test_node_name = test_node_name.split("[")[0]
-            test_file_path, test_case_name = test_node_name.split("::")
-            test_file_path = os.path.abspath(str(path_refix.joinpath(test_file_path)))
-
-            test_visitor = HybridCloudTestVisitor(test_file_path, test_case_name)
-            if test_visitor.exists:
-                yield test_id, test_visitor
+                self.count += 1
+                if self.count % 100 == 0:
+                    with open(self.file_path, mode="w") as f:
+                        json.dump(self.to_json(), f)
+
+    def hybrid_cloud_test(self, test_name: str) -> HybridCloudTestVisitor:
+        # test_id = self.test_names[test_name]["id"]
+        test_file_path: str
+        test_case_name: str
+        test_name = test_name.split("[")[0]
+        test_file_path, test_case_name = test_name.split("::")
+        test_file_path = ROOT + test_file_path
+
+        test_visitor = HybridCloudTestVisitor(test_file_path, test_case_name)
+        test_visitor.load()
+        return test_visitor
+
+    def determine_silo_based_on_connections(self, test_name: str) -> SiloMode:
+        logger = logging.getLogger()
+        test_id = self.get_or_create_id(self.test_names, test_name)
+        region_votes = 0
+        control_votes = 0
+        for model_id in self.connections[test_id]:
+            model_name = self.reverse_lookup[model_id]
+            try:
+                model = apps.get_model("sentry", model_name)
+            except Exception:
+                continue
+
+            if not model:
+                logger.warning(f"Model {model_name} not found in db 'sentry'")
+                continue
+
+            if SiloMode.CONTROL in model._meta.silo_limit.modes:
+                control_votes += 1
+            elif SiloMode.REGION in model._meta.silo_limit.modes:
+                region_votes += 1
+
+        logger.info(f"   Control: {control_votes}, Region: {region_votes}")
+        if control_votes > region_votes:
+            return SiloMode.CONTROL
+        return SiloMode.REGION
 
 
 class HybridCloudTestDecoratorVisitor(ast.NodeVisitor):

src/sentry/utils/silo/decorate_unit_tests.py

@@ -1,37 +1,47 @@
 from __future__ import annotations
 
+import logging
 import os
 import re
 import sys
 from collections import defaultdict
 from dataclasses import dataclass
 from functools import cached_property
-from typing import Callable, Dict, Iterable, List, Mapping, Set, Tuple
+from typing import Callable, Iterable, List, Mapping, Set, Tuple
 
-from sentry.utils.silo.common import Keywords, has_control_name, has_region_name
+from sentry.silo.base import SiloMode
+from sentry.testutils.modelmanifest import ModelManifest
 
+logger = logging.getLogger()
 
-def decorate_unit_tests(silo_keywords: Dict[str, Keywords]):
+
+def decorate_unit_tests(model_manifest: ModelManifest):
     pytest_collection = sys.stdin.read()
 
     case_map = TestCaseMap(TestCaseFunction.parse(pytest_collection))
 
     for (decorator, count) in case_map.report(
-        "control_silo_test", "region_silo_test", classes_only=True
+        "control_silo_test(stable=True)", "region_silo_test(stable=True)", classes_only=True
     ):
-        print(f"{decorator or 'None'}: {count}")  # noqa
+        logger.info(f"{decorator or 'None'}: {count}")  # noqa
 
     def condition(match: TestCaseMatch) -> str | None:
         if not match.case.is_class:
             return None
 
-        if has_control_name(match.case.name, silo_keywords):
-            return "control_silo_test"
-        elif has_region_name(match.case.name, silo_keywords):
-            return "region_silo_test"
+        test_name = match.path + "::" + match.case.name
+        test = model_manifest.test_names.get(test_name, None)
+        if test and not test["annotated"]:
+            mode = model_manifest.determine_silo_based_on_connections(test_name)
+            if mode == SiloMode.REGION:
+                return "region_silo_test"
+            elif mode == SiloMode.CONTROL:
+                return "control_silo_test"
+
+        return None
 
     count = case_map.add_decorators(condition)
-    print(f"Decorated {count} case{'' if count == 1 else 's'}")  # noqa
+    logger.info(f"Decorated {count} case{'' if count == 1 else 's'}")  # noqa
 
 
 @dataclass(frozen=True, eq=True)
@@ -175,9 +185,10 @@ class TestCaseMap:
         count = 0
         for match in self.case_matches:
             decorator = condition(match)
-            if decorator:
+            if decorator == "control_silo_test":
                 result = match.add_decorator(decorator)
-                count += int(result)
+                if result:
+                    count += int(result)
         return count
 
 
@@ -188,6 +199,9 @@ class TestCaseMatch:
     decorators: Tuple[str]
 
     def add_decorator(self, decorator: str) -> bool:
+        logger.info(
+            f"Decorator: {decorator}. IN: {self.decorators} - {decorator in self.decorators}"
+        )
         if decorator in self.decorators:
             return False
         with open(self.path) as f:
@@ -198,4 +212,5 @@ class TestCaseMatch:
         new_code = f"from sentry.testutils.silo import {decorator}\n{new_code}"
         with open(self.path, mode="w") as f:
             f.write(new_code)
+        logger.info(f"!!! Updating {self.path}")
         return True

tests/acceptance/test_auth.py

@@ -4,7 +4,7 @@ from sentry.testutils import AcceptanceTestCase
 from sentry.testutils.silo import control_silo_test
 
 
-@control_silo_test
+@control_silo_test(stable=True)
 class AuthTest(AcceptanceTestCase):
     def enter_auth(self, username, password):
         self.browser.get("/auth/login/")

tests/acceptance/test_oauth_authorize.py

@@ -2,7 +2,7 @@ from sentry.testutils import AcceptanceTestCase
 from sentry.testutils.silo import control_silo_test
 
 
-@control_silo_test
+@control_silo_test(stable=True)
 class OAuthAuthorizeTest(AcceptanceTestCase):
     def setUp(self):
         super().setUp()

tests/conftest.py

@@ -96,7 +96,7 @@ def _escape(s):
     return s.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
 
 
-_MODEL_MANIFEST_FILE_PATH = os.getenv("SENTRY_MODEL_MANIFEST_FILE_PATH")
+_MODEL_MANIFEST_FILE_PATH = "./model-manifest.json"  # os.getenv("SENTRY_MODEL_MANIFEST_FILE_PATH")
 _model_manifest = None
 
 

tests/sentry/api/bases/test_sentryapps.py

@@ -16,6 +16,7 @@ from sentry.testutils.helpers.faux import Mock
 from sentry.testutils.silo import control_silo_test
 
 
+@control_silo_test(stable=True)
 class SentryAppPermissionTest(TestCase):
     def setUp(self):
         self.permission = SentryAppPermission()

tests/sentry/api/endpoints/test_sentry_app_stats.py

@@ -34,7 +34,7 @@ class SentryAppStatsTest(APITestCase):
         )
 
 
-@control_silo_test
+@control_silo_test(stable=True)
 class GetSentryAppStatsTest(SentryAppStatsTest):
     def test_superuser_sees_unowned_published_stats(self):
         self.login_as(user=self.superuser, superuser=True)