Refactor testing logic from macros to pure functions #624

rajarshimaitra · 2022-06-06T11:43:02Z

Description

This finishes the Summer of Bitcoin Task #481 which aimed at refactoring the bvlockchain tests from macros to functions. This allows easier readability of the test logic using any kind of rust analysis tool and allows easy contribution by new contributors. Thanks to SoB particiapant @saikishore222 and @SanthoshAnguluri for the work.

This PR also refactors the same for database tests and moves the test artifacts into their own helper module.

Notes to the reviewers

I am not sure if this requires changelog update. But Added an entry over there anyway as its quite a big change.
Now that individual blockchain tests are to be specified in each module, please check that I am not missing any test cases.

UPDATE: This PR now includes the fix for #719 . The fix is included here for easier review and to reduce duplicate review works. The last two commits includes the bug fix.

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 just reviewed the first two commits for now, the changes looks good, but I'd avoid saying that the first commit is "move only": you're also changing the logic (see txin comment), and refactoring populate_test_db into a function.

It might be easier to review if the first commit really was move-only, as we could use --color-moved=dimmed-zebra, but I understand that it might be a bit of a hassle...

src/testutils/helpers.rs

rajarshimaitra · 2022-08-11T13:19:15Z

Thanks for the look @danielabrozzoni ..

It might be easier to review if the first commit really was move-only, as we could use --color-moved=dimmed-zebra, but I understand that it might be a bit of a hassle...

It didn't made much sense to just do a move and then change logic.. And as I am making it from macro to function, it won't be a pure move anyway even without the logic change.. Might get more confusing than what it is to review.. Reverted the commit message though..

src/testutils/helpers.rs

afilini · 2022-08-16T11:25:12Z

src/wallet/mod.rs

+    #[cfg(test)]
+    /// Return an mutable reference to the internal database
+    pub(crate) fn database_mut(&self) -> impl std::ops::DerefMut<Target = D> + '_ {
+        self.database.borrow_mut()
+    }
+


I don't understand exactly why this is necessary

Me neither.. But when I tried to directly use self.database.borrow_mut() in the function calls the compiler got angry and threw lots of problems that I couldn't subvert, and the easiest thing I found was to have this redundant function.. This does bother me, as we should not need something like this.. But for time being this works.. And I will try to remove it again and see if it can work.. But would prefer to do it in a separate PR..

Another problem faced is previously the populate_test_db macro was in the database module.. Which made accessing database easy.. But now as it has been taken out into testutils::helper we cannot just call wallet.db as that's private variable and only exposed via the database() api.. Which only gives immutable access.. So thats the major reason I had to make a mutable version of the same API.. But only for testing situations..

We can make the Wallet::database field pub(crate).

I tried it here: rajarshimaitra#8

src/testutils/blockchain_tests.rs

rajarshimaitra · 2022-08-17T14:07:25Z

Hey @afilini I have updated the conftime calculation logic. I think now it is reverted as previous.. And it now expects the min_conf time to be lower than current height with a message.

Also changed the blockchain tests to now run with a generic init_blockchain() function.. So I think for external users the test flow now would look like

Create your own init_blockchain() function.
Pass that into the macro.
Invoke the test cases names you wanna test on..

But the user still needs to manually specify the test cases name in the macro.. I couldn't find a good way to automate that part too..

kept the update commit separate for easier review.. But it might also makes sense to squash them, because many stuffs of the previous commits are now redundant..

rajarshimaitra · 2022-08-17T18:04:34Z

I am also wondering if we should document it somewhere that our blockchain tests are public and how to use them. Libraries don't usually expose their tests. So might be helpful for users..

rajarshimaitra · 2022-08-31T13:08:10Z

Thanks @afilini @danielabrozzoni for the initial look.

Pushed some updates

Rebased and resolved conflicts with master. Few changes added here previously are already done in master. Those are removed from this PR.
Restructured all the commits. Including review suggestion and many follow up code updates.
Refer commit messages for a change summary.

This PR is ready for a second look.. :)

evanlinjin

LGTM, however it's really hard to read through all the changes of blockchain_tests.rs, but I am trusting that nothing dodgy is added. Also added some comments here and there.

I would love to see this merged soon so we can get started on https://discord.com/channels/753336465005608961/753367451319926827/1012298169830354966 😄

src/blockchain/esplora/mod.rs

evanlinjin · 2022-09-02T14:07:02Z

src/blockchain/rpc.rs

+        )
+    ];
+
+    #[cfg(not(feature = "test-rpc-legacy"))]


Nit: IMO writing this as #[cfg(feature = "test-rpc")] is better.

On further look, it seems the intention of test-rpc-legacy is a to be a feature to disable certain tests.. So making it test-rpc won't work, as all the rpc tests are by default in test-rpc. This not thing, makes these tests disabled when test-rpc-legacy is turned on.. Maybe there was an easier way to do, but I won't mess with it in this PR..

src/testutils/helpers.rs

rajarshimaitra · 2022-09-03T03:32:19Z

LGTM, however it's really hard to read through all the changes of blockchain_tests.rs, but I am trusting that nothing dodgy is added. Also added some comments here and there.

Please don't trust. Verify! 😅

Ya the diff for the blockchain tests is pretty nasty.. Where as its just a single big move of all the tests out of the macro.. But for some reason that's how git is showing.. Let me know if breaking this diff into smaller pieces can help in review..

One thing most important to verify that we are playing all the previous tests.

No existing test was removed.
The call list at the blockchian module sites are covering everything they were covering previously.

Thanks for the review @evanlinjin I will update them in the next push..

danielabrozzoni · 2022-09-26T12:47:30Z

Moving this one to the next milestone, as it seems that there is still some work to do :)

rajarshimaitra · 2022-09-26T14:52:54Z

Sorry I forgot about the last pending updates.. Yes this can wait for the next release.. There are few conflicts too.. I will resolve them after 0.23 is cut..

- Few test helpers and macros are scattered around `database/memory.rs` and `wallet/mod.rs`. These are collected in a single place `testutils/helpers.rs`. - the `populate_test_db` macro is changed into a function. Internal logic should remain same. - A new `run_tests_with_init` macro is added in `testutils.helpers.rs`, which can run database tests given an initializer function. Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

- in `keyvalue.rs`, `memory.rs`, `sqlite.rs` use the `run_tests_with_init` macro to run database tests. - in `wallet/mod.rs` update all the test calls from macro to function for `populate_test_db`. - some import fixes in `address_validator.rs` and `psbt/mod.rs`. Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

Similar to the database `run_test_with_init` another `make_blockchain_test` macro is added, which will run the blockchain tests given a initilizer function. These test functions are taken out of the macro and are placed as their own public functions. A doc comment explaining how to use the tests externally from bdk is added before the `blockchain_tests::test` module. Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

Apply the new `make_blockchain_test` macro to run the blockchain tests for rpc, electrum and esplora blockchain. Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

rajarshimaitra · 2022-10-10T17:03:01Z

Thanks for dealing with the mammoth this long everybody.. I think we are close to get this one in..

Addressed @evanlinjin 's comments.
Rebased on master..

This is now ready for a second look.. :)

danielabrozzoni · 2023-03-16T17:09:31Z

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

rajarshimaitra marked this pull request as draft June 15, 2022 07:51

rajarshimaitra force-pushed the test-helper-refactor branch 2 times, most recently from 49ef8a8 to daa5dcd Compare July 2, 2022 11:21

danielabrozzoni mentioned this pull request Jul 5, 2022

Avoid using immature coinbase inputs #614

Merged

6 tasks

rajarshimaitra force-pushed the test-helper-refactor branch from 093ef9d to c2e93c7 Compare August 10, 2022 16:30

rajarshimaitra changed the title ~~[Refactor] Test helper functions to their dedicated module~~ Refactor testing logic from macros to pure functions Aug 10, 2022

rajarshimaitra force-pushed the test-helper-refactor branch 3 times, most recently from 76ccf88 to 8de0e74 Compare August 11, 2022 07:31

rajarshimaitra marked this pull request as ready for review August 11, 2022 07:39

danielabrozzoni reviewed Aug 11, 2022

View reviewed changes

src/testutils/helpers.rs Outdated Show resolved Hide resolved

src/testutils/helpers.rs Show resolved Hide resolved

rajarshimaitra force-pushed the test-helper-refactor branch 3 times, most recently from 74ef5e3 to 1220c99 Compare August 11, 2022 13:14

rajarshimaitra mentioned this pull request Aug 14, 2022

[bug-fix] Set the sync time in populate test db. #717

Closed

4 tasks

rajarshimaitra linked an issue Aug 15, 2022 that may be closed by this pull request

[bug] get_funded_wallet() does not return balance #719

Closed

rajarshimaitra mentioned this pull request Aug 15, 2022

Move wallet interoperabity tests out of testutils::blockchain_tests bdk_blockchain_tests macro #615

Closed

afilini reviewed Aug 16, 2022

View reviewed changes

notmandatory mentioned this pull request Aug 16, 2022

W33 BDK Library Team Call bitcoindevkit/.github#18

Closed

22 tasks

rajarshimaitra force-pushed the test-helper-refactor branch from 5bb51a7 to 9047014 Compare August 17, 2022 14:04

rajarshimaitra force-pushed the test-helper-refactor branch 3 times, most recently from 606340f to 1a573ee Compare August 17, 2022 18:49

rajarshimaitra mentioned this pull request Aug 19, 2022

Add a custom signer for hardware wallets #682

Merged

6 tasks

notmandatory assigned rajarshimaitra Aug 26, 2022

rajarshimaitra force-pushed the test-helper-refactor branch from 1a573ee to 5b27deb Compare August 31, 2022 13:04

evanlinjin reviewed Sep 2, 2022

View reviewed changes

danielabrozzoni added this to the Release 0.23.0 Feature Freeze milestone Sep 5, 2022

notmandatory mentioned this pull request Sep 13, 2022

W37 BDK Library Team Call bitcoindevkit/.github#22

Closed

26 tasks

danielabrozzoni modified the milestones: Release 0.23.0 Feature Freeze, Release 0.24.0 Feature Freeze Sep 26, 2022

rajarshimaitra force-pushed the test-helper-refactor branch 3 times, most recently from ff371ae to 9ac9bcc Compare October 10, 2022 16:18

rajarshimaitra and others added 4 commits October 10, 2022 21:51

Apply blockchain test changes

63dd109

Apply the new `make_blockchain_test` macro to run the blockchain tests for rpc, electrum and esplora blockchain. Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

Update CHANGELOG

2e7612d

Co-authored-by: SanthoshAnguluri <[email protected]> Co-authored-by: saikishore222 <[email protected]>

rajarshimaitra force-pushed the test-helper-refactor branch from 9ac9bcc to 2e7612d Compare October 10, 2022 16:22

notmandatory mentioned this pull request Oct 10, 2022

W41 BDK Library Team Call bitcoindevkit/.github#27

Closed

26 tasks

notmandatory removed this from the Release 0.24.0 Feature Freeze milestone Oct 11, 2022

notmandatory mentioned this pull request Nov 20, 2022

W47 BDK Library Team Call bitcoindevkit/.github#33

Closed

19 tasks

danielabrozzoni closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor testing logic from macros to pure functions #624

Refactor testing logic from macros to pure functions #624

rajarshimaitra commented Jun 6, 2022 •

edited

Loading

danielabrozzoni left a comment

rajarshimaitra commented Aug 11, 2022 •

edited

Loading

afilini Aug 16, 2022

rajarshimaitra Aug 17, 2022

rajarshimaitra Aug 17, 2022 •

edited

Loading

evanlinjin Sep 2, 2022 •

edited

Loading

rajarshimaitra commented Aug 17, 2022

rajarshimaitra commented Aug 17, 2022

rajarshimaitra commented Aug 31, 2022

evanlinjin left a comment •

edited

Loading

evanlinjin Sep 2, 2022

rajarshimaitra Sep 3, 2022

rajarshimaitra Oct 10, 2022

rajarshimaitra commented Sep 3, 2022 •

edited

Loading

danielabrozzoni commented Sep 26, 2022

rajarshimaitra commented Sep 26, 2022

rajarshimaitra commented Oct 10, 2022

danielabrozzoni commented Mar 16, 2023

Refactor testing logic from macros to pure functions #624

Refactor testing logic from macros to pure functions #624

Conversation

rajarshimaitra commented Jun 6, 2022 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

danielabrozzoni left a comment

Choose a reason for hiding this comment

rajarshimaitra commented Aug 11, 2022 • edited Loading

afilini Aug 16, 2022

Choose a reason for hiding this comment

rajarshimaitra Aug 17, 2022

Choose a reason for hiding this comment

rajarshimaitra Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

evanlinjin Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented Aug 17, 2022

rajarshimaitra commented Aug 17, 2022

rajarshimaitra commented Aug 31, 2022

evanlinjin left a comment • edited Loading

Choose a reason for hiding this comment

evanlinjin Sep 2, 2022

Choose a reason for hiding this comment

rajarshimaitra Sep 3, 2022

Choose a reason for hiding this comment

rajarshimaitra Oct 10, 2022

Choose a reason for hiding this comment

rajarshimaitra commented Sep 3, 2022 • edited Loading

danielabrozzoni commented Sep 26, 2022

rajarshimaitra commented Sep 26, 2022

rajarshimaitra commented Oct 10, 2022

danielabrozzoni commented Mar 16, 2023

rajarshimaitra commented Jun 6, 2022 •

edited

Loading

rajarshimaitra commented Aug 11, 2022 •

edited

Loading

rajarshimaitra Aug 17, 2022 •

edited

Loading

evanlinjin Sep 2, 2022 •

edited

Loading

evanlinjin left a comment •

edited

Loading

rajarshimaitra commented Sep 3, 2022 •

edited

Loading