type tests.sentry.incidents.action_handlers.* #98112
Conversation
github-actions
bot
added
the
Scope: Backend
| @@ -69,8 +68,8 @@ def run_test(self, incident, method): | |||
| metric_value=metric_value, | |||
| ) | |||
|
|
|||
| data = orjson.loads(responses.calls[0].request.body) | |||
| return data | |||
| # data = orjson.loads(responses.calls[0].request.body) | |||
There was a problem hiding this comment.
Intended to delete; this return value is never used.
There was a problem hiding this comment.
(Thanks for the callout!)
| @@ -10,10 +11,15 @@ class FireTest(TestCase, abc.ABC): | |||
| __test__ = Abstract(__module__, __qualname__) | |||
|
|
|||
| @abc.abstractmethod | |||
| def run_test(self, incident: Incident, method: str, **kwargs): | |||
| def run_test(self, incident: Incident, method: str, **kwargs: object) -> Any: | |||
There was a problem hiding this comment.
Looking at the usage would it be better to have this typed as -> None vs Any. Like Any doesn't give us much info to work with
There was a problem hiding this comment.
So the thing is: there are nine definitions of run_test in this module. Seven of them return None, the test_discord.py instance returns the result of orjson.loads (which is typed as Any), and the one in test_slack.py returns tuple[Incident, str].
The run_test return is only used in __init__.py, where it's returned immediately from run_fire_test — whose return is only used in test_slack.py, which needs the tuple return.
I can look into whether there's something smarter we can do for inheritance, but the tricky thing here is that this function genuinely is returning very different things.
There was a problem hiding this comment.
so I'm wondering if we could just get rid of those return values. It doesn't look like test_slack or test_discord care about the data returned in their usages
There was a problem hiding this comment.
It's needed in test_slack.py:136 but I think I can raise the relevant logic there.
thetruecpaul
force-pushed
the
hackweek/cpaul/testactionahldneere
branch
from
8d6ba0a to
5f59fd3
Compare
Test Plan:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| @@ -46,7 +46,7 @@ def setUp(self) -> None: | |||
| self.handler = SentryAppActionHandler() | |||
|
|
|||
| @responses.activate | |||
| def run_test(self, incident, method): | |||
| def run_test(self, incident: Incident, method: str) -> None: | |||
There was a problem hiding this comment.
Bug: Missing **kwargs in run_test Method
The run_test method in these test files is missing the **kwargs parameter required by its abstract base method. This signature mismatch could cause runtime errors if additional arguments, like chart_url, are passed.
Additional Locations (5)
tests/sentry/incidents/action_handlers/test_opsgenie.py#L176-L177tests/sentry/incidents/action_handlers/test_pagerduty.py#L163-L164tests/sentry/incidents/action_handlers/test_msteams.py#L79-L80tests/sentry/incidents/action_handlers/test_discord.py#L52-L53tests/sentry/incidents/action_handlers/test_email.py#L73-L74
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## hackweek/typing-25 #98112 +/- ##
=====================================================
Coverage ? 80.63%
=====================================================
Files ? 8598
Lines ? 379421
Branches ? 24710
=====================================================
Hits ? 305941
Misses ? 73102
Partials ? 378 |
Found a pattern where this module just needed their
run_testmethods typed.