Louvorg/ReaderForSelfoss-multiplatform
Louvorg/ReaderForSelfoss-multiplatform
3
2
Fork 2
Code Issues 16 Pull Requests Projects 1 Releases 65 Wiki Activity

Repository Unit Tests #50

Merged
AmineB merged 38 commits from davidoskky/ReaderForSelfoss-multiplatform:repository_tests into master 2022-09-30 11:31:55 +00:00
Conversation 7 Commits 38 Files Changed 7 +5 -5
2 changed files with 5 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit cb4f2f02ef - Show all commits

4
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
View File

@ -143,13 +143,13 @@ class Repository(private val api: SelfossApi, private val appSettingsService: Ap
return success
}
suspend fun getTags(): List<SelfossModel.Tag>? {
suspend fun getTags(): List<SelfossModel.Tag> {
return if (isNetworkAvailable()) {
val apiTags = api.tags()
if (apiTags.success && apiTags.data != null && (appSettingsService.isItemCachingEnabled() || !appSettingsService.isUpdateSourcesEnabled())) {
resetDBTagsWithData(apiTags.data)
}
apiTags.data
apiTags.data ?: emptyList()
} else {
getDBTags().map { it.toView() }
}

6
shared/src/commonTest/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryTest.kt
View File

@ -285,7 +285,7 @@ class RepositoryTest() {
assertSame(true, success)
assertSame(1, repository.badgeAll)
assertSame(0, repository.badgeUnread)
assertSame(1, repository.badgeUnread)
assertSame(1, repository.badgeStarred)
coVerify(exactly = 0) { api.stats() }
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 13:48:08 +00:00
Review
Copy Link

Be cause of the line 40 of the setup function you have an unread/starred item in your DB.

Be cause of the line 40 of the `setup` function you have an unread/starred item in your DB.
davidoskky commented 2022-09-27 18:22:38 +00:00
Review
Copy Link

Yes, this is intended.
Since items caching is disabled I would expect the function not to access items in the database.
Maybe it's better if I define that item within this function to avoid ambiguity.

Yes, this is intended. Since items caching is disabled I would expect the function not to access items in the database. Maybe it's better if I define that item within this function to avoid ambiguity.
AmineB commented 2022-09-27 18:30:32 +00:00
Review
Copy Link

Since items caching is disabled I would expect the function not to access items in the database.

I answerd this in another comment.

The DB should be empty when the setting is disabled, so this should never happen.

> Since items caching is disabled I would expect the function not to access items in the database. I answerd this in another comment. The DB should be empty when the setting is disabled, so this should never happen.
davidoskky commented 2022-09-27 18:49:15 +00:00
Review
Copy Link

Even if the DB is empty, there is no reason to call it if the setting is disabled.
I placed something in the DB just to test that it was not called.

Even if the DB is empty, there is no reason to call it if the setting is disabled. I placed something in the DB just to test that it was not called.
AmineB commented 2022-09-27 18:51:58 +00:00
Review
Copy Link

I find it useless to test if caching is enabled to check if there is something in the DB.

I find it useless to test if caching is enabled to check if there is something in the DB.
davidoskky commented 2022-09-27 19:31:39 +00:00
Review
Copy Link

It should be quicker and prevents errors in case some problems with DB clearing happens.

It should be quicker and prevents errors in case some problems with DB clearing happens.
AmineB commented 2022-09-27 19:35:35 +00:00
Review
Copy Link

Still, db cleaning should be done on setting change.

Still, db cleaning should be done on setting change.
👍 1
verify(atLeast = 1) { db.itemsQueries.items().executeAsList()}
@ -391,13 +391,13 @@ class RepositoryTest() {
every { connectivityStatus.isNetworkConnected } returns MutableStateFlow(false)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-09-27 13:31:45 +00:00
Outdated
Review
Copy Link

Tags and sources caching is handled via the update_sources setting.

Also, disabling that setting should clear the DB (this is not the case yet)

So this line should return null, as we won't check for the update_sources setting every time we try to fetch the db data.

Tags and sources caching is handled via the update_sources setting. Also, disabling that setting should clear the DB (this is not the case yet) So this line should return null, as we won't check for the update_sources setting every time we try to fetch the db data.
AmineB commented 2022-09-27 13:35:22 +00:00
Outdated
Review
Copy Link

Returning null isn't possible, so it should be an empty list.

Returning `null` isn't possible, so it should be an empty list.
👍 1
davidoskky commented 2022-09-27 18:26:58 +00:00
Outdated
Review
Copy Link

Why is tags caching handled by update_sources?

Why is tags caching handled by update_sources?
AmineB commented 2022-09-27 18:32:40 +00:00
Outdated
Review
Copy Link

Because it's handled separatly, so users can cache the sources/tags without caching anything else.

Because it's handled separatly, so users can cache the sources/tags without caching anything else.
davidoskky commented 2022-09-27 18:50:17 +00:00
Outdated
Review
Copy Link

I don't think sources and tags should be cached if items are not cached; what would be the use of such a function?

I don't think sources and tags should be cached if items are not cached; what would be the use of such a function?
AmineB commented 2022-09-27 18:53:07 +00:00
Outdated
Review
Copy Link

I don't think sources and tags should be cached if items are not cached; what would be the use of such a function?

As mentionnend by you in a previous comment, it is used to limit the ammount of calls to the selfoss instance, by limiting the call to tags/sources routes.

> I don't think sources and tags should be cached if items are not cached; what would be the use of such a function? As mentionnend by you in a previous comment, it is used to limit the ammount of calls to the selfoss instance, by limiting the call to tags/sources routes.
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
var testTags: List<SelfossModel.Tag> = emptyList()
runBlocking {
testTags = repository.getTags()
}
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
assertSame(tagsDB.first().name, testTags.first().tag)
coVerify(exactly = 0) { api.tags() }
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 13:38:39 +00:00
Review
Copy Link

this should be assertEquals(emptyList(), testTags)

this should be `assertEquals(emptyList(), testTags)`
👍 1
verify(atLeast = 1) { db.tagsQueries.tags().executeAsList() }
}
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 13:38:50 +00:00
Review
Copy Link

This will be called once.

This will be called once.