test: Improve test coverage of SelectCoinsBnB #13222

1. 
   Empact commented at 10:04 pm on May 12, 2018: member

   There were a few test cases in SelectCoinsBnB testing that did not properly address the code under test:

   • Negative effective value test did not trigger the condition so was redundant with basic value selection tests. Note I switched from assert to throw in order to make this condition testable.
   • early bailout optimization tests did not test the actual selection because their utxo pool was polluted by the make_hard_case test preceding.
   • None of the tests asserted on the value_ret output value. This output is very relevant to sanity checks and more informative in the case of failures where the returned value has changed, because equal_sets does not provide information on its failure.
2. Empact force-pushed on May 12, 2018
3. Empact force-pushed on May 12, 2018
4. Empact force-pushed on May 12, 2018
5. 
   Empact commented at 10:43 pm on May 12, 2018: member

   /cc @achow101, #10637
6. Empact force-pushed on May 12, 2018
7. 
   achow101 commented at 11:23 pm on May 12, 2018: member

   utACK d938e1f4f05b7f4e010fa66e2b06c4112f373cdb
8. fanquake added the label Wallet on May 13, 2018
9. fanquake added the label Tests on May 13, 2018
10. in src/wallet/coinselection.cpp:77 in d938e1f4f0 outdated

    72@@ -71,8 +73,10 @@ bool SelectCoinsBnB(std::vector<CInputCoin>& utxo_pool, const CAmount& target_va
    73     // Calculate curr_available_value
    74     CAmount curr_available_value = 0;
    75     for (const CInputCoin& utxo : utxo_pool) {
    76-        // Assert that this utxo is not negative. It should never be negative, effective value calculation should have removed it
    77-        assert(utxo.effective_value > 0);
    78+        if (utxo.effective_value <= 0) {
    79+            // It should never be negative, effective value calculation should have removed it
    

    ---

    ---

    

    qmma70 commented at 3:41 am on June 6, 2018:

    Minor: negative or zero
11. 
    qmma70 commented at 3:42 am on June 6, 2018: contributor

    utACK
12. Empact force-pushed on Jun 6, 2018
13. 
    Empact commented at 9:27 am on June 6, 2018: member

    Updated comment
14. Empact force-pushed on Jun 8, 2018
15. 
    Empact commented at 7:32 am on June 8, 2018: member

    Improved effective value test case comments
16. Empact force-pushed on Jul 17, 2018
17. DrahtBot added the label Needs rebase on Jul 24, 2018
18. test: Add testing of value_ret for SelectCoinsBnB 1be3226f1c
19. Empact force-pushed on Jul 25, 2018
20. 
    Empact commented at 3:31 pm on July 25, 2018: member

    Rebased for #12257
21. Empact force-pushed on Jul 25, 2018
22. DrahtBot removed the label Needs rebase on Jul 25, 2018
23. test: Fix SelectCoinsBnB negative effective value tests

    This test did not previously test the condition, because it did not supply
    utxos with negative effective value.
    
    In order to make the condition testable, I switched from `assert` to
    `std::domain_error`, because throws are testable.

    469bff55ed
24. Empact force-pushed on Aug 3, 2018
25. DrahtBot added the label Needs rebase on Aug 25, 2018
26. 
    DrahtBot commented at 8:46 pm on August 25, 2018: member
27. Empact closed this on Sep 16, 2018

    ---

28. laanwj removed the label Needs rebase on Oct 24, 2019
29. MarcoFalke locked this on Dec 16, 2021

Contributors
Empact achow101 qmma70 DrahtBot

Labels
Wallet Tests