test: 3 new tests for SelectCoins function #24820

pull akankshakashyap wants to merge 1 commits into bitcoin:master from akankshakashyap:coin_selection_tc changing 1 files +42 −0

akankshakashyap commented at 6:05 PM on April 10, 2022: contributor
Three new tests have been added.
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
DrahtBot added the label Wallet on Apr 10, 2022
akankshakashyap force-pushed on Apr 12, 2022
akankshakashyap commented at 7:33 AM on April 14, 2022: contributor

@brunoerg @MarcoFalke @achow101 could you please review it, as it would be helpful for my summer of bitcoin proposal.
in src/wallet/test/coinselector_tests.cpp:381 in 5c15850ded outdated
```
 376 | +        std::vector<COutput> coins;
 377 | +
 378 | +        expected_result.Clear();
 379 | +        add_coin(coins, *wallet, 10 * CENT, 6 * 24, false, 0, true);
 380 | +        add_coin(coins, *wallet, 9 * CENT, 6 * 24, false, 0, true);
 381 | +        add_coin(coins, *wallet, 1 * CENT, 6 * 24, false, 0, true);
```
achow101 commented at 8:50 PM on April 14, 2022:

In 5c15850ded2a3195fae0dd238e1aebaaa46b13ce "3 new tests for SelectCoins function"

All of this setup is the same in all three test cases. These can be deduplicated and just have a single scope for all these cases.

akankshakashyap commented at 9:12 AM on April 16, 2022:

made these changes.
MarcoFalke commented at 9:19 AM on April 16, 2022: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
akankshakashyap force-pushed on Apr 16, 2022
fanquake commented at 10:13 AM on April 20, 2022: member
Given this hasn't been re-reviewed yet, I'd suggest improving your commit message. It should follow a similar form to the PR title / description, a title, followed by a newline, and then a description of the change. Irrelevant things like "removed duplicate setup from tests", should be dropped. i.e:
```
test: add 3 new test cases for SelectCoins()

1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
```

add 3 new test cases for SelectCoins()

1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.

3f8def51d5

akankshakashyap force-pushed on Apr 22, 2022
achow101 commented at 9:18 PM on May 16, 2022: member

ACK 3f8def51d53a078a5ee71ec675b5e06b784147de
brunoerg approved
brunoerg commented at 1:21 PM on May 19, 2022: member

ACK 3f8def51d53a078a5ee71ec675b5e06b784147de

Nice adds!
MarcoFalke assigned achow101 on May 20, 2022
achow101 merged this on May 20, 2022
achow101 closed this on May 20, 2022
sidhujag referenced this in commit b6ebdece85 on May 28, 2022
sidhujag referenced this in commit dc45435922 on May 28, 2022
DrahtBot locked this on May 20, 2023

Contributors

Labels