Refactor: Move transaction testing utilities from `crates/chain/tests/common` to `testenv` crate #1612

tvpeter · 2024-09-14T21:52:14Z

Description

This change moves all transaction testing utilities (macros and functions) from crates/chain/tests/common to crates/testenv/tx_utils to minimize duplication.

Closes #1602

Notes to the reviewers

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

New Features:

I've added tests for the new feature
I've added docs for the new feature

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

evanlinjin · 2024-09-15T05:56:01Z

Hey @tvpeter thank you for taking this on! Please follow the [https://www.conventionalcommits.org/en/v1.0.0/](Conventional Commit Specification) for the commit messages. I'll review this properly soon!

tvpeter · 2024-09-15T12:38:18Z

@evanlinjin thank you.
I have reworded the commit messages to follow the specification.

LagginTimes · 2024-09-16T03:17:20Z

crates/testenv/Cargo.toml

@@ -18,6 +18,7 @@ workspace = true
 [dependencies]
 bdk_chain = { path = "../chain", version = "0.19.0", default-features = false }
 electrsd = { version = "0.28.0", features = [ "bitcoind_25_0", "esplora_a33e97e1", "legacy" ] }
+bitcoin = { version = "0.32.0", default-features = false }


Hello! Since bdk_chain already re-exports bitcoin, we don't need to add bitcoin as a dependency here.

Fixed. Thank you

oleonardolima

Concept ACK

I only wonder about naming/structure, maybe reword it to only utils.rs or have both chain_utils.rs and tx_utils.rs, but I'm not sure about it.

tvpeter · 2024-09-16T18:50:25Z

Alright @oleonardolima. I initially thought that it will contain transaction related utilities only but on a second thought most of the utilities are not just for transactions. So I will rename it to just utils.rs and move the descriptors sample array there too. Thank you.

evanlinjin · 2024-09-26T09:59:06Z

Please use the conventional commits format for commit messages.

Looking at this now, maybe we should create a bdk_testutil crate for this instead? These are light macros, functions. Whereas bdk_testenv requires downloading binaries during the build process.

cc. @ValuedMammal @notmandatory

ValuedMammal · 2024-09-26T13:59:34Z

Looking at this now, maybe we should create a bdk_testutil crate for this instead? These are light macros, functions. Whereas bdk_testenv requires downloading binaries during the build process.

Another option that comes to mind is to have conditional compilation of testenv with something like a "full" feature flag, but I don't have a strong opinion on it at the moment.

LagginTimes · 2024-09-29T13:18:54Z

Looking at this now, maybe we should create a bdk_testutil crate for this instead? These are light macros, functions. Whereas bdk_testenv requires downloading binaries during the build process.

Another option that comes to mind is to have conditional compilation of testenv with something like a "full" feature flag, but I don't have a strong opinion on it at the moment.

I think conditional compilation sounds like the better option here vs. having an additional crate to manage, since both crates would be primarily focused on testing.

ValuedMammal · 2024-09-30T12:56:11Z

Looking at this now, maybe we should create a bdk_testutil crate for this instead? These are light macros, functions. Whereas bdk_testenv requires downloading binaries during the build process.

Another option that comes to mind is to have conditional compilation of testenv with something like a "full" feature flag, but I don't have a strong opinion on it at the moment.

I think conditional compilation sounds like the better option here vs. having an additional crate to manage, since both crates would be primarily focused on testing.

~~Considering the test utils are common to other crates and change infrequently, it might be just as easy to move them to a test_util module in bdk_core~~

Edit: As discussed on dev call the current direction on the PR makes sense

LagginTimes · 2024-10-01T15:44:31Z

Nit: I think squashing the commits down to one would make the history a bit cleaner.

tvpeter · 2024-10-01T16:14:12Z

Nit: I think squashing the commits down to one would make the history a bit cleaner.

Done

oleonardolima

I did another review and left some comments. Overall, it's looking good!

I think you still need to change the commit message to lowercase.

oleonardolima · 2024-10-03T00:38:36Z

crates/chain/tests/test_local_chain.rs

@@ -9,6 +9,7 @@ use bdk_chain::{
    },
    BlockId,
 };
+use bdk_testenv::{chain_update, h, local_chain};


As we are importing this now from bdk_testenv, and deleted the macros from commond/mod.rs I guess we can remove the code at L16-17.

#[macro_use] mod common;

oleonardolima · 2024-10-03T00:41:46Z

crates/testenv/src/utils.rs

+#[allow(unused_macros)]
+#[macro_export]
+macro_rules! h {
+    ($index:literal) => {{
+        bitcoin::hashes::Hash::hash($index.as_bytes())
+    }};
+}


IIRC we discussed during the call that this could have a meaningful name, I would use hash for instance.

oleonardolima · 2024-10-03T00:48:57Z

You also probably need to rebase on top of the master, so the CI issues are fixed (in case you are curious, check: #1636).

tvpeter · 2024-10-03T09:07:54Z

I did another review and left some comments. Overall, it's looking good!

I think you still need to change the commit message to lowercase.

Done. Thank you

LagginTimes · 2024-10-03T10:33:05Z

crates/testenv/src/utils.rs

+macro_rules! local_chain {
+    [ $(($height:expr, $block_hash:expr)), * ] => {{
+        #[allow(unused_mut)]
+        bdk_chain::local_chain::LocalChain::from_blocks([$(($height, $block_hash).into()),*].into_iter().collect())
+            .expect("chain must have genesis block")
+    }};
+}


Nit: I suggest renaming this expression to hash for consistency with the other macros.

The previous h! macro has been renamed to hash!, so it will conflict. Since it builds a LocalChain, is it better to rename it new_local_chain?

Oops! Just to clarify: I meant to change block_hash to hash, not rename the local_chain macro. Apologies for the confusion!

- add `utils` mod to testenv crate to keep all utilities moved from `crates/chain/test/common` - replace all test tx macros in `chain` crate with import from `testenv` crate - tests/test_indexed_tx_graph - tests/test_keychain_txout_index - tests/test_local_chain - tests/test_tx_graph - tests/test_tx_graph_conflicts - deleted all moved macros and functions in `crates/chain/test/common/mod` - replace `bitcoin` external crate with `bd_chain::bitcoin` - move `DESCRIPTORS` sample array to `utils` for other crates to use - rename h! macro to hash! for clarity - rename localchain! macro parameter from 'block_hash' to 'hash' [Ticket: bitcoindevkit#1602]

ValuedMammal

ACK 88a8403

notmandatory

ACK 88a8403

Thanks for taking this one on.

tvpeter mentioned this pull request Sep 14, 2024

Move stuff from crates/chain/tests/common to bdk_testenv and make them publicly-accessible #1602

Closed

tvpeter force-pushed the test/re-org-macros branch from e26be1e to b571fb0 Compare September 15, 2024 12:36

notmandatory assigned tvpeter Sep 15, 2024

notmandatory added the new feature New feature or request label Sep 15, 2024

notmandatory added this to the 1.0.0-beta milestone Sep 15, 2024

LagginTimes reviewed Sep 16, 2024

View reviewed changes

oleonardolima reviewed Sep 16, 2024

View reviewed changes

tvpeter force-pushed the test/re-org-macros branch from 07d931f to fefd376 Compare September 16, 2024 19:40

notmandatory added tests and removed new feature New feature or request labels Sep 19, 2024

oleonardolima mentioned this pull request Sep 19, 2024

Clean up dependency structure #1611

Open

tvpeter force-pushed the test/re-org-macros branch from fefd376 to 78d078a Compare October 1, 2024 16:13

oleonardolima reviewed Oct 3, 2024

View reviewed changes

tvpeter force-pushed the test/re-org-macros branch from 78d078a to e140f25 Compare October 3, 2024 09:05

LagginTimes reviewed Oct 3, 2024

View reviewed changes

tvpeter force-pushed the test/re-org-macros branch from e140f25 to 88a8403 Compare October 3, 2024 12:14

ValuedMammal approved these changes Oct 8, 2024

View reviewed changes

notmandatory approved these changes Oct 14, 2024

View reviewed changes

notmandatory merged commit ad59970 into bitcoindevkit:master Oct 14, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Move transaction testing utilities from `crates/chain/tests/common` to `testenv` crate #1612

Refactor: Move transaction testing utilities from `crates/chain/tests/common` to `testenv` crate #1612

tvpeter commented Sep 14, 2024

evanlinjin commented Sep 15, 2024

tvpeter commented Sep 15, 2024

LagginTimes Sep 16, 2024

tvpeter Sep 16, 2024

oleonardolima left a comment

tvpeter commented Sep 16, 2024

evanlinjin commented Sep 26, 2024

ValuedMammal commented Sep 26, 2024

LagginTimes commented Sep 29, 2024

ValuedMammal commented Sep 30, 2024 •

edited

Loading

LagginTimes commented Oct 1, 2024

tvpeter commented Oct 1, 2024

oleonardolima left a comment

oleonardolima Oct 3, 2024

oleonardolima Oct 3, 2024

oleonardolima commented Oct 3, 2024

tvpeter commented Oct 3, 2024

LagginTimes Oct 3, 2024

tvpeter Oct 3, 2024

LagginTimes Oct 3, 2024

tvpeter Oct 3, 2024

ValuedMammal left a comment

notmandatory left a comment

Refactor: Move transaction testing utilities from crates/chain/tests/common to testenv crate #1612

Refactor: Move transaction testing utilities from crates/chain/tests/common to testenv crate #1612

Conversation

tvpeter commented Sep 14, 2024

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

evanlinjin commented Sep 15, 2024

tvpeter commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima left a comment

Choose a reason for hiding this comment

tvpeter commented Sep 16, 2024

evanlinjin commented Sep 26, 2024

ValuedMammal commented Sep 26, 2024

LagginTimes commented Sep 29, 2024

ValuedMammal commented Sep 30, 2024 • edited Loading

LagginTimes commented Oct 1, 2024

tvpeter commented Oct 1, 2024

oleonardolima left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleonardolima commented Oct 3, 2024

tvpeter commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValuedMammal left a comment

Choose a reason for hiding this comment

notmandatory left a comment

Choose a reason for hiding this comment

Refactor: Move transaction testing utilities from `crates/chain/tests/common` to `testenv` crate #1612

Refactor: Move transaction testing utilities from `crates/chain/tests/common` to `testenv` crate #1612

ValuedMammal commented Sep 30, 2024 •

edited

Loading