Repository navigation
chore(ACI): Add back subscription processor crash rate detector test coverage - #105205
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| # update the detector to be dynamic | ||
| detector = self.crash_rate_detector | ||
| detector.update(config={"detection_type": "dynamic", "comparison_delta": None}) |
There was a problem hiding this comment.
Bug: Config dict replaced instead of merged
The call detector.update(config={"detection_type": "dynamic", "comparison_delta": None}) replaces the entire config dict rather than merging into it. This differs from the pattern used elsewhere in this file at lines 125 and 136, which correctly use detector.config.update({...}) to preserve existing config values. Since update() sets attributes directly via setattr, any other config values set during detector creation will be lost.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #105205 +/- ##
========================================
Coverage 80.56% 80.57%
========================================
Files 9443 9443
Lines 404671 404705 +34
Branches 25723 25723
========================================
+ Hits 326013 326079 +66
+ Misses 78226 78194 -32
Partials 432 432 |
Add missing test coverage for the subscription processor. Many tests we still need were deleted and need to be recovered but updated for ACI. * Add missing tests for early returns from [here](https://github.com/getsentry/sentry/blob/e3f3190fcfcc4ff63e3562516ab92f641d4d1680/tests/sentry/incidents/subscription_processor/test_subscription_processor.py#L287-L388) * Add missing anomaly detection test coverage from [here](https://github.com/getsentry/sentry/blob/e3f3190fcfcc4ff63e3562516ab92f641d4d1680/tests/sentry/incidents/subscription_processor/test_subscription_processor.py#L3138) TODO in future PRs: - [x] Add missing crash rate alert tests from [here](https://github.com/getsentry/sentry/blob/e3f3190fcfcc4ff63e3562516ab92f641d4d1680/tests/sentry/incidents/subscription_processor/test_subscription_processor.py#L3741) #105205 - [x] Add missing upsample tests from [here](https://github.com/getsentry/sentry/blob/e3f3190fcfcc4ff63e3562516ab92f641d4d1680/tests/sentry/incidents/subscription_processor/test_subscription_processor.py#L4626) #105272 * Add missing EAP test (I can't get this one to pass, it always receives `None` from `get_eap_aggregation_value` so I will ask Shruthi post holidays) from [here](https://github.com/getsentry/sentry/blob/e3f3190fcfcc4ff63e3562516ab92f641d4d1680/tests/sentry/incidents/subscription_processor/test_subscription_processor.py#L1869)
cfe603b to
44b91ac
Compare
| } | ||
|
|
||
| mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) | ||
| update_value = (1 - 0 / 100) + 0.05 |
There was a problem hiding this comment.
Bug: Dynamic detector test uses invalid crash rate values
The dynamic detector test uses (1 - 0 / 100) instead of (1 - self.critical_threshold / 100) or (1 - self.warning_threshold / 100) like the equivalent non-dynamic test. This produces values of 1.05 and 0.95, representing impossible crash rates (105% and 95%). The test passes only because Seer's response is mocked, but the test data doesn't represent valid crash rate scenarios. This appears to be a copy-paste error where placeholder 0 values weren't replaced with the threshold properties.
Additional Locations (1)
There was a problem hiding this comment.
seems valid, up to you if you want to change it
| } | ||
|
|
||
| mock_seer_request.return_value = HTTPResponse(orjson.dumps(seer_return_value), status=200) | ||
| update_value = (1 - 0 / 100) + 0.05 |
There was a problem hiding this comment.
seems valid, up to you if you want to change it
| "anomaly_type": AnomalyType.HIGH_CONFIDENCE.value, | ||
| }, | ||
| "timestamp": 1, | ||
| "value": 10, |
There was a problem hiding this comment.
Should value here correspond to update_value for test clarity?
866a1d4 to
981e92a
Compare
This branch was successfully deployed
2 participants
Companion PR to #105113 that adds back the crash rate alert tests and updates them with ACI models. I removed a couple that have to do with resolve thresholds higher than 1 since we don't actually let users set those, but everything else remains.