-
Notifications
You must be signed in to change notification settings - Fork 28
test: Move bump fee + foreign utxo tests #199
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.
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
base: master
Are you sure you want to change the base?
test: Move bump fee + foreign utxo tests #199
Conversation
Pull Request Test Coverage Report for Build 15828049596Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you consider separating the taproot-related test cases into a different module? There are many test cases associated with taproot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this, I was thinking about more ways to separate the tests. In addition to the build_fee_bump and add_foreign_utxo tests, we could have a tests/persisted_wallet.rs
file for the persistence related tests.
In general it makes sense for tests to be grouped according to the functional unit being tested, so for example the tests for creating transactions/PSBTs can go together, but I think further separating files by address type (legacy, segwit, etc) is less important.
For now the tests/wallet.rs
file still contains all of the tests related to addresses, creating txs, and signing. I tried to minimize the disruption and potential conflict with other PRs. Any other suggestions, let me know @tvpeter @oleonardolima.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is more ideal
In general it makes sense for tests to be grouped according to the functional unit being tested, so for example the tests for creating transactions/PSBTs can go together, but I think further separating files by address type (legacy, segwit, etc) is less important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the
tests/wallet.rs
file still contains all of the tests related to addresses, creating txs, and signing. I tried to minimize the disruption and potential conflict with other PRs. Any other suggestions, let me know @tvpeter @oleonardolima.
I agree with the changes from this PR, good work!
As you've mentioned in the call, I also think that the tests related exclusively to signing could be moved to a specific module (it can be done in a follow-up PR or kept as it is). However, they'll probably be removed once the signing feature is removed, and we're relying on rust-bitcoin's Psbt::sign
. Maybe it'll make it easier to remove? NBD though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 22e28b2
Thank you for working on this Mammal.
I did not proceed with #238 since they are related.
There are some warnings related to bitcoin::key::TweakedKeypair::to_inner
but once you rebase, they should be gone.
I have left a comment on whether it is a good idea to separate taproot-related test cases into their module.
Thank you
22e28b2
to
ea39339
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK ea39339
These files are added to wallet/tests directory
add_foreign_utxo.rs
persisted_wallet.rs
build_fee_bump
.rscommon.rs
fixes #238
Checklists
All Submissions: