From c057e81fa355959d3b1955e906e2e047e7bca774 Mon Sep 17 00:00:00 2001 From: Alex Zaslavsky Date: Tue, 18 Jul 2023 16:44:29 -0700 Subject: [PATCH] test(backup): Ensure coverage of all exportable models The basic idea here is to include a test that finds all non-abstract descendants of our `BaseModel`, filter down to just the exportable ones (ie, those that set `__include_in_export__ = True`), and then ensure that each of them is included in at least one of the tests seen in `test_models.py`. This is done by introducing a new `mark` wrapper class which ingests all of the "target" models used by the `@targets` test decorator at init time, allowing us to create an exhaustive list of all types passed to `@targets`. Issue: getsentry/team-ospo#156 --- tests/sentry/backup/test_coverage.py | 49 ++++++++++++ tests/sentry/backup/test_models.py | 110 +++++++++++++++------------ 2 files changed, 110 insertions(+), 49 deletions(-) create mode 100644 tests/sentry/backup/test_coverage.py diff --git a/tests/sentry/backup/test_coverage.py b/tests/sentry/backup/test_coverage.py new file mode 100644 index 00000000000000..37f7fdedc327cf --- /dev/null +++ b/tests/sentry/backup/test_coverage.py @@ -0,0 +1,49 @@ +from __future__ import annotations + +from typing import Type + +from sentry.db.models import BaseModel +from tests.sentry.backup.test_models import TESTED_MODELS + + +def get_final_derivations_of(model: Type): + """A "final" derivation of the given `model` base class is any non-abstract class for the + "sentry" app with `BaseModel` as an ancestor. Top-level calls to this class should pass in `BaseModel` as the argument.""" + out = set() + for sub in model.__subclasses__(): + subs = sub.__subclasses__() + if subs: + out.update(get_final_derivations_of(sub)) + if not sub._meta.abstract and sub._meta.db_table and sub._meta.app_label == "sentry": + out.add(sub) + return out + + +def get_exportable_final_derivations_of(model: Type): + """Like `get_final_derivations_of`, except that it further filters the results to include only `__include_in_export__ = True`.""" + return set( + filter( + lambda c: getattr(c, "__include_in_export__") is True, + get_final_derivations_of(model), + ) + ) + + +ALL_EXPORTABLE_MODELS = {c.__name__ for c in get_exportable_final_derivations_of(BaseModel)} + + +# Note: this gets checked at runtime, but better to avoid possible runtime errors and catch it early +# in CI. +def test_all_final_derivations_of_django_model_set_included_in_export(): + missing = set( + filter( + lambda c: not hasattr(c, "__include_in_export__"), + get_final_derivations_of(BaseModel), + ) + ) + assert not missing + + +def test_exportable_final_derivations_of_django_model_are_tested(): + untested = ALL_EXPORTABLE_MODELS - TESTED_MODELS + assert not untested diff --git a/tests/sentry/backup/test_models.py b/tests/sentry/backup/test_models.py index 17e447b78c6e2b..7bb7f6902955c8 100644 --- a/tests/sentry/backup/test_models.py +++ b/tests/sentry/backup/test_models.py @@ -95,8 +95,20 @@ from sentry.utils.json import JSONData from tests.sentry.backup import ValidationError, tmp_export_to_file +TESTED_MODELS = set() -def targets_models(*expected_models: Type): + +def mark(*marking: Type): + """A function that runs at module load time (which is why this logic can't be folded into the + `targets` decorator) and marks all models that appear in at least one test. This is then used by + test_coverage.py to ensure that all final derivations of django's "Model" that set + `__include_in_export__ = True` are exercised by at least one test here.""" + for model in marking: + TESTED_MODELS.add(model.__name__) + return marking + + +def targets(expected_models: list[Type]): """A helper decorator that checks that every model that a test "targeted" was actually seen in the output, ensuring that we're actually testing the thing we think we are. Additionally, this decorator is easily legible to static analysis, which allows for static checks to ensure that @@ -111,7 +123,7 @@ def wrapped(*args, **kwargs): expected_model_names = {"sentry." + model.__name__.lower() for model in expected_models} notfound = sorted(expected_model_names - actual_model_names) if len(notfound) > 0: - raise AssertionError(f"Some `@targets_models` entries were not used: {notfound}") + raise AssertionError(f"Some `@targets` entries were not used: {notfound}") return ret return wrapped @@ -136,8 +148,8 @@ def import_export_then_validate(self) -> JSONData: """Test helper that validates that data imported from a temporary `.json` file correctly matches the actual outputted export data. - Return the actual JSON, so that we may use the `@targets_models` decorator to ensure that - we have at least one instance of all the "tested for" models in the actual output.""" + Return the actual JSON, so that we may use the `@targets` decorator to ensure that we have + at least one instance of all the "tested for" models in the actual output.""" with tempfile.TemporaryDirectory() as tmpdir: tmp_expect = Path(tmpdir).joinpath(f"{self._testMethodName}.expect.json") @@ -186,18 +198,18 @@ def create_monitor(self): config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB}, ) - @targets_models(Actor) + @targets(mark(Actor)) def test_actor(self): self.create_user(email="test@example.com") self.create_team(name="pre save team", organization=self.organization) return self.import_export_then_validate() - @targets_models(AlertRule, QuerySubscription, SnubaQuery, SnubaQueryEventType) + @targets(mark(AlertRule, QuerySubscription, SnubaQuery, SnubaQueryEventType)) def test_alert_rule(self): self.create_alert_rule() return self.import_export_then_validate() - @targets_models(AlertRuleActivity, AlertRuleExcludedProjects) + @targets(mark(AlertRuleActivity, AlertRuleExcludedProjects)) def test_alert_rule_excluded_projects(self): user = self.create_user() org = self.create_organization(owner=user) @@ -205,7 +217,7 @@ def test_alert_rule_excluded_projects(self): self.create_alert_rule(include_all_projects=True, excluded_projects=[excluded]) return self.import_export_then_validate() - @targets_models(AlertRuleTrigger, AlertRuleTriggerAction, AlertRuleTriggerExclusion) + @targets(mark(AlertRuleTrigger, AlertRuleTriggerAction, AlertRuleTriggerExclusion)) def test_alert_rule_trigger(self): excluded = self.create_project() rule = self.create_alert_rule(include_all_projects=True) @@ -213,7 +225,7 @@ def test_alert_rule_trigger(self): self.create_alert_rule_trigger_action(alert_rule_trigger=trigger) return self.import_export_then_validate() - @targets_models(ApiAuthorization, ApiApplication) + @targets(mark(ApiAuthorization, ApiApplication)) def test_api_authorization_application(self): user = self.create_user() app = ApiApplication.objects.create(name="test", owner=user) @@ -222,7 +234,7 @@ def test_api_authorization_application(self): ) return self.import_export_then_validate() - @targets_models(ApiToken) + @targets(mark(ApiToken)) def test_api_token(self): user = self.create_user() app = ApiApplication.objects.create( @@ -231,20 +243,20 @@ def test_api_token(self): ApiToken.objects.create(application=app, user=user, token=uuid4().hex, expires_at=None) return self.import_export_then_validate() - @targets_models(ApiKey) + @targets(mark(ApiKey)) def test_api_key(self): user = self.create_user() org = self.create_organization(owner=user) ApiKey.objects.create(key=uuid4().hex, organization_id=org.id) return self.import_export_then_validate() - @targets_models(Authenticator) + @targets(mark(Authenticator)) def test_authenticator(self): user = self.create_user() Authenticator.objects.create(user=user, type=1) return self.import_export_then_validate() - @targets_models(AuthIdentity, AuthProvider) + @targets(mark(AuthIdentity, AuthProvider)) def test_auth_identity_provider(self): user = self.create_user() test_data = { @@ -261,28 +273,28 @@ def test_auth_identity_provider(self): ) return self.import_export_then_validate() - @targets_models(ControlOption) + @targets(mark(ControlOption)) def test_control_option(self): ControlOption.objects.create(key="foo", value="bar") return self.import_export_then_validate() - @targets_models(Counter) + @targets(mark(Counter)) def test_counter(self): project = self.create_project() Counter.increment(project, 1) return self.import_export_then_validate() - @targets_models(Dashboard) + @targets(mark(Dashboard)) def test_dashboard(self): self.create_dashboard() return self.import_export_then_validate() - @targets_models(DashboardTombstone) + @targets(mark(DashboardTombstone)) def test_dashboard_tombstone(self): DashboardTombstone.objects.create(organization=self.organization, slug="test-tombstone") return self.import_export_then_validate() - @targets_models(DashboardWidget, DashboardWidgetQuery) + @targets(mark(DashboardWidget, DashboardWidgetQuery)) def test_dashboard_widget(self): dashboard = self.create_dashboard() widget = DashboardWidget.objects.create( @@ -295,29 +307,29 @@ def test_dashboard_widget(self): DashboardWidgetQuery.objects.create(widget=widget, order=1, name="Test Query") return self.import_export_then_validate() - @targets_models(Email) + @targets(mark(Email)) def test_email(self): Email.objects.create(email="email@example.com") return self.import_export_then_validate() - @targets_models(Environment) + @targets(mark(Environment)) def test_environment(self): self.create_environment() return self.import_export_then_validate() - @targets_models(EnvironmentProject) + @targets(mark(EnvironmentProject)) def test_environment_project(self): env = self.create_environment() project = self.create_project() EnvironmentProject.objects.create(project=project, environment=env, is_hidden=False) return self.import_export_then_validate() - @targets_models(Incident) + @targets(mark(Incident)) def test_incident(self): self.create_incident() return self.import_export_then_validate() - @targets_models(IncidentActivity) + @targets(mark(IncidentActivity)) def test_incident_activity(self): IncidentActivity.objects.create( incident=self.create_incident(), @@ -326,7 +338,7 @@ def test_incident_activity(self): ) return self.import_export_then_validate() - @targets_models(IncidentSnapshot, TimeSeriesSnapshot) + @targets(mark(IncidentSnapshot, TimeSeriesSnapshot)) def test_incident_snapshot(self): IncidentSnapshot.objects.create( incident=self.create_incident(), @@ -341,13 +353,13 @@ def test_incident_snapshot(self): ) return self.import_export_then_validate() - @targets_models(IncidentSubscription) + @targets(mark(IncidentSubscription)) def test_incident_subscription(self): user_id = self.create_user().id IncidentSubscription.objects.create(incident=self.create_incident(), user_id=user_id) return self.import_export_then_validate() - @targets_models(IncidentTrigger) + @targets(mark(IncidentTrigger)) def test_incident_trigger(self): excluded = self.create_project() rule = self.create_alert_rule(include_all_projects=True) @@ -359,12 +371,12 @@ def test_incident_trigger(self): status=1, ) - @targets_models(Monitor) + @targets(mark(Monitor)) def test_monitor(self): self.create_monitor() return self.import_export_then_validate() - @targets_models(MonitorEnvironment, MonitorLocation) + @targets(mark(MonitorEnvironment, MonitorLocation)) def test_monitor_environment(self): monitor = self.create_monitor() env = Environment.objects.create(organization_id=monitor.organization_id, name="test_env") @@ -382,17 +394,17 @@ def test_monitor_environment(self): ) return self.import_export_then_validate() - @targets_models(NotificationAction, NotificationActionProject) + @targets(mark(NotificationAction, NotificationActionProject)) def test_notification_action(self): self.create_notification_action(organization=self.organization, projects=[self.project]) return self.import_export_then_validate() - @targets_models(Option) + @targets(mark(Option)) def test_option(self): Option.objects.create(key="foo", value="bar") return self.import_export_then_validate() - @targets_models(OrgAuthToken) + @targets(mark(OrgAuthToken)) def test_org_auth_token(self): user = self.create_user() org = self.create_organization(owner=user) @@ -406,13 +418,13 @@ def test_org_auth_token(self): ) return self.import_export_then_validate() - @targets_models(Organization, OrganizationMapping) + @targets(mark(Organization, OrganizationMapping)) def test_organization(self): user = self.create_user() self.create_organization(owner=user) return self.import_export_then_validate() - @targets_models(OrganizationAccessRequest, OrganizationMember, OrganizationMemberTeam, Team) + @targets(mark(OrganizationAccessRequest, OrganizationMember, OrganizationMemberTeam, Team)) def test_organization_membership(self): organization = self.create_organization(name="test_org", owner=self.user) user = self.create_user("other@example.com") @@ -423,7 +435,7 @@ def test_organization_membership(self): OrganizationAccessRequest.objects.create(member=member, team=team) return self.import_export_then_validate() - @targets_models(OrganizationOption) + @targets(mark(OrganizationOption)) def test_organization_option(self): organization = self.create_organization(name="test_org", owner=self.user) OrganizationOption.objects.create( @@ -431,25 +443,25 @@ def test_organization_option(self): ) return self.import_export_then_validate() - @targets_models(Project, ProjectKey, ProjectOption, ProjectTeam) + @targets(mark(Project, ProjectKey, ProjectOption, ProjectTeam)) def test_project(self): self.create_project() return self.import_export_then_validate() - @targets_models(ProjectBookmark) + @targets(mark(ProjectBookmark)) def test_project_bookmark(self): user = self.create_user() project = self.create_project() self.create_project_bookmark(project=project, user=user) return self.import_export_then_validate() - @targets_models(ProjectKey) + @targets(mark(ProjectKey)) def test_project_key(self): project = self.create_project() self.create_project_key(project) return self.import_export_then_validate() - @targets_models(ProjectOwnership) + @targets(mark(ProjectOwnership)) def test_project_ownership(self): project = self.create_project() ProjectOwnership.objects.create( @@ -457,13 +469,13 @@ def test_project_ownership(self): ) return self.import_export_then_validate() - @targets_models(ProjectRedirect) + @targets(mark(ProjectRedirect)) def test_project_redirect(self): project = self.create_project() ProjectRedirect.record(project, "old_slug") return self.import_export_then_validate() - @targets_models(Relay, RelayUsage) + @targets(mark(Relay, RelayUsage)) def test_relay(self): _, public_key = generate_key_pair() relay_id = str(uuid4()) @@ -471,7 +483,7 @@ def test_relay(self): RelayUsage.objects.create(relay_id=relay_id, version="0.0.1", public_key=public_key) return self.import_export_then_validate() - @targets_models(Repository) + @targets(mark(Repository)) def test_repository(self): Repository.objects.create( name="test_repo", @@ -480,14 +492,14 @@ def test_repository(self): ) return self.import_export_then_validate() - @targets_models(Rule, RuleActivity, RuleSnooze) + @targets(mark(Rule, RuleActivity, RuleSnooze)) def test_rule(self): rule = self.create_project_rule(project=self.project) RuleActivity.objects.create(rule=rule, type=RuleActivityType.CREATED.value) self.snooze_rule(user_id=self.user.id, owner_id=self.user.id, rule=rule) return self.import_export_then_validate() - @targets_models(RecentSearch, SavedSearch) + @targets(mark(RecentSearch, SavedSearch)) def test_search(self): RecentSearch.objects.create( organization=self.organization, @@ -503,7 +515,7 @@ def test_search(self): ) return self.import_export_then_validate() - @targets_models(SentryApp, SentryAppComponent, SentryAppInstallation) + @targets(mark(SentryApp, SentryAppComponent, SentryAppInstallation)) def test_sentry_app(self): app = self.create_sentry_app(name="test_app", organization=self.organization) self.create_sentry_app_installation( @@ -514,7 +526,7 @@ def test_sentry_app(self): updater.run(self.user) return self.import_export_then_validate() - @targets_models(PendingIncidentSnapshot) + @targets(mark(PendingIncidentSnapshot)) def test_snapshot(self): incident = self.create_incident() PendingIncidentSnapshot.objects.create( @@ -522,7 +534,7 @@ def test_snapshot(self): ) return self.import_export_then_validate() - @targets_models(ServiceHook) + @targets(mark(ServiceHook)) def test_service_hook(self): app = self.create_sentry_app() actor = Actor.objects.create(type=ACTOR_TYPES["team"]) @@ -538,14 +550,14 @@ def test_service_hook(self): ) return self.import_export_then_validate() - @targets_models(User, UserEmail, UserOption, UserPermission) + @targets(mark(User, UserEmail, UserOption, UserPermission)) def test_user(self): user = self.create_user() self.add_user_permission(user, "users.admin") UserOption.objects.create(user=user, key="timezone", value="Europe/Vienna") return self.import_export_then_validate() - @targets_models(UserIP) + @targets(mark(UserIP)) def test_user_ip(self): user = self.create_user() UserIP.objects.create( @@ -556,7 +568,7 @@ def test_user_ip(self): ) return self.import_export_then_validate() - @targets_models(UserRole, UserRoleUser) + @targets(mark(UserRole, UserRoleUser)) def test_user_role(self): user = self.create_user() role = UserRole.objects.create(name="test-role")