-
Notifications
You must be signed in to change notification settings - Fork 741
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
FTUE - Onboarding registration steps unit tests #5408
Conversation
Unit Test Results106 files + 4 106 suites +4 1m 14s ⏱️ +11s Results for commit ce2c309. ± Comparison against base commit 17d363c. This pull request removes 12 and adds 18 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Matrix SDKIntegration Tests Results:
|
ae20e58
to
6ded764
Compare
537e5f0
to
7a4e4a2
Compare
6ded764
to
1c63789
Compare
…n steps - keeps the previous behaviour
… feature to be enabled
7a4e4a2
to
3d20d46
Compare
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.
Thanks for extracting some logic from the OnBoardingViewModel
and adding more tests! I have just small remarks. Have you re-tested the onboarding flow on a device? I have not tested on my side.
|
||
// Reset actions | ||
open class ResetAction : OnboardingAction() | ||
open class ResetAction : OnboardingAction |
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.
Should we try to use sealed interface as well for ResetAction
? Like this:
sealed interface ResetAction : OnboardingAction
object ResetHomeServerType : ResetAction
object ResetHomeServerUrl : ResetAction
object ResetSignMode : ResetAction
object ResetLogin : ResetAction
object ResetResetPassword : ResetAction
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! makes sense since I'm in the area 👍
@@ -164,6 +165,7 @@ class OnboardingViewModel @AssistedInject constructor( | |||
is OnboardingAction.ProfilePictureSelected -> handleProfilePictureSelected(action) | |||
OnboardingAction.SaveSelectedProfilePicture -> updateProfilePicture() | |||
is OnboardingAction.PostViewEvent -> _viewEvents.post(action.viewEvent) | |||
OnboardingAction.StopEmailValidationCheck -> currentJob = null |
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.
Do you think we could add a comment to explain what type of Job
we are cancelling here. I find it hard to understand what process we are cancelling here: there are several assignments for currentJob
in the ViewModel
.
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.
as part of the registration flow it's cancelling the wait for email verification step (when we send an email and wait for the user to click the verification link in their email client), however like you mentioned the viewmodel reuses the same currentJob
instance for all of its tasks
for some extra context
- the currentJob uses a property setter to cancel any previous jobs on update
private var currentJob: Job? = null
set(value) {
// Cancel any previous Job
field?.cancel()
field = value
}
The email validation waiting is tied to the pause/resume lifecycle
override fun onResume() {
viewModel.handle(OnboardingAction.PostRegisterAction(RegisterAction.CheckIfEmailHasBeenValidated(0)))
}
override fun onPause() {
viewModel.handle(OnboardingAction.StopEmailValidationCheck)
}
I would prefer to avoid adding a comment if a suitable method could be extracted, how do you feel about...
OnboardingAction.StopEmailValidationCheck -> cancelWaitForEmailValidation()
fun cancelWaitForEmailValidation() {
currentJob = null
}
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.
Yes okay it is clear with a new private method, no need for comments in that case.
if (action.hasLoadingState()) { | ||
setState { copy(asyncRegistration = Loading()) } | ||
} | ||
kotlin.runCatching { registrationActionHandler.handleRegisterAction(registrationWizard, action) } |
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.
Maybe we can remove the kotlin.
prefix here?
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 catch, will do 👍
|
||
test | ||
.assertStates( | ||
.assertStatesWithPrevious( |
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 am a little confused about the intended checks of assertStatesWithPrevious
without knowing the implementation of this method. Do we want to check the ViewModel
emits states given in the parameters in the same order they are declared? This is the withPrevious
which confuses me I guess.
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.
this name was tricky, totally open to ideas
the aim is for each vararg
to be given the previously expected value to assert against, allows the assertions to be focused on the differences between each state emission rather than having to pass the entire state each time
and yep the order is also asserted against as this is an exact check, any extra or missing states will fail the test
val flow = flowOf {
emit(0)
emit(10)
emit(12)
emit(20)
}
flow.assertStatesWithPrevious(
0,
{ initial -> initial + 10 },
{ next -> next + 2 },
{ next -> next + 8 }
)
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.
Okay, great! Thanks for explaining! What about renaming to assertStatesChanges
or assertStatesModification
or do you prefer to keep assertStatesWithPrevious
. In any case, I think you can add your explanation and the example as a documentation method, it helps a lot.
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.
assertStatesChanges
sounds good! will also add some documentation
@@ -55,6 +55,21 @@ class ViewModelTest<S, VE>( | |||
return this | |||
} | |||
|
|||
fun assertStatesWithPrevious(initial: S, vararg expected: S.() -> S): ViewModelTest<S, VE> { |
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.
As said in another comment, we could add some KDoc to the assertStates
methods to clarify the usages. And maybe rename the assertStatesWithPrevious
if we can find something better?
} | ||
|
||
private suspend fun testSuccessfulActionDelegation(case: Case) { | ||
fakeRegistrationWizard.givenSuccessFor(result = A_SESSION, case.expect) |
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.
Maybe we could also check we only call the right method on the wizard for the given case. For that we would need to create a wizard instance per case instead of a global one:
val fakeRegistrationWizard = FakeRegistrationWizard()
fakeRegistrationWizard.givenSuccessFor(result = A_SESSION, case.expect)
val result = registrationActionHandler.handleRegisterAction(fakeRegistrationWizard, case.action)
coVerifyAll {
case.expect(fakeRegistrationWizard)
}
result shouldBeEqualTo AN_EXPECTED_RESULT
What do you think?
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.
@ouchadam I don't know if you have seen this comment? Not a blocker, but I just wanted to know your opinion about it.
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.
ah I somehow missed this 😅 great point! yes, will definitely create a new instance per case
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.
for checking if the expected case is called I'm a little weary of using verifies on non side effect functions as they can cause false positives (although your example also includes checking the return value for extra security)
fun handle(wizard, action) {
when(action) {
foo ->
// wrong behaviour but a verify acceptTerms would still pass
wizard.acceptTerms()
wizard.dummy()
}
}
I would lean towards the verify and result assertion being a little tooo similar as we need to return a value in order for the test to pass, if the wizard was triggering Unit
side effects then I would 100% be for also verifying
could be swayed but would prefer to include less code in the test if possible!
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 see you point. In fact, the reason I wanted to add this verification is to be sure we do not call any other methods on the wizard instance than the one that produces the result (to avoid calls which may produced side effects). I used coVerifyAll
which means: "starts a verification block that should include all calls for coroutines".
But I will approve the PR, it is up to you if you want to add this verification.
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.
if the mocks are set with relaxed=false (default)
then any non mocked functions will throw but someone could easily switch the setting to relaxed=true
, we need to have some processes/rules~ 🤔
// with a non relaxed mock
val mock = mock<RegistrationWizard>(relaxed = false)
coEvery { mock.acceptTerms() } returns RegistrationResult.Success(result)
mock.acceptTerms()
mock.dummy() // throws exception due to not being mocked
// with a relaxed mock
val mock = mock<RegistrationWizard>(relaxed = true)
coEvery { mock.acceptTerms() } returns RegistrationResult.Success(result)
mock.acceptTerms()
mock.dummy()
coVerifyAll { mock.acceptTerms() } // fails due to unexpected dummy()
if I've fully understood your point (correct me if I'm wrong!), we're both aligned with trying to fail the tests when we call unexpected methods and it comes down to whether we want to explicitly call verify or rely on the lack of mocking to catch unexpected calls
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.
Yes we are aligned 😄 I have forgotten there was this relaxed
parameter. I would say using relaxed
makes this verification in an implicit way while with the verify
pattern we explicitly says what we want to test. On my side, I would go for verify
pattern (but we can keep the relaxed = false
as well).
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've included the verify ce2c309 🎉
} | ||
|
||
fun givenResultsFor(wizard: RegistrationWizard, result: List<Pair<RegisterAction, RegistrationResult>>) { | ||
coEvery { instance.handleRegisterAction(wizard, any()) } answers { |
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.
Perhaps it would be better to name explicitly the implicit parameter of the different lambdas it
?
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.
makes sense to avoid the shadowing, will do 👍
} | ||
|
||
fun givenSuccessfulAcceptTerms(session: Session) { | ||
coEvery { acceptTerms() } returns RegistrationResult.Success(session) |
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.
Maybe we could replace it by givenSuccessFor(session) { acceptTerms() }
? Plus, do we still need to keep givenSuccessfulDummy
and givenSuccessfulAcceptTerms
if we have the more generic method givenSuccessFor
?
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.
aha 🤦 you're right! and it turns out both givenSuccessfulAcceptTerms
givenSuccessfulDummy
aren't used because they've been replaced by the generic version, will delete the unused functions
yep! it's also worth mentioning #5519 is based on this PR (and the other FTUE draft PRs) and there's gifs to prove it still works 😄 |
…, giving more context to the currentjob=null
… creating new test instances for each case
Marked as draft as this relies on #5389
Type of change
Content
Adds unit tests around the Registration steps within the
OnboardingViewModel
RegistrationWizard
action interactions to it's own class to make testing the view model easierRegisterAction
to sealed interface to allow for compile time type checkingMotivation and context
To improve the test coverage around the authentication flows. Could be considered part of #5200
Screenshots / GIFs
No UI changes
Tests
Tested devices