Refactor testing logic from macros to pure functions

[rajarshimaitra]

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:

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

[rajarshimaitra]

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

[rajarshimaitra]

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]

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]

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.. :)

[rajarshimaitra]

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]

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

[rajarshimaitra]

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

[rajarshimaitra]

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]

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!