chore(aci milestone 3): refactor migration helper tests #84285
+170
−408
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Backend
❌ 5 Tests Failed:
View the top 1 failed tests by shortest run time
View the full list of 2 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
saponifi3d
reviewed
Jan 30, 2025
Comment on lines
+2224
to
+2246
| def create_alert_rule_detector( | ||
| alert_rule: AlertRule | None = None, | ||
| detector: Detector | None = None, | ||
| **kwargs, | ||
| ) -> AlertRuleDetector: | ||
| if alert_rule is None: | ||
| alert_rule = Factories.create_alert_rule() | ||
| if detector is None: | ||
| detector = Factories.create_detector() | ||
| return AlertRuleDetector.objects.create(alert_rule=alert_rule, detector=detector) | ||
|
|
||
| @staticmethod | ||
| @assume_test_silo_mode(SiloMode.REGION) | ||
| def create_alert_rule_workflow( | ||
| alert_rule: AlertRule | None = None, | ||
| workflow: Workflow | None = None, | ||
| **kwargs, | ||
| ) -> AlertRuleWorkflow: | ||
| if alert_rule is None: | ||
| alert_rule = Factories.create_alert_rule() | ||
| if workflow is None: | ||
| workflow = Factories.create_workflow() | ||
| return AlertRuleWorkflow.objects.create(alert_rule=alert_rule, workflow=workflow) |
There was a problem hiding this comment.
just because these are for our migration only, let's add these to a migration base test class instead. (if we wanted to keep these around after the migration, this is 💯)
Comment on lines
+663
to
+668
| def create_alert_rule_detector(self, *args, **kwargs): | ||
| return Factories.create_alert_rule_detector(*args, **kwargs) | ||
|
|
||
| def create_alert_rule_workflow(self, *args, **kwargs): | ||
| return Factories.create_alert_rule_workflow(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
same comment about these not being intended to hav a long shelf life.
Comment on lines
+257
to
+259
| # check workflow-related tables | ||
| assert not Workflow.objects.filter(id=self.workflow.id).exists() | ||
| assert not AlertRuleWorkflow.objects.filter(id=self.alert_rule_workflow.id).exists() |
There was a problem hiding this comment.
This seems like it might be a good separate test rather than 1 assertion chunk. The reason for that is if we fail to remove a workflow, we won't know if the detector stuff is deleting correctly or not because this test will stop execution at the first failed assertion.
Comment on lines
+261
to
+266
| # check detector-related tables | ||
| assert not Detector.objects.filter(id=self.detector.id).exists() | ||
| assert not AlertRuleDetector.objects.filter(id=self.alert_rule_detector.id).exists() | ||
| assert not DetectorWorkflow.objects.filter(id=self.detector_workflow.id).exists() | ||
| assert not DetectorState.objects.filter(id=self.detector_state.id).exists() | ||
| assert not DataSourceDetector.objects.filter(id=self.data_source_detector.id).exists() |
There was a problem hiding this comment.
This seems like another good logic chunk to pull out as a separate test -- are these all on_cascade deletions?
Comment on lines
+268
to
+271
| # check data condition groups | ||
| assert not DataConditionGroup.objects.filter( | ||
| id=self.detector_data_condition_group.id | ||
| ).exists() |
There was a problem hiding this comment.
this feels like a detector-related table as well, since it should be part of the cascade deletion of a detector
Comment on lines
+273
to
+274
| # check data source | ||
| assert not DataSource.objects.filter(id=self.data_source.id).exists() |
There was a problem hiding this comment.
Then a good final test block. Each of these test blocks would be more or less what you have here just split out to different def test_..
for example:
def test_dual_delete_migrate_alert_rule__workflow(self):
dual_delete_migrated_alert_rule(self.metric_alert)
with self.tasks():
run_scheduled_deletions()
assert not Workflow.objects.filter(id=self.workflow.id).exists()
assert not AlertRuleWorkflow.objects.filter(id=self.alert_rule_workflow.id).exists()
def test_dual_delete_migrate_alert_rule__data_sources(self):
dual_delete_migrated_alert_rule(self.metric_alert)
with self.tasks():
run_scheduled_deletions()
assert not DataSource.objects.filter(id=self.data_source.id).exists()These tests are a bit repetitive, and if you want to go deeper there's https://docs.pytest.org/en/stable/how-to/parametrize.html -- which would allow us to turn those 4 tests back into a single method. That said, it's not that important and the verbosity can help if we handle these differently in the future.
| detector.refresh_from_db() | ||
|
|
||
| assert detector.owner_user_id == self.user.id | ||
| assert detector.owner_team_id is None | ||
|
|
||
| def test_update_metric_alert_config(self): | ||
| migrate_alert_rule(self.metric_alert, self.rpc_user) | ||
| detector = self.detector |
There was a problem hiding this comment.
nit; i'd just use self.detector instead of detector later; that way it's clear you're using a variable from the class instance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's do this the right way!
BaseMetricAlertMigrationTestwith some helper methods that create the expected objects for a dual written alert rule/trigger/action. We can call these insetUp()for each individual test class.