test: clean and extend availablecoins_tests coverage

[furszy]

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 wallet: simplify ListCoins implementation #25659 where I'm using the same functionality to clean/speedup another flow as well.

[DrahtBot]

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.

[furszy]

rebased, conflicts solved.

[aureleoules]

ACK d799db5 - LGTM

[aureleoules]

reACK 6ef8af2

[theStack]

Concept ACK

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.

[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]

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

[theStack]

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

Thanks, that solved the issue. Running the test with filter.skip_locked set to true fails now as expected:

wallet/test/wallet_tests.cpp(636): error: in "wallet_tests/BasicOutputTypesTest": check size == available_coins.coins[type].size() has failed [2 != 0]

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?

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

[theStack]

ACK 9622fe6

Verified that the commits deduplicate code in a nice way and increase test coverage at the same time. I don't have a strong opinion about the last commit, since I'm not sure if decreasing the number of compilation units is necessarily better (-> longer compilation time if only changes in BasicOutputTypesTest are made), and if we can be sure that AvailableCoinsTestingSetup will never differ from ListCoinsTestingSetup. Absolutely not a blocker, but would still be interesting to know what the original motivation was to put this test in an extra file with its own test setup and if it still applies (@josibake).

[aureleoules]

reACK 9622fe6, nice cleanup!

[achow101]

ACK 9622fe6