test(aci milestone 3): rewrite subscription processor tests for workflow engine #95487
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #95487 +/- ##
==========================================
+ Coverage 80.08% 80.65% +0.56%
==========================================
Files 8569 8569
Lines 377479 377479
Branches 24577 24577
==========================================
+ Hits 302293 304444 +2151
+ Misses 74820 72669 -2151
Partials 366 366 |
mifu67
force-pushed
the
mifu67/aci/subscription-processor-tests
branch
from
7f7d827 to
0a83a3e
Compare
github-actions
bot
added
the
Scope: Backend
tests/sentry/incidents/subscription_processor/test_subscription_processor_base.py
Show resolved
Hide resolved
| detector_trigger.comparison = new_threshold | ||
| detector_trigger.save() | ||
|
|
||
| def build_subscription_update(self, subscription, time_delta=None, value=EMPTY): |
There was a problem hiding this comment.
This function and send_update look to be the same as from the existing ProcessUpdateTest - are they copied just so it's easier for us to delete the legacy stuff once we've moved over? We do have tests that we won't be deleting (like test_removed_project for example) but I suppose we can just move those over into the newer files and delete the entire old file?
There was a problem hiding this comment.
Yeah, I want to do a quick and easy file deletion once the switch to ACI is fully complete :P
| self.update_threshold(detector, DetectorPriorityLevel.HIGH, 0.5) | ||
| self.update_threshold(detector, DetectorPriorityLevel.OK, 0.5) |
There was a problem hiding this comment.
Why do we update it twice? Won't it just be the latest one (line 42)?
There was a problem hiding this comment.
The detector priority level here determines the data condition whose threshold gets updated. We have to update both the resolve and critical conditions.
tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py
Show resolved
Hide resolved
ceorourke
force-pushed
the
mifu67/aci/subscription-processor-tests
branch
from
1cb8e67 to
8985b19
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| else: | ||
| # XXX: after we fully migrate to single processing we can return early here | ||
| # this just preserves test functionality for now | ||
| metrics.incr("incidents.alert_rules.skipping_update_invalid_aggregation_value") |
There was a problem hiding this comment.
Bug: Anomaly Detection Fails With Null Aggregation Value
The commit unintentionally allows anomaly detection processing to occur with aggregation_value=None, specifically calling get_anomaly_data_from_seer_legacy with a None value, which was previously prevented by an early return. Additionally, the incidents.alert_rules.skipping_update_invalid_aggregation_value metric is now incremented whenever aggregation_value is None, regardless of the has_metric_issue_single_processing flag, altering its previous behavior.
Duplicate tests from
subscription_processor.pybut rewritten for workflow engine. These will be used for single processing to ensure we don't lose test coverage. This has a lot of tests but isn't comprehensive, there will be follow up PRs for:assert_open_periodandassert_no_open_periodmethods and adding those to tests (waiting on feat(aci): Write to IncidentGroupOpenPeriod #97621)