Cache test

[larmie56]

What does this PR do?

• Add test for the cache contract

[Ezike]

Try not to add module specific dependencies to the general Android Library plugin

[larmie56]

Okay, thank you

[Ezike]

@larmie56 I added robolectric dependency to the cache module, which seems to provide the remaining classes that were missing in your build.

I also fixed the tests cos they were failing. Changed runblockingTest to just runBlocking{} cos the former was causing tests to fail. There's a thread about the issue .

Also, since the database is generating the id which is also the primary key of the Expense table, I removed the id field from the constructor and made it a class property. This makes it easy to create the ExpenseEntity without supplying an id, since the Db will handle generating ids.

This also fixes a problem where calling ExpenseRepository.getExpense(id) in the test fails. This is caused by a discrepancy in the id or primary key with which we are retrieving an Expense with. The id we had before is no longer the id that the database saved (Since the database generated a new one).
To mitigate this issue, the Dao::insert function now returns a Long which is now the id of the newly created object. We can then use this returned id to get an Expense object with getExpense()

[larmie56]

@larmie56 I added robolectric dependency to the cache module, which seems to provide the remaining classes that were missing in your build.

I also fixed the tests cos they were failing. Changed runblockingTest to just runBlocking{} cos the former was causing tests to fail. There's a thread about the issue .

Also, since the database is generating the id which is also the primary key of the Expense table, I removed the id field from the constructor and made it a class property. This makes it easy to create the ExpenseEntity without supplying an id, since the Db will handle generating ids.

This also fixes a problem where calling ExpenseRepository.getExpense(id) in the test fails. This is caused by a discrepancy in the id or primary key with which we are retrieving an Expense with. The id we had before is no longer the id that the database saved (Since the database generated a new one).
To mitigate this issue, the Dao::insert function now returns a Long which is now the id of the newly created object. We can then use this returned id to get an Expense object with getExpense()

Hi Tobey! Thanks for the detailed explanation, I now understand why the tests were failing/not running.

I was thinking it'll be better not to replace on conflict, in the Dao::insert. When we replace on insertion, I'm not sure we know it's the same expense entry (since update will handle same expense entry). So replacing might cause data to be loss. If I have it figured out wrongly, please can you help clear it up?

[Ezike]

@larmie56 I added robolectric dependency to the cache module, which seems to provide the remaining classes that were missing in your build.
I also fixed the tests cos they were failing. Changed runblockingTest to just runBlocking{} cos the former was causing tests to fail. There's a thread about the issue .
Also, since the database is generating the id which is also the primary key of the Expense table, I removed the id field from the constructor and made it a class property. This makes it easy to create the ExpenseEntity without supplying an id, since the Db will handle generating ids.
This also fixes a problem where calling ExpenseRepository.getExpense(id) in the test fails. This is caused by a discrepancy in the id or primary key with which we are retrieving an Expense with. The id we had before is no longer the id that the database saved (Since the database generated a new one).
To mitigate this issue, the Dao::insert function now returns a Long which is now the id of the newly created object. We can then use this returned id to get an Expense object with getExpense()

Hi Tobey! Thanks for the detailed explanation, I now understand why the tests were failing/not running.

I was thinking it'll be better not to replace on conflict, in the Dao::insert. When we replace on insertion, I'm not sure we know it's the same expense entry (since update will handle same expense entry). So replacing might cause data to be loss. If I have it figured out wrongly, please can you help clear it up?

If two expense objects have the same ID, then they're assumed to be the same. The insert method will check if a record with that primary key already exists before deciding what to do. That's why the on conflict strategy exists.
Or maybe I don't understand what you mean.

[larmie56]

@larmie56 I added robolectric dependency to the cache module, which seems to provide the remaining classes that were missing in your build.
I also fixed the tests cos they were failing. Changed runblockingTest to just runBlocking{} cos the former was causing tests to fail. There's a thread about the issue .
Also, since the database is generating the id which is also the primary key of the Expense table, I removed the id field from the constructor and made it a class property. This makes it easy to create the ExpenseEntity without supplying an id, since the Db will handle generating ids.
This also fixes a problem where calling ExpenseRepository.getExpense(id) in the test fails. This is caused by a discrepancy in the id or primary key with which we are retrieving an Expense with. The id we had before is no longer the id that the database saved (Since the database generated a new one).
To mitigate this issue, the Dao::insert function now returns a Long which is now the id of the newly created object. We can then use this returned id to get an Expense object with getExpense()

Hi Tobey! Thanks for the detailed explanation, I now understand why the tests were failing/not running.
I was thinking it'll be better not to replace on conflict, in the Dao::insert. When we replace on insertion, I'm not sure we know it's the same expense entry (since update will handle same expense entry). So replacing might cause data to be loss. If I have it figured out wrongly, please can you help clear it up?

If two expense objects have the same ID, then they're assumed to be the same. The insert method will check if a record with that primary key already exists before deciding what to do. That's why the on conflict strategy exists.
Or maybe I don't understand what you mean.

Okay it clears it up. Thanks alot

[larmie56]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

[Ezike]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.

I think the id stuff I did might be wrong even, but I'll check it further

[Ezike]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.

I think the id stuff I did might be wrong even, but I'll check it further

Immutability helps to eliminate some family of bugs. So we will mostly use read only properties everywhere.

[larmie56]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.

I think the id stuff I did might be wrong even, but I'll check it further

Okay, thank you for pointing out the

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.
I think the id stuff I did might be wrong even, but I'll check it further

Immutability helps to eliminate some family of bugs. So we will mostly use read only properties everywhere.

Yeaa... I was thinking of a way to update without altering the immutability, but then I remembered a couple of repos I had seen that used mutable fields, so I just decided to do that. Thank you for pointing it out

[larmie56]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.

I think the id stuff I did might be wrong even, but I'll check it further

Okay thank you, I will fix them. And about the id, I think it can stay in the constructor, still as a default parameter. But that doesn't solve the mutability. Also, in the Kotlin fundamental course, they used an example where the id was mutable. Although that might be because they were more concerned with teaching a concept, than immutability.

[Ezike]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.
I think the id stuff I did might be wrong even, but I'll check it further

Okay thank you, I will fix them. And about the id, I think it can stay in the constructor, still as a default parameter. But that doesn't solve the mutability. Also, in the Kotlin fundamental course, they used an example where the id was mutable. Although that might be because they were more concerned with teaching a concept, than immutability.

I think we can move it back to the constructor then. It may need to stay mutable since the database is supposed to increment it. But I'm not sure, you can confirm

[larmie56]

I changed ExpenseEntity.info to a var, to allow updating. I didn't feel any other field needed mutability (for now), so I left them as they were

Nope. Copy the existing expense object in order to create a new one with the updated info. We only use vars in very exceptional cases. Also remove the !! and figure out a different way to write your update test without these two things.
I think the id stuff I did might be wrong even, but I'll check it further

Okay thank you, I will fix them. And about the id, I think it can stay in the constructor, still as a default parameter. But that doesn't solve the mutability. Also, in the Kotlin fundamental course, they used an example where the id was mutable. Although that might be because they were more concerned with teaching a concept, than immutability.

I think we can move it back to the constructor then. It may need to stay mutable since the database is supposed to increment it. But I'm not sure, you can confirm

Okay sure, I'll confirm it

[larmie56]

I tried to move the id to the constructor but some previous tests were failing. And from what I found after doing some searching, doing it the way you did it was a sort of norm. So I left it the way you did it

[Ezike]

I tried to move the id to the constructor but some previous tests were failing. And from what I found after doing some searching, doing it the way you did it was a sort of norm. So I left it the way you did it

Yea I noticed moving the id out of constructor makes the tests pass cos the id is no longer used in measuring whether two objects are equal. I guess it only looks at fields in the constructor

[larmie56]

I tried to move the id to the constructor but some previous tests were failing. And from what I found after doing some searching, doing it the way you did it was a sort of norm. So I left it the way you did it

Yea I noticed moving the id out of constructor makes the tests pass cos the id is no longer used in measuring whether two objects are equal. I guess it only looks at fields in the constructor

Yea I think so. Most of the other tests I saw while searching for options did the same thing

[larmie56]

I don't really understand inserting the expense into the database twice. Is it like to sort of simulate multiple entries?

[Ezike]

Yes

[larmie56]

Okay, all clear now. Thank you

[Ezike]

Please how will we handle update of an entry?

Just insert into the db and it'll replace the existing entry. Hm but with auto generated keys I wonder if that will work. Maybe not

[larmie56]

Please how will we handle update of an entry?

Just insert into the db and it'll replace the existing entry. Hm but with auto generated keys I wonder if that will work. Maybe not

Okay. Yeaaa... the ids might vary

[Ezike]

Please how will we handle update of an entry?

Just insert into the db and it'll replace the existing entry. Hm but with auto generated keys I wonder if that will work. Maybe not

Okay. Yeaaa... the ids might vary

The db will increment the id before inserting, thereby duplicating the record.
Bring back the update stuff and add the test

[larmie56]

Please how will we handle update of an entry?

Just insert into the db and it'll replace the existing entry. Hm but with auto generated keys I wonder if that will work. Maybe not

Okay. Yeaaa... the ids might vary

The db will increment the id before inserting, thereby duplicating the record.
Bring back the update stuff and add the test

Okay, I'll do that

[Ezike]

Nice work