-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: new login use case chooser for context enterprise vs fallback (WPB-15966) #3287
feat: new login use case chooser for context enterprise vs fallback (WPB-15966) #3287
Conversation
Test Results3 442 tests 3 334 ✅ 5m 22s ⏱️ Results for commit 70d4290. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 3334 Passed, 108 Skipped, 1m 0s Total Time |
logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/auth/ObserveLoginContextUseCase.kt
Outdated
Show resolved
Hide resolved
/** | ||
* Common operations for the server configuration repository. | ||
*/ | ||
internal abstract class ServerConfigRepositoryExtension( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed to cover this class with test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, in fact these are covered by the classes extending this one ServerConfigDataSource
and CustomServerConfigDataSource
, so they are covered as can be seen here. https://app.codecov.io/gh/wireapp/kalium/pull/3287/blob/logic/src/commonMain/kotlin/com/wire/kalium/logic/configuration/server/ServerConfigRepositoryExtension.kt
But, there is one actually missing for the new function in CustomServerConfigDataSource observeServerConfigByLinks
I will add one to cover this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but it's unclear exactly what is being tested, we are only sure that the test runs through those lines of code.
We need to isolate the class during testing. This way, we can validate its behavior independently, without interference from external classes/dependencies, ensuring that it behaves correctly in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look at a later point since this is the epic still, but the problem with this is, given is an abstract class, it is not straight forward to test, and the functions are covered indeed by the inherited testable classes
|
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
LoginContext
(Enterprise or Fallback) according to conditionsTesting
Test Coverage (Optional)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.