bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

test: clean and extend availablecoins_tests coverage

Open furszy opened this issue 2 years ago • 2 comments

Negative PR with extended test coverage :).

  1. Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.

  2. The class AvailableCoinsTestingSetup inside availablecoins_tests.cpp is a plain copy of ListCoinsTestingSetup that is inside wallet_tests.cpp.

    So, deleted the file and moved the BasicOutputTypesTest test case to wallet_tests.cpp.

  3. 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.

furszy avatar Aug 05 '22 19:08 furszy

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 with AvailableCoins() 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.

DrahtBot avatar Aug 06 '22 04:08 DrahtBot

rebased, conflicts solved.

furszy avatar Aug 18 '22 13:08 furszy

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).

furszy avatar Dec 07 '22 23:12 furszy

Updated per feedback. Flipped the expected size checks. Small Diff

furszy avatar Dec 13 '22 23:12 furszy

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.

furszy avatar Dec 14 '22 14:12 furszy

ACK 9622fe64b8785430c71d4abc8637075026dc690c

achow101 avatar Jan 03 '23 17:01 achow101