Repository Unit Tests #50
No reviewers
Labels
No Label
Difficulty - Beginner friendly
Difficulty = Easy
Difficulty = Hard
Difficulty = Medium
Priority = CRITICAL
Priority = High
Priority = Low
Priority = Normal
Priority = Some day
Status - Fixed somewhere else
Status - No details provided
Status = Can't fix
Status = Duplicate
Status = Help wanted
Status = Invalid
Status = Need more details
Status = Taken
Status = Wontfix
Type - Selfoss works like this
Type = Bug
Type = Chore
Type = Enhancement
Type = Feature
Type = Question
Type = SELFOSS API ISSUE
Type = Tools
Type = UX/Design
Up For Grabs
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Louvorg/ReaderForSelfoss-multiplatform#50
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:repository_tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Types of changes
This closes issue #49
The last two tests I added on reloadBadges fail, the function will have to be fixed.
Wouldn't it be better if SelfossApi.spouts() returned a List rather than a Map?
It would simplify the logic and conform with other api functions.
WIP: Repository Unit Teststo Repository Unit TestsI have added all the tests I could.
I had some problems mocking the functions required to mark items as read/starred because of some problems with expect functions, which I'm unsure how to go about for mocking.
Several tests fail, I believe I have written the tests to account for the expected behavior of these functions, thus these should be corrected.
@ -269,6 +269,7 @@ class ArticleFragment : Fragment(), DIAware {
private fun getContentFromMercury(customTabsIntent: CustomTabsIntent) {
if (repository.isNetworkAvailable()) {
binding.progressBar.visibility = View.VISIBLE
// TODO: The api should be accessed through the repository
This is a simple api call to fetch the artcile content. There is no need to put it inside the repository. It is only used here.
Yes, I do understand.
I suggest this edit just because it would allow to keep all information about the connectivity private within the repository and increase incapsulation.
But there is no real significant difference.
@ -39,3 +40,3 @@
init {
// TODO: Dispatchers.IO not available in KMM, an alternative solution should be found
CoroutineScope(Dispatchers.Main).launch {
runBlocking {
What does this change do ?
This does not change the behavior of the function at all.
runBlocking and Dispatchers.Main are basically the same thing.
However, the testing framework for the common code does not support Dispatchers.
This edit serves pretty much just to make the tests work properly.
I'm certain there is some way to make it work keeping the Dispatchers in there, but since these are equivalent I preferred saving some time and just slightly change the function.
This is the return format of the api.
@ -0,0 +44,4 @@
}
@Test
fun `Instantiate repository`() {
This does not seem to do anything. Is it needed ?
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 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.
@ -0,0 +49,4 @@
}
@Test
fun `Instantiate repository without api version`() {
This does not seem to do anything. Is it needed ?
Same as above.
@ -0,0 +57,4 @@
}
@Test
fun `Instantiate repository with negative stats`() {
This does not seem to do anything. Is it needed ?
Same as above.
@ -0,0 +287,4 @@
}
assertSame(false, success)
assertSame(0, repository.badgeAll)
Be cause of the line 40 of the
setup
function you have an unread/starred item in your DB.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.
I answerd this in another comment.
The DB should be empty when the setting is disabled, so this should never happen.
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.
I find it useless to test if caching is enabled to check if there is something in the DB.
It should be quicker and prevents errors in case some problems with DB clearing happens.
Still, db cleaning should be done on setting change.
@ -0,0 +388,4 @@
TAG("second_DB", "yellow", 0))
coEvery { api.tags() } returns SelfossModel.StatusAndData(success = true, data = tags)
coEvery { db.tagsQueries.tags().executeAsList() } returns tagsDB
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.
Returning
null
isn't possible, so it should be an empty list.Why is tags caching handled by update_sources?
Because it's handled separatly, so users can cache the sources/tags without caching anything else.
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.
@ -0,0 +398,4 @@
testTags = repository.getTags()
}
assertSame(null, testTags)
this should be
assertEquals(emptyList(), testTags)
@ -0,0 +400,4 @@
assertSame(null, testTags)
coVerify(exactly = 0) { api.tags() }
verify(exactly = 0) { db.tagsQueries.tags().executeAsList() }
This will be called once.
@ -0,0 +442,4 @@
}
@Test
fun `get sources without connection and items caching disabled`() {
The same comments about
Get tags without connection and items caching disabled
should be considered here.@ -0,0 +602,4 @@
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var response = false
runBlocking {
response = repository.updateRemote()
Update remote should be changed to.
@ -0,0 +714,4 @@
items = repository.tryToCacheItemsAndGetNewOnes()
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
This should be changed to
coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }
This function is supposed to store articles for the background; it should store all articles independently of the tags selected by the user in the foreground.
I don't know what you wanted to test.
The thing is, passing
null, null, null, null
in the function will make it fail, because in your calls, you pass data.I expect the function to behave differently from how it is currently behaving.
Expected behavior:
When background sync starts, the last 200 articles for each item type are cached.
Thus I expect these 3 calls
getItems("unread", 0, null, null, null, null, 200)
getItems("all", 0, null, null, null, null, 200)
getItems("starred", 0, null, null, null, 200)
If one of those nulls is substituted by another value, it will not be the last 200 articles that will be stored, but 200 articles filtered according to some external factor.
This is not what I would expect as an user, I would expect that the latest articles are always available if I enable background syncronization.
Well, this is because you set filters from lines 701 to 711. When using the app, no filter would be set when calling this method, so you'll have what you expect.
No, filters can be set when this method is called.
This method should deliberately ignore filters.
Imagine this:
Since the repository stores the filter internally, the filter will be set for this background function as well.
This function should make the call without any filter even when filters are set in the repository.
Ok.
So
getMaxItemsForBackground
should be changed, then.@ -0,0 +715,4 @@
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
assertSame(3, items.size)
tryToCacheItemsAndGetNewOnes
will only return1
item as it only returns new items.You will also need to add
every { db.itemsQueries.transaction(any(), any()) } returns Unit
to the setup.@ -0,0 +739,4 @@
items = repository.tryToCacheItemsAndGetNewOnes()
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
This should be
coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }
As above
As above.
I don't know what you wanted to test.
The thing is, passing null, null, null, null in the function will make it fail, because in your calls, you pass data.
@ -0,0 +328,4 @@
testTags = repository.getTags()
}
assertNotSame(tags, testTags)
I don't understand what you wanted to test here. The network is available, so the two will be the same.
If update_sources handles tags fetching, then tags should not be fetched using the api.
The network is available. The api call will be done.
update_sources
handles the tags/sources caching.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.
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.Users should be able to store only tags and sources, but
items_caching
should cache everything, you are right.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.
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.
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.
Yep, you could do that.
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.
@ -0,0 +329,4 @@
}
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
Same here, the two values are different in the initialization, they couldn't be the same after.
I would expect tags to be fetched from the database, since the api should not be used.
The network is available. The api call will be done. update_sources handles the tags/sources caching.
@ -0,0 +261,4 @@
success = repository.reloadBadges()
}
assertSame(true, success)
In
reloadBadges()
, there is a missingsuccess = true
in the else statement.@ -0,0 +263,4 @@
assertSame(true, success)
assertSame(1, repository.badgeAll)
assertSame(0, repository.badgeUnread)
Be cause of the line 40 of the setup function you have an unread/starred item in your DB.
Right, I definitely have to define these things within the functions to avoid these kinds of errors.
@ -0,0 +330,4 @@
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
coVerify(exactly = 0) { api.tags() }
The network is available, so this will be called. And the next one won't.
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.
The setting name IS confusing. But but it handles the tags/sources caching.
@ -0,0 +46,4 @@
@Test
fun `Instantiate repository`() {
val success = try {
As mentionnend in my previous comment. The try catch must be in the code, not in the unit test.
Same for all the other try catch in here.
I'm not sure how I would implement this test otherwise, I guess the repository should raise an exception during initialization then.
Yep. That should be the case.
@ -0,0 +349,4 @@
var testTags: List<SelfossModel.Tag> = emptyList()
runBlocking {
testTags = repository.getTags()
testTags = repository.getTags()
Why is this called twice ?
To test that api.tags() gets called only once regardless of how many calls are made to the repository.
There are still multiple failing tests.
@davidoskky can you also please rebase your work on master, please ?
All tests pass now
@ -249,6 +249,7 @@ class ArticleFragment : Fragment(), DIAware {
private fun getContentFromMercury() {
if (repository.isNetworkAvailable()) {
binding.progressBar.visibility = View.VISIBLE
// TODO: The api should be accessed through the repository
Please remove this comment.
@ -382,6 +403,8 @@ class Repository(private val api: SelfossApi, private val appSettingsService: Ap
}
}
// TODO: This should be private (since all api calls are made through the repository
Because of the mercury api using this, can you remove this comment, please ?
@ -435,1 +458,4 @@
// TODO: This function should check for duplicate items
suspend fun tryToCacheItemsAndGetNewOnes(): List<SelfossModel.Item> {
try {
val previousNewItems = getDBItems().count { it.unread }
This is useless.
@ -391,3 +414,4 @@
db.actionsQueries.deleteAction(action.id)
// TODO: This function should be private
fun getDBTags(): List<TAG> = db.tagsQueries.tags().executeAsList()
If it's not used elsewere, please remove the todo, and change the function to private.
Same for the other todos.
getDBTags and getDBSources are used in the home activity.
It would be better in my opinion to remove this direct access to the database and let the repository check all the logical steps.
I feel that refactoring that is a bit out of the scope of this PR though.
Yes. Could you add a comment in the home activity, the refactoring will be done later.
@ -432,3 +457,2 @@
suspend fun tryToCacheItemsAndGetNewOnes(): List<SelfossModel.Item>? {
// TODO: This function should check for duplicate items
I think that the DB handles the duplicates.
Does it use the id to check for duplicates? If that is the case then all is good.
@ -441,2 +461,4 @@
insertDBItems(fullItemsList)
return fullItemsList
} catch (e: Throwable) {
// We do nothing
As the function name describes, this must only return the new items.
Ops, you're right I made a mistake while changing things...