FTUE - Onboarding registration steps unit tests #5408

ouchadam · 2022-03-02T18:23:44Z

Marked as draft as this relies on #5389

Type of change

Feature
Bugfix
Technical
Other :

Content

Adds unit tests around the Registration steps within the OnboardingViewModel

Extracts out the RegistrationWizard action interactions to it's own class to make testing the view model easier
Converts the RegisterAction to sealed interface to allow for compile time type checking

Motivation 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

Physical
Emulator
OS version(s):

github-actions · 2022-03-02T18:53:30Z

Unit Test Results

106 files +  4 106 suites +4 1m 14s ⏱️ +11s
188 tests +  6 188 ✔️ +  6 0 💤 ±0 0 ❌ ±0
622 runs +24 622 ✔️ +24 0 💤 ±0 0 ❌ ±0

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.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given a selected picture when handling save selected profile picture then updates upstream avatar and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is not supported when updating display name then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is supported when updating display name then updates upstream user display name and moves to choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given homeserver does not support personalisation when registering account then updates state and emits account created event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given no selected picture when saving selected profile picture then emits failure event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given only supports changing profile picture when handling PersonalizeProfile then emits contents choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given supports changing display name when handling PersonalizeProfile then emits contents choose display name
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given upstream failure when handling display name update then emits failure event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given upstream update avatar fails when saving selected profile picture then emits failure event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when handling PostViewEvent then emits contents as view event
…

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given a selected picture, when handling save selected profile picture, then updates upstream avatar and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is not supported, when updating display name, then updates upstream user display name and completes personalization
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given changing profile picture is supported, when updating display name, then updates upstream user display name and moves to choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given no selected picture, when saving selected profile picture, then emits failure event
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given only supports changing profile picture, when handling PersonalizeProfile, then emits contents choose profile picture
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given register action ignores result, when handling action, then does nothing on success
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given register action is non loadable, when handling action, then posts next steps without loading
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given register action requires more steps, when handling action, then posts next steps
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given supports changing display name, when handling PersonalizeProfile, then emits contents choose display name
…

♻️ This comment has been updated with latest results.

github-actions · 2022-03-02T19:33:30Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
=passed=6 failures=17 errors=0 skipped=2
[org.matrix.android.sdk.account]
=passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
=passed=108 failures=12 errors=0 skipped=38
[org.matrix.android.sdk.ordering]
=passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
=passed=0 failures=1 errors=0 skipped=0

…ates

… flow simpler

…n steps - keeps the previous behaviour

… feature to be enabled

mnaturel

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.

mnaturel · 2022-03-17T09:19:11Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingAction.kt


    // Reset actions
-    open class ResetAction : OnboardingAction()
+    open class ResetAction : OnboardingAction


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

yeah! makes sense since I'm in the area 👍

mnaturel · 2022-03-17T09:27:56Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

            is OnboardingAction.ProfilePictureSelected     -> handleProfilePictureSelected(action)
            OnboardingAction.SaveSelectedProfilePicture    -> updateProfilePicture()
            is OnboardingAction.PostViewEvent              -> _viewEvents.post(action.viewEvent)
+            OnboardingAction.StopEmailValidationCheck      -> currentJob = null


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.

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 }

Yes okay it is clear with a new private method, no need for comments in that case.

mnaturel · 2022-03-17T09:29:31Z

vector/src/main/java/im/vector/app/features/onboarding/OnboardingViewModel.kt

+            if (action.hasLoadingState()) {
+                setState { copy(asyncRegistration = Loading()) }
+            }
+            kotlin.runCatching { registrationActionHandler.handleRegisterAction(registrationWizard, action) }


Maybe we can remove the kotlin. prefix here?

good catch, will do 👍

mnaturel · 2022-03-17T10:50:22Z

vector/src/test/java/im/vector/app/features/onboarding/OnboardingViewModelTest.kt


        test
-                .assertStates(
+                .assertStatesWithPrevious(


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.

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 } )

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.

assertStatesChanges sounds good! will also add some documentation

mnaturel · 2022-03-17T11:06:47Z

vector/src/test/java/im/vector/app/test/Extensions.kt

        return this
    }

+    fun assertStatesWithPrevious(initial: S, vararg expected: S.() -> S): ViewModelTest<S, VE> {


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?

mnaturel · 2022-03-17T12:57:14Z

vector/src/test/java/im/vector/app/features/onboarding/RegistrationActionHandlerTest.kt

+    }
+
+    private suspend fun testSuccessfulActionDelegation(case: Case) {
+        fakeRegistrationWizard.givenSuccessFor(result = A_SESSION, case.expect)


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?

@ouchadam I don't know if you have seen this comment? Not a blocker, but I just wanted to know your opinion about it.

ah I somehow missed this 😅 great point! yes, will definitely create a new instance per case

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!

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.

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

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).

I've included the verify ce2c309 🎉

mnaturel · 2022-03-17T13:00:40Z

vector/src/test/java/im/vector/app/test/fakes/FakeRegisterActionHandler.kt

+    }
+
+    fun givenResultsFor(wizard: RegistrationWizard, result: List<Pair<RegisterAction, RegistrationResult>>) {
+        coEvery { instance.handleRegisterAction(wizard, any()) } answers {


Perhaps it would be better to name explicitly the implicit parameter of the different lambdas it?

makes sense to avoid the shadowing, will do 👍

mnaturel · 2022-03-17T13:03:55Z

vector/src/test/java/im/vector/app/test/fakes/FakeRegistrationWizard.kt

+    }
+
+    fun givenSuccessfulAcceptTerms(session: Session) {
+        coEvery { acceptTerms() } returns RegistrationResult.Success(session)


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?

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

ouchadam · 2022-03-17T15:53:15Z

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.

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 😄

…lain its usage

…, giving more context to the currentjob=null

… creating new test instances for each case

ouchadam added the Z-FTUE Issue is relevant to the first time use project or experience label Mar 2, 2022

ouchadam force-pushed the feature/adm/personalisation-complete branch 2 times, most recently from ae20e58 to 6ded764 Compare March 7, 2022 17:17

ouchadam force-pushed the feature/adm/onboarding-tests branch from 537e5f0 to 7a4e4a2 Compare March 7, 2022 17:40

ouchadam mentioned this pull request Mar 11, 2022

FTUE - Single loading state #5517

Merged

6 tasks

ouchadam force-pushed the feature/adm/personalisation-complete branch from 6ded764 to 1c63789 Compare March 15, 2022 13:12

Base automatically changed from feature/adm/personalisation-complete to develop March 15, 2022 17:33

ouchadam added 12 commits March 15, 2022 17:42

converting onboarding action to sealed interface

c15e908

adding test helper for asserting states whilst combining previous upd…

4225f62

…ates

extracting registration steps to separate handler to make testing the…

3fa4150

… flow simpler

ensure the pid add/resend methods do not trigger the next registratio…

434ee67

…n steps - keeps the previous behaviour

adding commas to separate the test name sections

b2a1aa1

cleaning up test names and bodies to be clearer

75cbb72

adding case for result ignoring register actions

804513c

allowing test withPrevious to be supplied a list

390ae43

adding test case for the non loading registration steps

694016f

adding changelog entry

fc5c057

fixing wrong action for starting the sign up

fe206fe

enabling the personalize step for the unit tests preemptively for the…

3d20d46

… feature to be enabled

ouchadam force-pushed the feature/adm/onboarding-tests branch from 7a4e4a2 to 3d20d46 Compare March 15, 2022 17:52

ouchadam marked this pull request as ready for review March 16, 2022 10:22

ouchadam requested review from a team, ganfra and mnaturel and removed request for a team and ganfra March 16, 2022 10:24

mnaturel reviewed Mar 17, 2022

View reviewed changes

ouchadam added 6 commits March 17, 2022 16:38

removing fully qualified import

d77061b

updating with previous state helper and including javadoc to help exp…

5df2ae9

…lain its usage

avoiding shadowed lambda parameters

d514751

removing unused fake helper methods

ba76aac

converting open class to sealed interface for extra type safety

192d1c4

extracting named function out for cancelling the email validation job…

abf62af

…, giving more context to the currentjob=null

mnaturel approved these changes Mar 18, 2022

View reviewed changes

ouchadam added 2 commits March 18, 2022 13:15

explicitly declaring the fake registrationb wizard as not relaxed and…

7f943d3

… creating new test instances for each case

including verification to ensure no other methods are being called

ce2c309

ouchadam merged commit ea9c9ae into develop Mar 18, 2022

ouchadam deleted the feature/adm/onboarding-tests branch March 18, 2022 15:57

FTUE - Onboarding registration steps unit tests #5408

FTUE - Onboarding registration steps unit tests #5408

Uh oh!

Conversation

ouchadam commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Uh oh!

github-actions bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

github-actions bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Matrix SDK

Integration Tests Results:

Uh oh!

mnaturel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam commented Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ouchadam commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022 •

edited

Loading

ouchadam Mar 17, 2022 •

edited

Loading

ouchadam Mar 18, 2022 •

edited

Loading

ouchadam Mar 18, 2022 •

edited

Loading

ouchadam commented Mar 17, 2022 •

edited

Loading