CreatePollViewModel unit tests [PSF-1122] #6320
Conversation
onurays
requested review from
a team,
Claire1817 and
mnaturel
and removed request for
a team
| } | ||
| } | ||
|
|
||
| @Before |
There was a problem hiding this comment.
We should unmock mocked objects after each test in general. We can add:
@After
fun tearDown() {
unmockkAll()
}
| private val fakeSendService = mockk<SendService>() | ||
| private val fakeCancellable = mockk<Cancelable>() | ||
|
|
||
| private fun createPollViewModel(pollMode: PollMode): CreatePollViewModel { |
There was a problem hiding this comment.
Maybe we also should add tests for PollMode.EDIT?
| @get:Rule | ||
| val mvrxTestRule = MvRxTestRule() | ||
|
|
||
| private val fakeSession = mockk<Session>() |
There was a problem hiding this comment.
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.
| createPollViewModel.handle(CreatePollAction.OnQuestionChanged(fakeQuestion)) | ||
|
|
||
| // We need to wait for createPollViewModel.onChange is triggered | ||
| delay(10) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
I tried assertStatesChanges and verify with a timeout but no luck.
There was a problem hiding this comment.
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 stateshowever 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!)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 expectedEmissionsThere was a problem hiding this comment.
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()
}There was a problem hiding this comment.
Ahah just found the same solution on my side 😄 I knew there was something strange with TestDispatcher.
There was a problem hiding this comment.
Thank you both for the great help! Please see my last commit (and approve :).
| import im.vector.app.features.poll.PollMode | ||
| import kotlin.random.Random | ||
|
|
||
| object CreatePollViewStates { |
There was a problem hiding this comment.
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.
|
|
||
| object CreatePollViewStates { | ||
|
|
||
| const val fakeRoomId = "fakeRoomId" |
There was a problem hiding this comment.
For all const val, it is preferable to use upper case format like:
const val A_FAKE_ROOM_ID = "fakeRoomId"
| import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent | ||
| import kotlin.random.Random | ||
|
|
||
| object FakeCreatePollViewStates { |
There was a problem hiding this comment.
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
|
Kudos, SonarCloud Quality Gate passed!
|
Type of change
Content
Motivation and context
Increase CreatePollViewModel test coverage to 100% (83/83)
Screenshots / GIFs
Tests
Tested devices
Checklist