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 +120 -2
2 changed files with 120 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 758708e18d - Show all commits

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

@ -380,11 +380,13 @@ class Repository(private val api: SelfossApi, private val appSettingsService: Ap
private fun deleteDBAction(action: ACTION) =
db.actionsQueries.deleteAction(action.id)
// TODO: This function should be private
fun getDBTags(): List<TAG> = db.tagsQueries.tags().executeAsList()
// TODO: This function should be private
fun getDBSources(): List<SOURCE> = db.sourcesQueries.sources().executeAsList()
fun resetDBTagsWithData(tagEntities: List<SelfossModel.Tag>) {
private fun resetDBTagsWithData(tagEntities: List<SelfossModel.Tag>) {
db.tagsQueries.deleteAllTags()
db.tagsQueries.transaction {
@ -394,7 +396,7 @@ class Repository(private val api: SelfossApi, private val appSettingsService: Ap
}
}
fun resetDBSourcesWithData(sources: List<SelfossModel.Source>) {
private fun resetDBSourcesWithData(sources: List<SelfossModel.Source>) {
db.sourcesQueries.deleteAllSources()
db.sourcesQueries.transaction {

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

@ -2,16 +2,19 @@ package bou.amine.apps.readerforselfossv2.repository
import bou.amine.apps.readerforselfossv2.dao.ITEM
import bou.amine.apps.readerforselfossv2.dao.ReaderForSelfossDB
import bou.amine.apps.readerforselfossv2.dao.TAG
import bou.amine.apps.readerforselfossv2.model.SelfossModel
import bou.amine.apps.readerforselfossv2.rest.SelfossApi
import bou.amine.apps.readerforselfossv2.service.AppSettingsService
import bou.amine.apps.readerforselfossv2.utils.ItemType
import bou.amine.apps.readerforselfossv2.utils.toEntity
import com.github.ln_12.library.ConnectivityStatus
import io.mockk.*
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.runBlocking
import kotlin.test.BeforeTest
import kotlin.test.Test
import kotlin.test.assertNotSame
import kotlin.test.assertSame
class RepositoryTest() {
@ -30,6 +33,7 @@ class RepositoryTest() {
every { appSettingsService.getApiVersion() } returns 4
every { appSettingsService.getBaseUrl() } returns "https://test.com/selfoss/"
every { appSettingsService.isItemCachingEnabled() } returns false
every { appSettingsService.isUpdateSourcesEnabled() } returns true
every { connectivityStatus.isNetworkConnected } returns MutableStateFlow(true)
@ -37,6 +41,9 @@ class RepositoryTest() {
coEvery { api.stats() } returns SelfossModel.Stats(NUMBER_ARTICLES, NUMBER_UNREAD, NUMBER_STARRED)
every { db.itemsQueries.items().executeAsList() } returns generateTestDBItems()
every { db.tagsQueries.deleteAllTags() } returns Unit
every { db.tagsQueries.transaction(any(), any()) } returns Unit
every { db.tagsQueries.insertTag(any()) } returns Unit
}
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 13:54:00 +00:00
Review
Copy Link

This does not seem to do anything. Is it needed ?

This does not seem to do anything. Is it needed ?
davidoskky commented 2022-09-27 18:18:37 +00:00
Review
Copy Link

The test can fail if the repository initialization generates an error; this is just a very general case to identify if something in the repository initialization is broken.
All other tests would fail as well, but this will give you certainty that the point of failure is in the initialization.

The test can fail if the repository initialization generates an error; this is just a very general case to identify if something in the repository initialization is broken. All other tests would fail as well, but this will give you certainty that the point of failure is in the initialization.
AmineB commented 2022-09-27 18:29:28 +00:00
Review
Copy Link

The way I understand your comment is that sometimes, the repository can fail an throw an error.

This should not be handled in a unit test, but handled in the code.

The exception should be catched and handled there.

You can keep your tests, but the call should return something, and the tests should actually test something.

Same for the three following tests.

The way I understand your comment is that sometimes, the repository can fail an throw an error. This should not be handled in a unit test, but handled in the code. The exception should be catched and handled there. You can keep your tests, but the call should return something, and the tests should actually **test** something. Same for the three following tests.
👍 1
@Test
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-09-28 06:41:59 +00:00
Outdated
Review
Copy Link

As mentionnend in my previous comment. The try catch must be in the code, not in the unit test.

As mentionnend in my previous comment. The try catch must be in the code, not in the unit test.
AmineB commented 2022-09-28 06:42:17 +00:00
Outdated
Review
Copy Link

Same for all the other try catch in here.

Same for all the other try catch in here.
davidoskky commented 2022-09-28 06:55:47 +00:00
Outdated
Review
Copy Link

I'm not sure how I would implement this test otherwise, I guess the repository should raise an exception during initialization then.

I'm not sure how I would implement this test otherwise, I guess the repository should raise an exception during initialization then.
AmineB commented 2022-09-28 06:57:14 +00:00
Outdated
Review
Copy Link

I guess the repository should raise an exception during initialization then.

Yep. That should be the case.

> I guess the repository should raise an exception during initialization then. Yep. That should be the case.
@ -281,6 +288,115 @@ class RepositoryTest() {
coVerify(exactly = 0) { api.stats() }
verify(exactly = 0) { db.itemsQueries.items().executeAsList()}
}
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
@Test
fun `Get tags`() {
val tags = listOf(SelfossModel.Tag("test", "red", 6),
SelfossModel.Tag("second", "yellow", 0))
coEvery { api.tags() } returns tags
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
runBlocking {
testTags = repository.getTags()
}
assertSame(tags, testTags)
verify(exactly = 0) { db.tagsQueries.tags().executeAsList() }
}
@Test
fun `Get tags with sources update disabled`() {
val tags = listOf(SelfossModel.Tag("test", "red", 6),
SelfossModel.Tag("second", "yellow", 0))
val tagsDB = listOf(TAG("test_DB", "red", 6),
TAG("second_DB", "yellow", 0))
coEvery { api.tags() } returns tags
coEvery { db.tagsQueries.tags().executeAsList() } returns tagsDB
every { appSettingsService.isUpdateSourcesEnabled() } returns false
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
runBlocking {
testTags = repository.getTags()
}
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
coVerify(exactly = 0) { api.tags() }
verify(atLeast = 1) { db.tagsQueries.tags().executeAsList() }
}
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 14:20:34 +00:00
Review
Copy Link

I don't understand what you wanted to test here. The network is available, so the two will be the same.

I don't understand what you wanted to test here. The network is available, so the two will be the same.
davidoskky commented 2022-09-27 18:38:14 +00:00
Review
Copy Link

If update_sources handles tags fetching, then tags should not be fetched using the api.

If update_sources handles tags fetching, then tags should not be fetched using the api.
AmineB commented 2022-09-27 18:40:05 +00:00
Review
Copy Link

The network is available. The api call will be done. update_sources handles the tags/sources caching.

The network is available. The api call will be done. `update_sources` handles the tags/sources caching.
davidoskky commented 2022-09-27 18:44:06 +00:00
Review
Copy Link

I am confused, as far as I remember update_sources was introduced to limit the amount of api calls to selfoss because that was heavy on the database and created some problems.
That is also how it is described in the option and that is what I would expect it to do.
Thus disabling update_sources should prevent such api calls.

I would use items_caching to enable/disable the use of the local database, without distinguishing between sources, tags and items because it makes no sense to store items and not tags or to store just tags and not items.
Also items_caching could be renamed to make it more clear, maybe enable_database.

I am confused, as far as I remember update_sources was introduced to limit the amount of api calls to selfoss because that was heavy on the database and created some problems. That is also how it is described in the option and that is what I would expect it to do. Thus disabling update_sources should prevent such api calls. I would use items_caching to enable/disable the use of the local database, without distinguishing between sources, tags and items because it makes no sense to store items and not tags or to store just tags and not items. Also items_caching could be renamed to make it more clear, maybe enable_database.
AmineB commented 2022-09-27 18:50:54 +00:00
Review
Copy Link

I am confused, as far as I remember update_sources was introduced to limit the amount of api calls to selfoss because that was heavy on the database and created some problems.
That is also how it is described in the option and that is what I would expect it to do.
Thus disabling update_sources should prevent such api calls.

With update_sources enabled, the api calls to tags and sources is always done.

With update_sources disabled, the api calls to tags and sources is done once to cache the data, and then, the data in the cache is used.

I would use items_caching to enable/disable the use of the local database, without distinguishing between sources, tags and items because it makes no sense to store items and not tags or to store just tags and not items.

Users should be able to store only tags and sources, but items_caching should cache everything, you are right.

Also items_caching could be renamed to make it more clear, maybe enable_database.

I don't like renaming settings names, because it means having "migration code" that handle setting the value from the old key. This code could be useless, and we don't really know when to delete it, because users may not update their app every release.

> I am confused, as far as I remember update_sources was introduced to limit the amount of api calls to selfoss because that was heavy on the database and created some problems. That is also how it is described in the option and that is what I would expect it to do. Thus disabling update_sources should prevent such api calls. With `update_sources` enabled, the api calls to tags and sources is always done. With `update_sources` disabled, the api calls to tags and sources is done once to cache the data, and then, the data in the cache is used. > I would use items_caching to enable/disable the use of the local database, without distinguishing between sources, tags and items because it makes no sense to store items and not tags or to store just tags and not items. Users should be able to store only tags and sources, but `items_caching` should cache everything, you are right. > Also items_caching could be renamed to make it more clear, maybe enable_database. I don't like renaming settings names, because it means having "migration code" that handle setting the value from the old key. This code could be useless, and we don't really know when to delete it, because users may not update their app every release.
davidoskky commented 2022-09-27 19:03:41 +00:00
Review
Copy Link

With update_sources disabled, the api calls to tags and sources is done once to cache the data, and then, the data in the cache is used.

Alright, so I should verify that the call to the api is performed only once, regardless of how many times this function is called and that tags and sources are correctly stored in the db.

Users should be able to store only tags and sources, but items_caching should cache everything, you are right.

If I correctly undestood: tags and sources are always saved to the db and it is not possible to disable this behavior, items are stored in the db only if items_caching is enabled.

> With update_sources disabled, the api calls to tags and sources is done once to cache the data, and then, the data in the cache is used. Alright, so I should verify that the call to the api is performed only once, regardless of how many times this function is called and that tags and sources are correctly stored in the db. > Users should be able to store only tags and sources, but items_caching should cache everything, you are right. If I correctly undestood: tags and sources are always saved to the db and it is not possible to disable this behavior, items are stored in the db only if items_caching is enabled.
AmineB commented 2022-09-27 19:23:01 +00:00
Review
Copy Link

Alright, so I should verify that the call to the api is performed only once, regardless of how many times this function is called and that tags and sources are correctly stored in the db.

Yep, you could do that.

If I correctly undestood: tags and sources are always saved to the db and it is not possible to disable this behavior, items are stored in the db only if items_caching is enabled.

No.

Tags and sources should be saved to the db when any of update_sources (this is implemented) or items_caching (this is not) is enabled.

Items are stored only if items_caching is enabled.

> Alright, so I should verify that the call to the api is performed only once, regardless of how many times this function is called and that tags and sources are correctly stored in the db. Yep, you could do that. > If I correctly undestood: tags and sources are always saved to the db and it is not possible to disable this behavior, items are stored in the db only if items_caching is enabled. No. Tags and sources should be saved to the db when any of update_sources (this is implemented) or items_caching (this is not) is enabled. Items are stored only if items_caching is enabled.
👍 1
@Test
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-09-27 14:21:20 +00:00
Outdated
Review
Copy Link

Same here, the two values are different in the initialization, they couldn't be the same after.

Same here, the two values are different in the initialization, they couldn't be the same after.
davidoskky commented 2022-09-27 18:38:45 +00:00
Outdated
Review
Copy Link

I would expect tags to be fetched from the database, since the api should not be used.

I would expect tags to be fetched from the database, since the api should not be used.
AmineB commented 2022-09-27 18:40:41 +00:00
Outdated
Review
Copy Link

The network is available. The api call will be done. update_sources handles the tags/sources caching.

The network is available. The api call will be done. update_sources handles the tags/sources caching.
fun `Get tags with sources update and db disabled`() {
AmineB marked this conversation as resolved
AmineB commented 2022-09-27 14:22:16 +00:00
Review
Copy Link

The network is available, so this will be called. And the next one won't.

The network is available, so this will be called. And the next one won't.
davidoskky commented 2022-09-27 18:40:00 +00:00
Review
Copy Link

If update_sources handles tag fetching then it shouldn't.
Maybe it would be better to rename update_sources so that it becomes more clear that it handles tags as well?
I do get confused at times.

If update_sources handles tag fetching then it shouldn't. Maybe it would be better to rename update_sources so that it becomes more clear that it handles tags as well? I do get confused at times.
AmineB commented 2022-09-27 18:42:27 +00:00
Review
Copy Link

The setting name IS confusing. But but it handles the tags/sources caching.

The setting name IS confusing. But but it handles the tags/sources caching.
val tags = listOf(SelfossModel.Tag("test", "red", 6),
SelfossModel.Tag("second", "yellow", 0))
val tagsDB = listOf(TAG("test_DB", "red", 6),
TAG("second_DB", "yellow", 0))
coEvery { api.tags() } returns tags
coEvery { db.tagsQueries.tags().executeAsList() } returns tagsDB
every { appSettingsService.isUpdateSourcesEnabled() } returns false
every { appSettingsService.isItemCachingEnabled() } returns false
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
runBlocking {
testTags = repository.getTags()
}
assertSame(null, testTags)
coVerify(exactly = 0) { api.tags() }
verify(exactly = 0) { db.tagsQueries.tags().executeAsList() }
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-09-28 06:44:20 +00:00
Outdated
Review
Copy Link

Why is this called twice ?

Why is this called twice ?
davidoskky commented 2022-09-28 06:54:42 +00:00
Outdated
Review
Copy Link

To test that api.tags() gets called only once regardless of how many calls are made to the repository.

To test that api.tags() gets called only once regardless of how many calls are made to the repository.
👍 1
}
@Test
fun `Get tags without connection`() {
val tags = listOf(SelfossModel.Tag("test", "red", 6),
SelfossModel.Tag("second", "yellow", 0))
val tagsDB = listOf(TAG("test_DB", "red", 6),
TAG("second_DB", "yellow", 0))
coEvery { api.tags() } returns tags
coEvery { db.tagsQueries.tags().executeAsList() } returns tagsDB
every { connectivityStatus.isNetworkConnected } returns MutableStateFlow(false)
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
runBlocking {
testTags = repository.getTags()
}
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
coVerify(exactly = 0) { api.tags() }
verify(atLeast = 1) { db.tagsQueries.tags().executeAsList() }
}
@Test
fun `Get tags without connection and items caching disabled`() {
val tags = listOf(SelfossModel.Tag("test", "red", 6),
SelfossModel.Tag("second", "yellow", 0))
val tagsDB = listOf(TAG("test_DB", "red", 6),
TAG("second_DB", "yellow", 0))
coEvery { api.tags() } returns tags
coEvery { db.tagsQueries.tags().executeAsList() } returns tagsDB
every { connectivityStatus.isNetworkConnected } returns MutableStateFlow(false)
every { appSettingsService.isItemCachingEnabled() } returns false
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var testTags: List<SelfossModel.Tag>? = null
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.
runBlocking {
testTags = repository.getTags()
}
assertSame(null, testTags)
coVerify(exactly = 0) { api.tags() }
verify(exactly = 0) { db.tagsQueries.tags().executeAsList() }
}
}
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
fun generateTestDBItems(item : FakeItemParameters = FakeItemParameters()) : List<ITEM> {