Unit Tests improvements #137

anselmoalexandre · 2022-10-28T06:11:51Z

Hi dear reviewer 👋🏿😀

I found out about this on one of @tamzi talks so I decided to have a look.
And when I did, saw that you guys are doing great job.

Congrats 🎉 🥳 👏 🍾 🙌 🎊 🥂

By just rumbling out on the code, among the other minor things that I saw that needs improvements, I decided to step up and contribute (dunno if I did 😀).

I added few modifications/improvements on Unit tests (I can continue reviewing the code if it's okay).

Let me know what you guys think. Also, if you need any help I can contribute more on this project/app.

Thanks ☺️
Anselmo Alexandre

chepsi · 2022-10-28T07:17:48Z

data/src/main/java/com/android254/data/network/util/TokenProvider.kt

@@ -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


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

@anselmoalexandre Good catch

Remove the suspend and return a flow 👯

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

@anselmoalexandre saw it

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

@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 😄

chepsi · 2022-10-28T07:17:53Z

data/src/main/java/com/android254/data/dao/SessionDao.kt

@@ -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]


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

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

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

@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?

@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. 😄

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

@michaelbukachi @wangerekaharun @misshannah @chepsi

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

michaelbukachi · 2022-10-28T07:24:27Z

data/build.gradle.kts

+            dependencies {
+                implementation("com.google.truth:truth:1.0.1")
+            }


I don't think we need another assertion library.

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 😀

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

michaelbukachi · 2022-10-28T07:25:38Z

data/src/test/java/com/android254/data/dao/SessionDaoTest.kt

-@RunWith(RobolectricTestRunner::class)
 class SessionDaoTest {
-
+    private lateinit var session: Session
+    @MockK
    private lateinit var sessionDao: SessionDao
-    private lateinit var db: Database


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

@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

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

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.

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

wangerekaharun · 2022-10-28T07:28:51Z

data/build.gradle.kts

@@ -84,6 +84,9 @@ kotlin {
            languageSettings.apply {
                optIn("kotlinx.coroutines.ExperimentalCoroutinesApi")
            }
+            dependencies {
+                implementation("com.google.truth:truth:1.0.1")


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!

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

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

misshannah · 2022-10-31T15:49:24Z

@anselmoalexandre can you resolve the conflicts before we merge this?

anselmoalexandre · 2022-11-01T06:22:40Z

@anselmoalexandre can you resolve the conflicts before we merge this?

Sure

Anselmo Alexandre added 4 commits October 27, 2022 22:21

AuthManagerTest code improvements

fde5b1e

Refactoring AuthApiTest test file

f18d3f6

Tests improvements on SessionDaoTest file

c60b401

Few suggestions

0149805

anselmoalexandre requested review from chepsi, wangerekaharun, michaelbukachi, jumaallan, kibettheophilus and misshannah as code owners October 28, 2022 06:11

chepsi reviewed Oct 28, 2022

View reviewed changes

chepsi approved these changes Oct 28, 2022

View reviewed changes

michaelbukachi reviewed Oct 28, 2022

View reviewed changes

wangerekaharun reviewed Oct 28, 2022

View reviewed changes

chepsi self-requested a review October 28, 2022 07:37

Adding Truth library dependency to versions catalog

9480825

anselmoalexandre requested review from wangerekaharun and michaelbukachi and removed request for chepsi October 28, 2022 08:15

Resolved merge conflicts and added fecth session flow unit test

74d9471

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit Tests improvements #137

Unit Tests improvements #137

anselmoalexandre commented Oct 28, 2022

chepsi Oct 28, 2022 •

edited

Loading

jumaallan Oct 28, 2022

anselmoalexandre Oct 28, 2022

jumaallan Oct 28, 2022

anselmoalexandre Oct 28, 2022

chepsi Oct 28, 2022 •

edited

Loading

jumaallan Oct 28, 2022

anselmoalexandre Oct 28, 2022 •

edited

Loading

jumaallan Oct 28, 2022

anselmoalexandre Oct 28, 2022

jumaallan Oct 28, 2022

agesa3 Oct 28, 2022

michaelbukachi Oct 28, 2022

anselmoalexandre Oct 28, 2022

michaelbukachi Oct 28, 2022

michaelbukachi Oct 28, 2022

anselmoalexandre Oct 28, 2022

anselmoalexandre Oct 28, 2022

anselmoalexandre Oct 28, 2022

michaelbukachi Oct 28, 2022

wangerekaharun Oct 28, 2022

anselmoalexandre Oct 28, 2022

anselmoalexandre Oct 28, 2022

misshannah commented Oct 31, 2022

anselmoalexandre commented Nov 1, 2022

Unit Tests improvements #137

Are you sure you want to change the base?

Unit Tests improvements #137

Conversation

anselmoalexandre commented Oct 28, 2022

chepsi Oct 28, 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

chepsi Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anselmoalexandre Oct 28, 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

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

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

misshannah commented Oct 31, 2022

anselmoalexandre commented Nov 1, 2022

chepsi Oct 28, 2022 •

edited

Loading

chepsi Oct 28, 2022 •

edited

Loading

anselmoalexandre Oct 28, 2022 •

edited

Loading