CreatePollViewModel unit tests [PSF-1122] #6320

onurays · 2022-06-15T18:23:25Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Motivation and context

Increase CreatePollViewModel test coverage to 100% (83/83)

Screenshots / GIFs

Tests

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…stion.

mnaturel · 2022-06-16T07:38:09Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewModelTest.kt

+        }
+    }
+
+    @Before


We should unmock mocked objects after each test in general. We can add:

@After fun tearDown() { unmockkAll() }

mnaturel · 2022-06-16T07:41:40Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewModelTest.kt

+    private val fakeSendService = mockk<SendService>()
+    private val fakeCancellable = mockk<Cancelable>()
+
+    private fun createPollViewModel(pollMode: PollMode): CreatePollViewModel {


Maybe we also should add tests for PollMode.EDIT?

mnaturel · 2022-06-16T07:48:55Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewModelTest.kt

+    @get:Rule
+    val mvrxTestRule = MvRxTestRule()
+
+    private val fakeSession = mockk<Session>()


We should use/create FakeObject classes to be reusable for other unit tests. Here you can use FakeSession and FakeRoom. You can add givenXXX methods in them to mock some calls (see existing examples). For the SendService, a FakeSendService can be created and instantiated inside the FakeRoom class. We may only use mockk<> inside tests class when it is very specific to the tested class or simple object.

Right, done.

mnaturel · 2022-06-16T07:55:16Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewModelTest.kt

+        createPollViewModel.handle(CreatePollAction.OnQuestionChanged(fakeQuestion))
+
+        // We need to wait for createPollViewModel.onChange is triggered
+        delay(10)


For state transition, you can use assertStatesChanges instead of assertState to avoid having to add a delay in the tests which may lead to result inconsistencies.

different cpu/computers will be able to execute faster/slower, we should avoid relying on delays whenever possible 🤞

// We need to wait for createPollViewModel.onChange is triggered in the test enviroment we should control all the dispatching and block asynchronous calls (which is the purpose of runTest and MvRxTestRule)

I tried assertStatesChanges and verify with a timeout but no luck.

the issue here looks like your handle is not blocking correctly 🤔

the ViewModel.test()extension is how we're capturing emissions from the ViewModel, if we interact with the ViewModel before test() then we'll miss state events

val test = createPollViewModel.test() // register state and event listeners viewModel.handle() // blocking - causes a state update test.assert() // asserts against the captured states

however here I can see the handle emissions happening after .test()

println("start") val createPollViewModel = createPollViewModel(PollMode.CREATE) createPollViewModel.handle(CreatePollAction.OnQuestionChanged(fakeQuestion)) println("handle") // We need to wait for createPollViewModel.onChange is triggered delay(10) createPollViewModel .also { println("test") } .test() // prints captures emissions .assertState(pollViewStateWithOnlyQuestion) .finish()

start handle test CreatePollViewState(roomId=fakeRoomId, editedEventId=null, mode=CREATE, question=What is your favourite coffee?, options=[, ], canCreatePoll=false, canAddMoreOptions=true, pollType=DISCLOSED_UNSTABLE)

notice how the emission happens after test(), which shows the logic and updates inside handle are non blocking (a test's worst enemy!)

separating the test() resolves the issue for me

println("start") val createPollViewModel = createPollViewModel(PollMode.CREATE) val test = createPollViewModel.test() createPollViewModel.handle(CreatePollAction.OnQuestionChanged(fakeQuestion)) println("handled") test .assertStates(initialCreatePollViewState, pollViewStateWithOnlyQuestion) .finish()

start CreatePollViewState(roomId=fakeRoomId, editedEventId=null, mode=CREATE, question=, options=[, ], canCreatePoll=false, canAddMoreOptions=true, pollType=DISCLOSED_UNSTABLE) CreatePollViewState(roomId=fakeRoomId, editedEventId=null, mode=CREATE, question=What is your favourite coffee?, options=[, ], canCreatePoll=false, canAddMoreOptions=true, pollType=DISCLOSED_UNSTABLE) handled

Okay I see; thanks for clarifying. Using the same Dispatcher seems to prevent us from having to pay attention to the call ordering. What do you think of having a TestDispatcher in the test extension which will be the same as the one used in the mvrx rule?

the ordering is important because we're subscribing to the ViewModel flows in order to capture their emissions

if all the helper logic was inlined it would look like...

// .test() val captures = mutableListOf() viewModel.stateFlow .onEach { captures.add(it} .launchIn(nonBlockingContext) // handle viewModel.setState { copy("foo") } // assert captures shouldBeEqualTo expectedEmissions

turns out there's a bug/coroutine-test incompatibility airbnb/mavericks#599

@get:Rule val mvrxTestRule = MvRxTestRule(testDispatcher = UnconfinedTestDispatcher()) @Test fun `can create poll`() = runTest { val createPollViewModel = createPollViewModel(PollMode.CREATE) val test = createPollViewModel.test() createPollViewModel.handle(CreatePollAction.OnQuestionChanged(A_FAKE_QUESTION)) repeat(CreatePollViewModel.MIN_OPTIONS_COUNT) { createPollViewModel.handle(CreatePollAction.OnOptionChanged(it, A_FAKE_OPTIONS[it])) } test.assertStatesChanges( initialCreatePollViewState, { copy(question = A_FAKE_QUESTION) }, { copy(options = listOf(A_FAKE_OPTIONS[0], "")) }, { copy(options = listOf(A_FAKE_OPTIONS[0], A_FAKE_OPTIONS[1])) }, { copy(canCreatePoll = true) }, ).finish() }

Ahah just found the same solution on my side 😄 I knew there was something strange with TestDispatcher.

Thank you both for the great help! Please see my last commit (and approve :).

mnaturel · 2022-06-16T08:05:43Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewStates.kt

+import im.vector.app.features.poll.PollMode
+import kotlin.random.Random
+
+object CreatePollViewStates {


I think we should move this class to the test package as it is only used as testing purpose. We could also rename it into FakeCreatePollViewStates or we could also declare all the fake data at the top of the related test file: CreatePollViewModelTest before the class declaration. I have a preference for the latter as we can see tha fake data and the tests at the same time.

Exactly, done.

mnaturel · 2022-06-16T08:07:19Z

vector/src/test/java/im/vector/app/features/poll/create/CreatePollViewStates.kt

+
+object CreatePollViewStates {
+
+    const val fakeRoomId = "fakeRoomId"


For all const val, it is preferable to use upper case format like:

const val A_FAKE_ROOM_ID = "fakeRoomId"

Right, done.

mnaturel

Thanks for the tests! I have added some comments to improve reusability and readability.

ouchadam

great job! LGTM 💯

ouchadam · 2022-06-16T15:37:46Z

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

+import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent
+import kotlin.random.Random
+
+object FakeCreatePollViewStates {


minor naming comment for fakes vs fixtures

Fakes are objects where behaviour has been changed/overridden or they're extended with test helpers for capturing information, whereas a fixture is a reusable set of defaults,

I would argue the content of the file fits more into the description of a fixture

for example a FakeRoom could return a FakeTimeline which returns a list of PollEvent fixtures, usually it's a case of class = fake, data class = fixture

sonarcloud · 2022-06-16T17:22:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

onurays added 8 commits June 13, 2022 15:49

Create dummy data for poll view states.

cffa327

Test initial poll view state.

19de215

Test poll view state without enough options.

5b35534

Test poll views state with enough number of options but without a que…

0bf37ab

…stion.

Test poll view state with a question and enough number of options.

1f04e73

Test poll view state with a question and max number of options.

0ed9a18

Test poll view events when create poll is requested.

24d59eb

Changelog added.

934d860

onurays requested review from a team, Claire1817 and mnaturel and removed request for a team June 15, 2022 18:26

Lint fixes.

ec3248d

mnaturel reviewed Jun 16, 2022

View reviewed changes

onurays added 7 commits June 16, 2022 13:39

Rename test data class.

94c0a02

Create fake room services.

b558d14

Test poll view events when poll is edited.

841b63b

Test poll view state when poll option is deleted.

f13dfb8

Move fake class into the right package.

a1d35ae

Workaround for mavericks bug (airbnb/mavericks#599).

14a4a8e

Apply Maxime's fix to get latest state.

712a38e

onurays requested review from ouchadam and mnaturel June 16, 2022 15:30

ouchadam approved these changes Jun 16, 2022

View reviewed changes

ouchadam reviewed Jun 16, 2022

View reviewed changes

mnaturel approved these changes Jun 16, 2022

View reviewed changes

onurays merged commit 242cc28 into develop Jun 17, 2022

onurays deleted the feature/ons/poll_unit_tests branch June 17, 2022 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CreatePollViewModel unit tests [PSF-1122] #6320

CreatePollViewModel unit tests [PSF-1122] #6320

onurays commented Jun 15, 2022 •

edited

Loading

mnaturel Jun 16, 2022

onurays Jun 16, 2022

mnaturel Jun 16, 2022

onurays Jun 16, 2022

mnaturel Jun 16, 2022

onurays Jun 16, 2022

mnaturel Jun 16, 2022 •

edited

Loading

ouchadam Jun 16, 2022

onurays Jun 16, 2022

ouchadam Jun 16, 2022

ouchadam Jun 16, 2022 •

edited

Loading

mnaturel Jun 16, 2022

ouchadam Jun 16, 2022

ouchadam Jun 16, 2022

mnaturel Jun 16, 2022 •

edited

Loading

onurays Jun 16, 2022

mnaturel Jun 16, 2022

onurays Jun 16, 2022

mnaturel Jun 16, 2022

onurays Jun 16, 2022

mnaturel left a comment

ouchadam left a comment

ouchadam Jun 16, 2022 •

edited

Loading

sonarcloud bot commented Jun 16, 2022

+                      }
+                  }
+                  @Before


		object CreatePollViewStates {

		const val fakeRoomId = "fakeRoomId"

CreatePollViewModel unit tests [PSF-1122] #6320

CreatePollViewModel unit tests [PSF-1122] #6320

Conversation

onurays commented Jun 15, 2022 • edited Loading

Type of change

Content

Motivation and context

Increase CreatePollViewModel test coverage to 100% (83/83)

Screenshots / GIFs

Tests

Tested devices

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

ouchadam left a comment

Choose a reason for hiding this comment

ouchadam Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Jun 16, 2022

onurays commented Jun 15, 2022 •

edited

Loading

mnaturel Jun 16, 2022 •

edited

Loading

ouchadam Jun 16, 2022 •

edited

Loading

mnaturel Jun 16, 2022 •

edited

Loading

ouchadam Jun 16, 2022 •

edited

Loading