bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
860 stars 307 forks source link

test: consolidate esplora blocking and async test code #1113

Open notmandatory opened 1 year ago

notmandatory commented 1 year ago

There should be a way to consolidate blocking and async tests and TestEnv (probably with macros as Alekos originally did).

_Originally posted by @notmandatory in https://github.com/bitcoindevkit/bdk/pull/1110#discussion_r1316601369_

vladimirfomene commented 1 year ago

@notmandatory do you think this can be labelled as a good first issue?

ikeogu commented 7 months ago

Good day team, can I contribute to this? Although I will need some briefing .

notmandatory commented 7 months ago

@ikeogu yes feel free to work on this, but first look over the code and post here or ask on discord if you have any questions before you get started.

srikanth-iyengar commented 7 months ago

@notmandatory I would like to contribute to this issue Correct me if I am wrong, Currently we have two separate tests for each blocking and async, and as you mention it is possible to have common tests covering both blocking and async ones

notmandatory commented 7 months ago

@srikanth-iyengar I think we need to figure out #1375 first, however that gets solved we should be able to use the same approach for this issue. What are your thoughts regarding the best way to consolidate async and blocking rust code?

srikanth-iyengar commented 7 months ago

I was thinking of something like

#[test_sync_async()]
fn test_both_sync_async() {
    let some_value = some_macro!( // this macro will decide which one to pick async or blocking 
         async { async_function().await },
        || sync_function()
    );
}

Will this be possible ? or we should go with synca

oleonardolima commented 1 month ago

@notmandatory As #1375 has been closed, do you still see the need for this one?

ValuedMammal commented 1 month ago

Not sure if this is the right issue to discuss it but I wanted to mention that test_finalize_chain_update takes a long time to complete (~56 seconds on my machine to run both blocking and async versions). I think it should be rewritten perhaps by consolidating the test cases, mining fewer blocks, and making less calls to esplora

oleonardolima commented 1 month ago

Not sure if this is the right issue to discuss it but I wanted to mention that test_finalize_chain_update takes a long time to complete (~56 seconds on my machine to run both blocking and async versions).

Oh, that's a lot. Thanks for bringing that up, I wasn't aware of it.

I think it should be rewritten perhaps by consolidating the test cases, mining fewer blocks, and making less calls to esplora

I think your suggestions are suitable, but regarding consolidating I guess we would still need something like synca.