ref(mediators): Make validator into a dataclass #79116
Conversation
github-actions
bot
added
the
Scope: Backend
| install=self.install, client_id=self.client_id, user=self.user | ||
| ) | ||
| def test_validates_generic_token_exchange_requirements(self): | ||
| assert self.refresher._validate() |
There was a problem hiding this comment.
is this too sus ? Since we're testing/calling a private function ? Lowkey, this is the easiest way to write this test with current design.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #79116 +/- ##
========================================
Coverage 78.30% 78.30%
========================================
Files 7135 7134 -1
Lines 314258 314434 +176
Branches 51331 51350 +19
========================================
+ Hits 246064 246220 +156
- Misses 61758 61772 +14
- Partials 6436 6442 +6 |
| self.record_analytics() | ||
| return token |
There was a problem hiding this comment.
These lines could be outside the transaction block 🤷
| from sentry.models.apiapplication import ApiApplication | ||
| from sentry.sentry_apps.models.sentry_app import SentryApp | ||
| from sentry.sentry_apps.services.app import RpcSentryAppInstallation | ||
| from sentry.users.models.user import User | ||
|
|
||
|
|
||
| class Validator(Mediator): | ||
| @dataclass | ||
| class Validator: |
There was a problem hiding this comment.
Interesting that this is just validation logic 🤔
There was a problem hiding this comment.
yeah, I did some more reading and this class is p. simple so it doesn't really matter which way we go about it.
| install=self.install, client_id=self.client_id, user=self.user | ||
| ) | ||
| def test_validate_generic_token_exchange_requirements(self): | ||
| assert self.grant_exchanger._validate() |
There was a problem hiding this comment.
Having tests call 'protected methods' can make tests brittle as code changes over time. The mock solution also had this weakness though. It looks like we have integration tests for the validation flow below though, and we could just delete these tests without losing any coverage.
Christinarlong
added
the
Trigger: Revert
|
PR reverted: 30dd412 |
This reverts commit e2df3f1. Co-authored-by: Christinarlong <[email protected]>
Turns the token_exchange Validator into dataclass and update refs/usages to reflect that
issue ref (#73857 )