Repository Unit Tests #50

Merged
AmineL merged 38 commits from davidoskky/ReaderForSelfoss-multiplatform:repository_tests into master 2022-09-30 11:31:55 +00:00
Contributor

Types of changes

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This is NOT translation related.

This closes issue #49

## Types of changes - [x] I have read the **CONTRIBUTING** document. - [x] My code follows the code style of this project. - [ ] I have updated the documentation accordingly. - [x] I have added tests to cover my changes. - [ ] All new and existing tests passed. - [x] This is **NOT** translation related. This closes issue #49
davidoskky added 1 commit 2022-09-09 11:46:06 +00:00
Initial testing setup
All checks were successful
continuous-integration/drone/pr Build is passing
99f2c04bf6
davidoskky added 1 commit 2022-09-10 07:08:44 +00:00
Normal items fetch test
All checks were successful
continuous-integration/drone/pr Build is passing
5853a19937
davidoskky added 1 commit 2022-09-10 07:37:29 +00:00
Check that the api is being used rather than the db
All checks were successful
continuous-integration/drone/pr Build is passing
60c24fc75a
davidoskky added 1 commit 2022-09-15 12:08:04 +00:00
Add item fetching tests
All checks were successful
continuous-integration/drone/pr Build is passing
a4636cc0c8
davidoskky added 1 commit 2022-09-16 10:04:24 +00:00
Test badge fetching
All checks were successful
continuous-integration/drone/pr Build is passing
cda3ba6cb4
Author
Contributor

The last two tests I added on reloadBadges fail, the function will have to be fixed.

The last two tests I added on reloadBadges fail, the function will have to be fixed.
davidoskky added 1 commit 2022-09-17 19:29:48 +00:00
Add CI test step
Some checks failed
continuous-integration/drone/pr Build is failing
c0381144d1
davidoskky added 1 commit 2022-09-17 20:04:38 +00:00
Tags tests
Some checks failed
continuous-integration/drone/pr Build is failing
758708e18d
Author
Contributor

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.

Wouldn't it be better if SelfossApi.spouts() returned a List<Spout> rather than a Map? It would simplify the logic and conform with other api functions.
davidoskky added 1 commit 2022-09-18 16:14:26 +00:00
get sources tests
Some checks failed
continuous-integration/drone/pr Build is failing
ef994460c1
davidoskky added 14 commits 2022-09-25 20:02:40 +00:00
Version scripts.
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/tag Build is passing
1dfa3c9f07
Trying to fix fdroid build.
All checks were successful
continuous-integration/drone/tag Build is passing
d0d6a4378c
Changelog.
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/tag Build is passing
0473a5f7bc
Shorter description.
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/tag Build is passing
e2411c00d8
Fixed release build issues.
Some checks are pending
continuous-integration/drone/push Build is running
continuous-integration/drone/tag Build is running
01763556b1
drone-sign (#57)
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/tag Build is passing
f4db02521d
## Types of changes

- [ ] I have read the **CONTRIBUTING** document.
- [ ] My code follows the code style of this project.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
- [ ] This is **NOT** translation related.

This closes issue #XXX

This is implements feature #YYY

This finishes chore #ZZZ

Co-authored-by: aminecmi <aminecmi@gmail.com>
Reviewed-on: https://gitea.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/pulls/57
added badge.
All checks were successful
continuous-integration/drone/push Build is passing
4c12c9d570
Cleaning.
Some checks are pending
continuous-integration/drone/push Build is running
270d959ee0
Fixing #54.
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
0264da8ccc
Merge pull request 'bugfix/ktor-404' (#62) from bugfix/ktor-404 into master
All checks were successful
continuous-integration/drone/push Build is passing
da71de6806
Reviewed-on: https://gitea.amine-louveau.fr/Louvorg/ReaderForSelfoss-multiplatform/pulls/62
# Conflicts:
#	.drone.yml
Adjust tests to changes in the data models
All checks were successful
continuous-integration/drone/pr Build is passing
366b2e10f1
davidoskky added 1 commit 2022-09-26 20:22:01 +00:00
Test create source
Some checks are pending
continuous-integration/drone/pr Build is running
63c550ead3
davidoskky added 1 commit 2022-09-26 20:26:14 +00:00
Test delete source
All checks were successful
continuous-integration/drone/pr Build is passing
71c0a4d340
davidoskky added 1 commit 2022-09-26 20:42:39 +00:00
Test update remote
Some checks are pending
continuous-integration/drone/pr Build is running
bf6f1a917e
davidoskky added 1 commit 2022-09-26 20:46:49 +00:00
Test login
Some checks are pending
continuous-integration/drone/pr Build is running
f46f98cef0
davidoskky added 1 commit 2022-09-26 20:51:08 +00:00
Test refresh login information
All checks were successful
continuous-integration/drone/pr Build is passing
3f0a3903ae
davidoskky added 1 commit 2022-09-26 21:11:40 +00:00
Test item caching
Some checks are pending
continuous-integration/drone/pr Build is running
a382fc89ea
davidoskky added 1 commit 2022-09-26 21:19:44 +00:00
Add comment
All checks were successful
continuous-integration/drone/pr Build is passing
e2afff0b8e
davidoskky changed title from WIP: Repository Unit Tests to Repository Unit Tests 2022-09-26 21:20:11 +00:00
Author
Contributor

I 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.

I 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.
AmineL reviewed 2022-09-27 13:15:21 +00:00
@ -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
Owner

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.

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.
Author
Contributor

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.

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.
AmineL marked this conversation as resolved
@ -39,3 +40,3 @@
init {
// TODO: Dispatchers.IO not available in KMM, an alternative solution should be found
CoroutineScope(Dispatchers.Main).launch {
runBlocking {
Owner

What does this change do ?

What does this change do ?
Author
Contributor

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 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.
AmineL marked this conversation as resolved
Owner

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.

This is the return format of the api.

> Wouldn't it be better if SelfossApi.spouts() returned a List<Spout> rather than a Map? > It would simplify the logic and conform with other api functions. This is the return format of the api.
AmineL reviewed 2022-09-27 14:14:33 +00:00
@ -0,0 +44,4 @@
}
@Test
fun `Instantiate repository`() {
Owner

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

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

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.
Owner

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.
AmineL marked this conversation as resolved
@ -0,0 +49,4 @@
}
@Test
fun `Instantiate repository without api version`() {
Owner

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

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

Same as above.

Same as above.
AmineL marked this conversation as resolved
@ -0,0 +57,4 @@
}
@Test
fun `Instantiate repository with negative stats`() {
Owner

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

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

Same as above.

Same as above.
AmineL marked this conversation as resolved
@ -0,0 +287,4 @@
}
assertSame(false, success)
assertSame(0, repository.badgeAll)
Owner

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.
Author
Contributor

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.
Owner

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.
Author
Contributor

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.
Owner

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.
Author
Contributor

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.
Owner

Still, db cleaning should be done on setting change.

Still, db cleaning should be done on setting change.
AmineL marked this conversation as resolved
@ -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
Owner

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.
Owner

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

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

Why is tags caching handled by update_sources?

Why is tags caching handled by update_sources?
Owner

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.
Author
Contributor

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

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.
AmineL marked this conversation as resolved
@ -0,0 +398,4 @@
testTags = repository.getTags()
}
assertSame(null, testTags)
Owner

this should be assertEquals(emptyList(), testTags)

this should be `assertEquals(emptyList(), testTags)`
AmineL marked this conversation as resolved
@ -0,0 +400,4 @@
assertSame(null, testTags)
coVerify(exactly = 0) { api.tags() }
verify(exactly = 0) { db.tagsQueries.tags().executeAsList() }
Owner

This will be called once.

This will be called once.
AmineL marked this conversation as resolved
@ -0,0 +442,4 @@
}
@Test
fun `get sources without connection and items caching disabled`() {
Owner

The same comments about Get tags without connection and items caching disabled should be considered here.

The same comments about `Get tags without connection and items caching disabled` should be considered here.
AmineL marked this conversation as resolved
@ -0,0 +602,4 @@
val repository = Repository(api, appSettingsService, connectivityStatus, db)
var response = false
runBlocking {
response = repository.updateRemote()
Owner

Update remote should be changed to.

    suspend fun updateRemote(): Boolean {
        return if (isNetworkAvailable()) {
            api.update().data.equals("finished")
        } else {
            false
        }
    }
Update remote should be changed to. ``` suspend fun updateRemote(): Boolean { return if (isNetworkAvailable()) { api.update().data.equals("finished") } else { false } } ```
AmineL marked this conversation as resolved
@ -0,0 +714,4 @@
items = repository.tryToCacheItemsAndGetNewOnes()
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
Owner

This should be changed to coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }

This should be changed to `coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }`
Author
Contributor

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.

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.
Owner

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 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.
Author
Contributor

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.

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.
Owner

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.

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.

> 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. 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.
Author
Contributor

No, filters can be set when this method is called.
This method should deliberately ignore filters.

Imagine this:

  • Open the app
  • Select one tag
  • The background sync starts at this moment

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.

No, filters can be set when this method is called. This method should deliberately ignore filters. Imagine this: - Open the app - Select one tag - The background sync starts at this moment 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.
Owner

Ok.

So getMaxItemsForBackground should be changed, then.

Ok. So `getMaxItemsForBackground` should be changed, then.
AmineL marked this conversation as resolved
@ -0,0 +715,4 @@
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
assertSame(3, items.size)
Owner

tryToCacheItemsAndGetNewOnes will only return 1 item as it only returns new items.

You will also need to add every { db.itemsQueries.transaction(any(), any()) } returns Unit to the setup.

`tryToCacheItemsAndGetNewOnes` will only return `1` item as it only returns new items. You will also need to add `every { db.itemsQueries.transaction(any(), any()) } returns Unit` to the setup.
AmineL marked this conversation as resolved
@ -0,0 +739,4 @@
items = repository.tryToCacheItemsAndGetNewOnes()
}
coVerify(exactly = 3) { api.getItems(any(), 0, null, null, null, null, 200) }
Owner

This should be coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }

This should be `coVerify(exactly = 3) { api.getItems(any(), 0, "Tag", 1, "search", null, 200) }`
Author
Contributor

As above

As above
Owner

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.

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.
AmineL marked this conversation as resolved
AmineL reviewed 2022-09-27 14:21:20 +00:00
@ -0,0 +328,4 @@
testTags = repository.getTags()
}
assertNotSame(tags, testTags)
Owner

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.
Author
Contributor

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.
Owner

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.
Author
Contributor

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.
Owner

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.
Author
Contributor

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.
Owner

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.
AmineL marked this conversation as resolved
@ -0,0 +329,4 @@
}
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
Owner

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.
Author
Contributor

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.
Owner

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.
AmineL marked this conversation as resolved
AmineL requested changes 2022-09-27 14:32:18 +00:00
@ -0,0 +261,4 @@
success = repository.reloadBadges()
}
assertSame(true, success)
Owner

In reloadBadges(), there is a missing success = true in the else statement.

In `reloadBadges()`, there is a missing `success = true` in the else statement.
AmineL marked this conversation as resolved
@ -0,0 +263,4 @@
assertSame(true, success)
assertSame(1, repository.badgeAll)
assertSame(0, repository.badgeUnread)
Owner

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.
Author
Contributor

Right, I definitely have to define these things within the functions to avoid these kinds of errors.

Right, I definitely have to define these things within the functions to avoid these kinds of errors.
AmineL marked this conversation as resolved
@ -0,0 +330,4 @@
assertNotSame(tags, testTags)
assertSame(tagsDB.first().name, testTags?.first()?.tag)
coVerify(exactly = 0) { api.tags() }
Owner

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.
Author
Contributor

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.
Owner

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.
AmineL marked this conversation as resolved
davidoskky added 1 commit 2022-09-27 21:16:48 +00:00
Add test cases for repository instantiation cases
All checks were successful
continuous-integration/drone/pr Build is passing
41c951b659
davidoskky added 3 commits 2022-09-27 21:37:47 +00:00
davidoskky added 1 commit 2022-09-27 21:44:55 +00:00
Remove unnecessary safe calls
All checks were successful
continuous-integration/drone/pr Build is passing
4781e30da2
AmineL requested changes 2022-09-28 06:46:38 +00:00
@ -0,0 +46,4 @@
@Test
fun `Instantiate repository`() {
val success = try {
Owner

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.
Owner

Same for all the other try catch in here.

Same for all the other try catch in here.
Author
Contributor

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.
Owner

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.
AmineL marked this conversation as resolved
@ -0,0 +349,4 @@
var testTags: List<SelfossModel.Tag> = emptyList()
runBlocking {
testTags = repository.getTags()
testTags = repository.getTags()
Owner

Why is this called twice ?

Why is this called twice ?
Author
Contributor

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.
AmineL marked this conversation as resolved
Owner

image

There are still multiple failing tests.

![image](/attachments/d57c4850-0a2e-4a50-a00f-ae32cf0ee947) There are still multiple failing tests.
158 KiB
Owner

@davidoskky can you also please rebase your work on master, please ?

@davidoskky can you also please rebase your work on master, please ?
davidoskky added 1 commit 2022-09-28 06:57:04 +00:00
Merge branch 'master' into repository_tests
Some checks failed
continuous-integration/drone/pr Build is failing
15ec0f2d26
davidoskky added 8 commits 2022-09-29 23:19:41 +00:00
Author
Contributor

All tests pass now

All tests pass now
davidoskky added 1 commit 2022-09-30 07:04:10 +00:00
Merge branch 'master' into repository_tests
Some checks are pending
continuous-integration/drone/pr Build is running
28b950f467
davidoskky added 1 commit 2022-09-30 07:12:09 +00:00
Remove unnecessary return value
Some checks failed
continuous-integration/drone/pr Build is failing
6f60ef4346
davidoskky added 1 commit 2022-09-30 09:23:57 +00:00
Always cache images in background
All checks were successful
continuous-integration/drone/pr Build is passing
f9ba13dc32
AmineL reviewed 2022-09-30 09:43:37 +00:00
@ -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
Owner

Please remove this comment.

Please remove this comment.
AmineL marked this conversation as resolved
@ -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
Owner

Because of the mercury api using this, can you remove this comment, please ?

Because of the mercury api using this, can you remove this comment, please ?
AmineL marked this conversation as resolved
@ -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 }
Owner

This is useless.

This is useless.
AmineL marked this conversation as resolved
AmineL requested changes 2022-09-30 09:48:57 +00:00
@ -391,3 +414,4 @@
db.actionsQueries.deleteAction(action.id)
// TODO: This function should be private
fun getDBTags(): List<TAG> = db.tagsQueries.tags().executeAsList()
Owner

If it's not used elsewere, please remove the todo, and change the function to private.

Same for the other todos.

If it's not used elsewere, please remove the todo, and change the function to private. Same for the other todos.
Author
Contributor

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.

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.
Owner

Yes. Could you add a comment in the home activity, the refactoring will be done later.

Yes. Could you add a comment in the home activity, the refactoring will be done later.
AmineL marked this conversation as resolved
@ -432,3 +457,2 @@
suspend fun tryToCacheItemsAndGetNewOnes(): List<SelfossModel.Item>? {
// TODO: This function should check for duplicate items
Owner

I think that the DB handles the duplicates.

I think that the DB handles the duplicates.
Author
Contributor

Does it use the id to check for duplicates? If that is the case then all is good.

Does it use the id to check for duplicates? If that is the case then all is good.
AmineL marked this conversation as resolved
davidoskky added 1 commit 2022-09-30 09:49:46 +00:00
Cleanup
Some checks are pending
continuous-integration/drone/pr Build is running
27bb056397
davidoskky added 1 commit 2022-09-30 09:59:19 +00:00
Cleanup
Some checks failed
continuous-integration/drone/pr Build is failing
8dc3d319cd
AmineL requested changes 2022-09-30 10:00:10 +00:00
@ -441,2 +461,4 @@
insertDBItems(fullItemsList)
return fullItemsList
} catch (e: Throwable) {
// We do nothing
Owner

As the function name describes, this must only return the new items.

As the function name describes, this must only return the new items.
Author
Contributor

Ops, you're right I made a mistake while changing things...

Ops, you're right I made a mistake while changing things...
davidoskky added 2 commits 2022-09-30 11:20:27 +00:00
Remove unnecessary call to api
All checks were successful
continuous-integration/drone/pr Build is passing
22da30eaa8
AmineL merged commit 6ec3e96909 into master 2022-09-30 11:31:55 +00:00
davidoskky deleted branch repository_tests 2022-09-30 12:30:54 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Louvorg/ReaderForSelfoss-multiplatform#50
No description provided.