bitcoin
bitcoin copied to clipboard
test: clean and extend availablecoins_tests coverage
Negative PR with extended test coverage :).
-
Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
-
The class
AvailableCoinsTestingSetup
insideavailablecoins_tests.cpp
is a plain copy ofListCoinsTestingSetup
that is insidewallet_tests.cpp
.So, deleted the file and moved the
BasicOutputTypesTest
test case towallet_tests.cpp
. -
Added arg to include/skip locked coins from the
AvailableCoins
result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount. Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | theStack, aureleoules, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
-
#26756 (wallet: Replace
GetBalance()
logic withAvailableCoins()
by w0xlt) - #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
- #25659 (wallet: simplify ListCoins implementation by furszy)
- #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
rebased, conflicts solved.
Thanks for the review!
One suggestion (feel free to ignore, as there is already an ACK): I think the changes in the second commit would be much easier to review if the deduplication refactors and the additional test coverage ("coverage for 'AvailableCoins' incremental result") are not mixed up and are split into two individual commits instead.
The "incremental result" coverage just means we are now checking that previous coins are still on the available coins vector too (before, we were merely checking that the new ones appear there). It was actually a good side-effect of the cleanup and code improvements. I don't think that is something that worth to split-up (plus, it would probably mean more code into the first commit, not less).
Updated per feedback. Flipped the expected size checks. Small Diff
Just one organizational nit: seems like this latest change is unintentionally part of the last commit (which is supposed to be purely move-only) rather than in the second one?
Pushed, thanks.
ACK 9622fe64b8785430c71d4abc8637075026dc690c