Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 6 commits into from

Conversation

tvpeter
Copy link

@tvpeter tvpeter commented Sep 14, 2024

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

@tvpeter tvpeter mentioned this pull request Sep 14, 2024
Open
@evanlinjin
Copy link
Member

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
Copy link
Author

tvpeter commented Sep 15, 2024

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

@notmandatory notmandatory added the new feature New feature or request label Sep 15, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Sep 15, 2024
LagginTimes
LagginTimes reviewed Sep 16, 2024
View reviewed changes
crates/testenv/Cargo.toml Outdated
@@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thank you

oleonardolima
oleonardolima reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

tvpeter commented Sep 16, 2024

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.

@tvpeter
Refactor(test): Move tx test utils to testenv
016692e 
- add `tx_utils` to house all transaction
utilities moved from `crates/chain/test/common`
- deleted all moved macros and functions in
`crates/chain/test/common/mod`
- add `bitcoin` crate to `testenv` crate
- add `tx_utils` mod to lib in testenv

[Issue: bitcoindevkit#1602]
@tvpeter
Refactor: use tx utils from testenv in chain crate
4b155bc 
- import testenv crate in chain crate
- replace all macros and fns in the following
tests 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

[issue: bitcoindevkit#1602]
@tvpeter
Refactor: Replace block_id macro def with tx util
f2bf084 
- import testenv crate
- delete common module and import `block_id` from
testenv crate

[issue: bitcoindevkit#1602]
@tvpeter
Refactor: replace bitcoin with bdk_chain::bitcoin
23ae7bb 
replace external crate `bitcoin` with re-exported
`bdk_chain::bitcoin`

[Ticket: bitcoindevkit#1602]
@tvpeter
Refactor: rename tx_utils to utils in testenv
8ebeab5 
- Rename `tx_utils.rs` to `utils.rs` in `testenv`
to allow all testing utilities to be included.
- update all imports

[Ticket: X]
@tvpeter
Refactor: move DESCRIPTORS sample array to testenv
fefd376 
move `DESCRIPTORS` sample array to `testenv` for
other crates to use

[Ticket: X]
@notmandatory notmandatory added tests and removed new feature New feature or request labels Sep 19, 2024
@oleonardolima oleonardolima mentioned this pull request Sep 19, 2024
Open
@evanlinjin
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleonardolima oleonardolima oleonardolima left review comments

@LagginTimes LagginTimes LagginTimes left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@tvpeter tvpeter

Labels
tests
Projects
BDK
Status: Needs Review
Milestone
1.0.0-beta
Development

Successfully merging this pull request may close these issues.

Move stuff from crates/chain/tests/common to bdk_testenv and make them publicly-accessible
6 participants
@tvpeter @evanlinjin @ValuedMammal @LagginTimes @oleonardolima @notmandatory