test: clean and extend availablecoins_tests coverage #25789
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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
force-pushed
the
2022_test_clean_redundant_availablecoins_test
branch
from
9dd10b1 to
014b9dd
Compare
|
rebased, conflicts solved. |
furszy
force-pushed
the
2022_test_clean_redundant_availablecoins_test
branch
2 times, most recently
from
27b4f55 to
d799db5
Compare
aureleoules
left a comment
aureleoules
left a comment
There was a problem hiding this comment.
ACK d799db5b87462243c685e04a91c128e1b24128ab - LGTM
furszy
force-pushed
the
2022_test_clean_redundant_availablecoins_test
branch
from
d799db5 to
6ef8af2
Compare
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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.
|
Thanks for the review!
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. |
furszy
force-pushed
the
2022_test_clean_redundant_availablecoins_test
branch
from
6ef8af2 to
de541b1
Compare
|
Updated per feedback. |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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?
Clean redundant code and add coverage for 'AvailableCoins' incremental result.
The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy of `ListCoinsTestingSetup` that is inside wallet_tests.cpp.
furszy
force-pushed
the
2022_test_clean_redundant_availablecoins_test
branch
from
de541b1 to
9622fe6
Compare
Pushed, thanks. |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
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
left a comment
aureleoules
left a comment
There was a problem hiding this comment.
reACK 9622fe6, nice cleanup!
|
ACK 9622fe6 |
6 participants
Negative PR with extended test coverage :).
Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
The class
AvailableCoinsTestingSetupinsideavailablecoins_tests.cppis a plain copyof
ListCoinsTestingSetupthat is insidewallet_tests.cpp.So, deleted the file and moved the
BasicOutputTypesTesttest case towallet_tests.cpp.Added arg to include/skip locked coins from the
AvailableCoinsresult. 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.