Add tests to improve coverage

[vladimirfomene]

Description

This PR add more test to the database module and also fixes certain bugs discovered by the written test. I also amended the name used for the database parameter in the test functions.

Notes to the reviewers

This contributes to fixing bitcoindevkit/bdk_wallet#179

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

[danielabrozzoni]

I'd like to see b91afdb split into multiple commits: at least one different commit for each bug discovered and fixed, with an explanation of the bug found. You modified quite a bit of code in src/database/keyvalue.rs and src/database/memory.rs, but without a clear commit message it's nearly to impossible to understand your intentions.

[vladimirfomene]

@danielabrozzoni, I have implemented the changes requested and explained reason for the change in the commit message.

[danielabrozzoni]

First of all, some styling nits:

1. You shouldn't put "Fix failing test: test_del_tx" as the commit message for 27c6735 - your commit is not fixing a test. Your commit is fixing a bug you've found. This is very different, as "Fix failing test" makes the reader think that in the commit you are modifying the actual test behavior.
2. This is a quite big nit, but I usually first put commits that fix bugs and then commits that add tests, not the other way around (in this PR d8f5026 is before 27c6735). The reason for that is: it helps me write a clearer commit message, as that I'm not tempted to cite any test in the bug-fixing commit, but also it makes the git history bisectable, which is always a nice to have.

The added tests look good and they actually improve the code coverage :) See https://coveralls.io/builds/52344495

I am a bit confused about 27c6735: to me, it doesn't seem that you need to modify select_transaction_details_by_txid, you can just remove the transaction from the returned object if include_raw is false:

diff --git a/src/database/sqlite.rs b/src/database/sqlite.rs
index 562c934..4fb6337 100644
--- a/src/database/sqlite.rs
+++ b/src/database/sqlite.rs
@@ -744,11 +744,16 @@ impl BatchOperations for SqliteDatabase {
         include_raw: bool,
     ) -> Result<Option<TransactionDetails>, Error> {
         match self.select_transaction_details_by_txid(txid)? {
-            Some(transaction_details) => {
+            Some(mut transaction_details) => {
                 self.delete_transaction_details_by_txid(txid)?;
 
                 if include_raw {
                     self.delete_transaction_by_txid(txid)?;
+                } else {
+                    // Since we want to return what we actually deleted from
+                    // the database, we make sure to put a "None" in the
+                    // transaction field here.
+                    transaction_details.transaction = None;
                 }
                 Ok(Some(transaction_details))
             }

Is there something I'm not seeing?

[vladimirfomene]

@danielabrozzoni, initially I made the transaction field None on the returned object and yes it fixed the bug. The other issue here is that if I was calling get_tx with include_raw = false, behind the scene select_transaction_details_by_txid will get the transaction even though it is not needed as it does a join query on transactions and transactiondetails.

[rajarshimaitra]

tACK e65edbf

Thanks for the new tests..

[danielabrozzoni]

Code review ACK e65edbf