Skip to content
This repository was archived by the owner on Jun 14, 2023. It is now read-only.

Unit Tests improvements #137

Open
anselmoalexandre wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Unit Tests improvements #137

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fde5b1e
AuthManagerTest code improvements
Oct 27, 2022
f18d3f6
Refactoring AuthApiTest test file
Oct 27, 2022
c60b401
Tests improvements on SessionDaoTest file
Oct 28, 2022
0149805
Few suggestions
Oct 28, 2022
9480825
Adding Truth library dependency to versions catalog
Oct 28, 2022
74d9471
Resolved merge conflicts and added fecth session flow unit test
Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions data/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ kotlin {
languageSettings.apply {
optIn("kotlinx.coroutines.ExperimentalCoroutinesApi")
}
dependencies {
implementation("com.google.truth:truth:1.0.1")
Copy link
Contributor

@wangerekaharun wangerekaharun Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @anselmoalexandre For all dependencies, you need to add them to the lib.versions file. You can check the Readme on why we do this.

Great PR!

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @anselmoalexandre For all dependencies, you need to add them to the lib.versions file. You can check the Readme on why we do this.

Great PR!

🤦🏿‍♂️ my bad. Let me add it to version catalog

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @anselmoalexandre For all dependencies, you need to add them to the lib.versions file. You can check the Readme on why we do this.

Great PR!

Done. Added the dependency to lib.versions file

}
Copy link
Contributor

@michaelbukachi michaelbukachi Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need another assertion library.

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need another assertion library.

Well, the Truth library makes assertions more readable and easy to understand unlike the normal assertions used throughout the hole tests (from what I have seen).
Perhaps we can replace the project assertions library by this one. Read a bit about it, you might change your mind 😀

Copy link
Contributor

@michaelbukachi michaelbukachi Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know about it 😅. I just don't think we need it. Tests can be made more readable without it. Please remove it.

}
}
}
2 changes: 1 addition & 1 deletion data/src/main/java/com/android254/data/dao/SessionDao.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ import com.android254.data.db.model.Session
interface SessionDao : BaseDao<Session> {

@Query("SELECT * FROM SESSION")
fun fetchSessions(): List<Session>
suspend fun fetchSessions(): List<Session> // This should be suspended or return FLow [Note: Don't suspend Flows]
Copy link
Contributor

@chepsi chepsi Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment. We can add it to the readme instead.

Copy link
Member

@jumaallan jumaallan Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre We should not suspend these queries. I think best way is to return flows instead

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jumaallan any reason why we shouldn't suspend it? 🤔

Copy link
Member

@jumaallan jumaallan Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre - maybe this explains it better - https://stackoverflow.com/a/68750634

Flows are different - their whole point is to provide the results at arbitrary times, and potentially without ever stopping! The data items are delivered whenever they're emitted, instead of all at once in a collection.

I am just thinking we should prefer using flows instead?

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre - maybe this explains it better - https://stackoverflow.com/a/68750634

Flows are different - their whole point is to provide the results at arbitrary times, and potentially without ever stopping! The data items are delivered whenever they're emitted, instead of all at once in a collection.

I am just thinking we should prefer using flows instead?

Yep, Flows are great. But you also need to question yourself: is it needed to use Flow? 🤔 Or I can just suspend this. 😄

Copy link
Member

@jumaallan jumaallan Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what everyone else thinks here (I would prefer using flows 😸 )

@michaelbukachi @wangerekaharun @misshannah @chepsi

Copy link

@agesa3 agesa3 Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support @jumaallan idea of using a flow.To better support this I can attach this link for reference https://medium.com/androiddevelopers/room-flow-273acffe5b57

}
2 changes: 1 addition & 1 deletion data/src/main/java/com/android254/data/network/util/TokenProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import kotlinx.coroutines.flow.Flow

interface TokenProvider {

suspend fun fetch(): Flow<String?>
suspend fun fetch(): Flow<String?> // @Allan -> Don't suspend Flows
Copy link
Contributor

@chepsi chepsi Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment. We can add it to the readme instead.

Copy link
Member

@jumaallan jumaallan Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre Good catch

Remove the suspend and return a flow 👯

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @chepsi
This comment is just a heads up to @jumaallan so he can refactor this. @jumaallan please check this

Copy link
Member

@jumaallan jumaallan Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre saw it

I can raise a PR, but if you want, please feel free to refactor it here

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre saw it

I can raise a PR, but if you want, please feel free to refactor it here

You can do the PR it's okay 👌🏿. Also, this was only heads up 😄


suspend fun update(accessToken: String)
}
49 changes: 21 additions & 28 deletions data/src/test/java/com/android254/data/dao/SessionDaoTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,51 +15,44 @@
*/
package com.android254.data.dao

import android.content.Context
import androidx.room.Room
import androidx.test.core.app.ApplicationProvider
import com.android254.data.db.Database
import com.android254.data.db.model.Session
import com.google.common.truth.Truth
import io.mockk.*
import io.mockk.impl.annotations.MockK
import kotlinx.coroutines.test.runTest
import kotlinx.datetime.toInstant
import org.hamcrest.CoreMatchers.`is`
import org.hamcrest.MatcherAssert.assertThat
import org.junit.After
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import java.io.IOException

@RunWith(RobolectricTestRunner::class)
class SessionDaoTest {

private lateinit var session: Session
@MockK
private lateinit var sessionDao: SessionDao
private lateinit var db: Database
Comment on lines -35 to -39
Copy link
Contributor

@michaelbukachi michaelbukachi Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using Robolectric because we want to test against a real db.

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelbukachi With robolectric: it slows down the tests a lot, but it's sometimes helpful if we really want to test the integration with the Android system

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to use it only in a few places, not everywhere. Considering that tests performance are also very important and should be fast as possible

Copy link
Author

@anselmoalexandre anselmoalexandre Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using Robolectric because we want to test against a real db.

But it's okay if you guys think you want to stick with this.

Copy link
Contributor

@michaelbukachi michaelbukachi Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anselmoalexandre We are aware of its drawbacks. We used it to ensure all our queries were running as expected.


@Before
fun setup() {
val context = ApplicationProvider.getApplicationContext<Context>()
db = Room.inMemoryDatabaseBuilder(
context,
Database::class.java
MockKAnnotations.init(this, relaxUnitFun = true)
session = Session(
id = 0,
name = "Welcome Keynote",
publishDate = "2010-06-01T22:19:44.475Z".toInstant()
)
.allowMainThreadQueries() // TODO Please delete me
.build()
sessionDao = db.sessionDao()
}

@After
@Throws(IOException::class)
fun tearDown() {
db.close()
coJustRun { sessionDao.insert(session) }
coEvery { sessionDao.fetchSessions() } returns listOf(session)
}

@Test
fun `test sessionDao fetches all sessions`() = runTest {
val session = Session(0, "Welcome Keynote", "2010-06-01T22:19:44.475Z".toInstant())
// Given
sessionDao.insert(session)
val result = sessionDao.fetchSessions().first()
assertThat(session.name, `is`(result.name))

// When
val result = sessionDao.fetchSessions().first().name

// Then
coVerify(atLeast = 1) { sessionDao.insert(session) }
coVerify { sessionDao.insert(session) }
Truth.assertThat(result).isEqualTo(session.name)
}
}
50 changes: 30 additions & 20 deletions data/src/test/java/com/android254/data/network/AuthApiTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ import com.android254.data.network.models.responses.UserDetails
import com.android254.data.network.util.HttpClientFactory
import com.android254.data.network.util.ServerError
import com.android254.data.preferences.DefaultTokenProvider
import com.google.common.truth.Truth
import io.ktor.client.engine.mock.*
import io.ktor.http.*
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking
import org.hamcrest.CoreMatchers.`is`
import org.hamcrest.MatcherAssert.assertThat
import kotlinx.coroutines.test.runTest
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
Expand All @@ -55,19 +54,21 @@ class AuthApiTest {

@Test(expected = ServerError::class)
fun `test ServerError is thrown when a server exception occurs`() {
// Given
val mockEngine = MockEngine {
delay(500)
respondError(HttpStatusCode.InternalServerError)
}
val httpClient = HttpClientFactory(DefaultTokenProvider(testDataStore)).create(mockEngine)
val api = AuthApi(httpClient)
runBlocking {
api.logout()
}

// When
runTest { api.logout() }
}

@Test
fun `test successful logout`() {
// Given
val mockEngine = MockEngine {
respond(
content = """{"message": "Success"}""",
Expand All @@ -77,14 +78,18 @@ class AuthApiTest {
}
val httpClient = HttpClientFactory(DefaultTokenProvider(testDataStore)).create(mockEngine)
val api = AuthApi(httpClient)
runBlocking {
val response = api.logout()
assertThat(response, `is`(Status("Success")))

// Then
runTest {
api.logout().also {
Truth.assertThat(it).isEqualTo(Status("Success"))
}
}
}

@Test
fun `test successful google login`() {
// Given
val content = """
{
"token": "test",
Expand All @@ -106,18 +111,23 @@ class AuthApiTest {
}
val httpClient = HttpClientFactory(DefaultTokenProvider(testDataStore)).create(mockEngine)
val api = AuthApi(httpClient)
runBlocking {
val accessToken = AccessToken(
token = "test",
user = UserDetails(
name = "Magak Emmanuel",
email = "[email protected]",
gender = null,
avatar = "http://localhost:8000/upload/avatar/img-20181016-wa0026jpg.jpg"
)

val accessToken = AccessToken(
token = "test",
user = UserDetails(
name = "Magak Emmanuel",
email = "[email protected]",
gender = null,
avatar = "http://localhost:8000/upload/avatar/img-20181016-wa0026jpg.jpg"
)
val response = api.googleLogin(GoogleToken("some token"))
assertThat(response, `is`(accessToken))
)

// Then
runTest {
api.googleLogin(GoogleToken("some token")).also {
Truth.assertThat(it).isEqualTo(accessToken)
}
}

}
}
98 changes: 57 additions & 41 deletions data/src/test/java/com/android254/data/repos/AuthManagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import com.android254.data.network.util.NetworkError
import com.android254.data.network.util.TokenProvider
import com.android254.domain.models.DataResult
import com.android254.domain.models.Success
import com.google.common.truth.Truth
import io.mockk.*
import kotlinx.coroutines.runBlocking
import org.hamcrest.CoreMatchers.`is`
import org.hamcrest.MatcherAssert.assertThat
import io.mockk.impl.annotations.MockK
import kotlinx.coroutines.test.runTest
import org.junit.Before
import org.junit.Test
import java.lang.Exception

class AuthManagerTest {
private val fakeUserDetails = UserDetails(
Expand All @@ -37,49 +37,65 @@ class AuthManagerTest {
avatar = "http://test.com"
)

@MockK
private lateinit var authApi: AuthApi

@MockK
private lateinit var tokenProvider: TokenProvider

private lateinit var authManager: AuthManager
private lateinit var exception: Exception
private lateinit var networkError: NetworkError

@Before
fun setup() {
MockKAnnotations.init(this, relaxUnitFun = true)
authManager = AuthManager(authApi, tokenProvider)
networkError = NetworkError()
exception = Exception()
}

@Test
fun `test getAndSaveApiToken successfully`() {
val mockApi = mockk<AuthApi>()
val mockTokenProvider = mockk<TokenProvider>()

runBlocking {
val repo = AuthManager(mockApi, mockTokenProvider)
coEvery { mockApi.googleLogin(any()) } returns AccessToken("test", user = fakeUserDetails)
coEvery { mockTokenProvider.update(any()) } just Runs

val result = repo.getAndSaveApiToken("test")
assertThat(result, `is`(DataResult.Success(Success)))
coVerify { mockTokenProvider.update("test") }
}
fun `test getAndSaveApiToken successfully`() = runTest {
// Given
coEvery { authApi.googleLogin(any()) } returns AccessToken(
"test",
user = fakeUserDetails
)
coEvery { tokenProvider.update(any()) } just Runs

// When
val result = authManager.getAndSaveApiToken("test")

//Then
coVerify { tokenProvider.update("test") }

// And
Truth.assertThat(result).isEqualTo(DataResult.Success(Success))
}

@Test
fun `test getAndSaveApiToken failure - network error`() {
val mockApi = mockk<AuthApi>()
val mockTokenProvider = mockk<TokenProvider>()

runBlocking {
val repo = AuthManager(mockApi, mockTokenProvider)
val exc = NetworkError()

coEvery { mockApi.googleLogin(any()) } throws exc
val result = repo.getAndSaveApiToken("test")
assertThat(result, `is`(DataResult.Error("Login failed", true, exc)))
}
fun `test getAndSaveApiToken failure - network error`() = runTest {
// Given
val exc = NetworkError()
coEvery { authApi.googleLogin(any()) } throws networkError

// When
val result = authManager.getAndSaveApiToken("test")

// Then
Truth.assertThat(result).isEqualTo(DataResult.Error("Login failed", true, networkError))
}

@Test
fun `test getAndSaveApiToken failure - other error`() {
val mockApi = mockk<AuthApi>()
val mockTokenProvider = mockk<TokenProvider>()

runBlocking {
val repo = AuthManager(mockApi, mockTokenProvider)
val exc = Exception()

coEvery { mockApi.googleLogin(any()) } throws exc
val result = repo.getAndSaveApiToken("test")
assertThat(result, `is`(DataResult.Error("Login failed", exc = exc)))
}
fun `test getAndSaveApiToken failure - other error`() = runTest {
// Given
coEvery { authApi.googleLogin(any()) } throws exception

// When
val result = authManager.getAndSaveApiToken("test")

// Then
Truth.assertThat(result).isEqualTo(DataResult.Error("Login failed", exc = exception))
}
}