droidconKE / droidconKE2019App

Android app for the second Android Developer conference-droidcon in Nairobi 2019
https://appetize.io/app/wavc552uqmn76jaz7bd2bf890r
MIT License
16 stars 15 forks source link

All Tests in AboutViewModelTest always pass #65

Closed johnGachihi closed 4 years ago

johnGachihi commented 4 years ago

I noticed that all the tests in AboutViewModelTest always pass. I think its because the assert calls are run within the context of OneTimeObserver and not the unit tests

michaelbukachi commented 4 years ago

They supposed to pass. We are testing the AboutViewModel only. Not it's interaction with any layer. Since we are dealing with livedata + couroutines we only want to do assertions when the live data is triggered.

johnGachihi commented 4 years ago

Sorry I didn't mention that I had commented out the implementation of the methods under test and the tests still passed. I also added assertThat(true, 'is'(false)) immediately below one of the other assertions but again the test passed

michaelbukachi commented 4 years ago

Interesting. Let me look into it.

michaelbukachi commented 4 years ago

@wangerekaharun It seems the viewModelScope is not affected by test scopes. I had to modify the functions that use the scope to have an optional CoroutineScope. There's a lot of repetition so it's not ideal. Further discussion is needed for an optimum approach.

wangerekaharun commented 4 years ago

Let me check on that

johnGachihi commented 4 years ago

There's this extension function I see being used in tests to get the value of livedata. It does not require the functions to be run in the test scope so using it would eliminate the added repetetive code.

Just:

    @Test
    fun testFetchAboutDetails() {

        coEvery { aboutDetailsRepo.getAboutDetails(any()) } returns FirebaseResult.Success(emptyList())

        aboutViewModel.fetchAboutDetails("value")

        val response = aboutViewModel.getAboutDetailsResponse()

        assertThat(response.getOrAwaitValue(), `is`(empty()))

    }
michaelbukachi commented 4 years ago

I'm usually against the idea of having timeouts in tests especially unit tests, it makes it seem flaky :sweat_smile: . @wangerekaharun @JabezNzomo99 What do you think?

wangerekaharun commented 4 years ago

Seen the additional scope in the functions. Am very green on the tests area so i cannot really give my opinion on the same. Same case applies to the timeouts question.

michaelbukachi commented 4 years ago

@johnGachihi do you mind making a pull request for the AboutViewModelTest with the snippet you proposed?

johnGachihi commented 4 years ago

@johnGachihi do you mind making a pull request for the AboutViewModelTest with the snippet you proposed?

@michaelbukachi I have just done so.

michaelbukachi commented 4 years ago

@johnGachihi do you mind making a pull request for the AboutViewModelTest with the snippet you proposed?

@michaelbukachi I have just done so.

Awesome. Let me take a look.

johnGachihi commented 4 years ago

@michaelbukachi I've been tinkering with this thing. I've found out that we don't really need the LiveData extension function. When I changed the coroutine dispatcher set in the CoroutinesRule junit rule to a TestCoroutineDispatcher the tests work without the LiveData extension. Should I close the pull request I had made and open a new one with these changes.

michaelbukachi commented 4 years ago

@johnGachihi No need for a new pull request since we haven't merged it yet. We'll merge the latest commit.

michaelbukachi commented 4 years ago

Remember to reference this issue whenever you commit so that we can track them easily.

michaelbukachi commented 4 years ago

Closing this.