Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Apr 8, 2025

These files are added to wallet/tests directory

  • add_foreign_utxo.rs
  • persisted_wallet.rs
  • build_fee_bump.rs
  • common.rs

fixes #238

Checklists

All Submissions:

Sorry, something went wrong.

@ValuedMammal ValuedMammal self-assigned this Apr 8, 2025
@ValuedMammal ValuedMammal added this to the 1.3.0 milestone Apr 8, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Apr 8, 2025
@coveralls
Copy link

coveralls commented Apr 8, 2025

Pull Request Test Coverage Report for Build 15828049596

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.55%

Totals Coverage Status
Change from base Build 15719064446: 0.0%
Covered Lines: 7377
Relevant Lines: 8623

💛 - Coveralls

@ValuedMammal ValuedMammal moved this from Needs Review to In Progress in BDK Wallet May 12, 2025
@ValuedMammal ValuedMammal removed this from the Wallet 2.0.0 milestone May 25, 2025
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@tvpeter tvpeter left a 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

@ValuedMammal ValuedMammal added the tests New or improved tests label Jun 19, 2025
@ValuedMammal ValuedMammal added this to the Wallet 2.1.0 milestone Jun 19, 2025

Verified

This commit was signed with the committer’s verified signature.
Calinou Hugo Locurcio
@ValuedMammal ValuedMammal force-pushed the refactor/wallet-tests branch from 22e28b2 to ea39339 Compare June 23, 2025 15:09
@ValuedMammal ValuedMammal requested a review from tvpeter June 23, 2025 15:19
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Jun 23, 2025
Copy link
Contributor

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

tACK ea39339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests New or improved tests
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Split tests into multiple files
4 participants