test: Add expected result assertions

yancyribbens commented at 7:26 pm on January 14, 2025: contributor

This is a trivial addition to the test suit, however it shouldn’t be required to add debug statements and manually run the tests if someone needs to know the results of this test.

DrahtBot commented at 7:26 pm on January 14, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31656.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jan 14, 2025

in src/wallet/test/coinselector_tests.cpp:1185 in dd4f894c58 outdated

1182@@ -1183,6 +1183,14 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests)
1183             return available_coins;
1184         });
1185         BOOST_CHECK(res);

brunoerg commented at 12:41 pm on January 15, 2025:

nit: Maybe this BOOST_CHECK is not needed anymore since you’re checking the result itself.

yancyribbens commented at 11:02 pm on January 15, 2025:

Good idea, thanks. I’ve removed the implicit check.

brunoerg approved

brunoerg commented at 12:44 pm on January 15, 2025: contributor

code review ACK dd4f894c58a8f220da224bd220cdad8cd810099a

This is just a simple addition to the test case “Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO” and does not increase any test coverage.

For this case, we currently only check whether coin grinder was able to return a result. Now, it checks if the result is the expected one.

yancyribbens force-pushed on Jan 15, 2025

yancyribbens commented at 11:08 pm on January 15, 2025: contributor

Updated commit message since this commit now also removes an assertion as discussed in this PR review.

in src/wallet/test/coinselector_tests.cpp:1186 in 0a4548589f outdated

1181@@ -1182,7 +1182,14 @@ BOOST_AUTO_TEST_CASE(coin_grinder_tests)
1182             }
1183             return available_coins;
1184         });
1185-        BOOST_CHECK(res);
1186+        SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG);
1187+        for (int i = 0; i < 10; i++) {

Prabhat1308 commented at 7:31 pm on January 17, 2025:

0        for (int i = 0; i < 10; ++i) {

Preference is given to ++i according to developer-notes.md

yancyribbens commented at 10:11 pm on January 20, 2025:

Thanks, done. I did a copy paste from above (line 1177) which I guess should be changed also but not here.

in src/wallet/test/coinselector_tests.cpp:1190 in 0a4548589f outdated

1186+        SelectionResult expected_result(CAmount(0), SelectionAlgorithm::CG);
1187+        for (int i = 0; i < 10; i++) {
1188+            add_coin(2 * COIN, i,expected_result);
1189+        }
1190+        for (int j = 0; j < 17; ++j) {
1191+            add_coin(0.33 * COIN, j + 10, expected_result);

Prabhat1308 commented at 7:33 pm on January 17, 2025:

nit: there seems to be a whitespace needed before expected_result in the first loop. please run a formatter over the code if not done already.

yancyribbens commented at 10:16 pm on January 20, 2025:

nit: there seems to be a whitespace needed before expected_result in the first loop.

Thanks, done.

please run a formatter over the code if not done already.

Do you mean git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v ?

I get an error after running, maybe my version fo clang-format is different.

0Formatting src/wallet/test/coinselector_tests.cpp
1/home/yancy/Git/bitcoin/src/.clang-format:46:33: error: invalid boolean
2BreakBeforeConceptDeclarations: Always
3                                ^~~~~~
4Error clang-format: Invalid argument

I’m using Debian clang-format version 14.0.6. Maybe I’ll open an issue for this.

Prabhat1308 commented at 7:35 pm on January 17, 2025: none

Code ack 0a45485

left some comments

test: Add expected result assertions

Add expected result assertions so that it's not required to add debug
statements and manually run the test to know the results.

Remove the check that a result is returned since the expected result
assertion implies that a result was returned.

678535be7d

yancyribbens force-pushed on Jan 20, 2025

yancyribbens commented at 10:17 pm on January 20, 2025: contributor

Updated PR to address format fix request.

test: Add expected result assertions #31656

Code Coverage & Benchmarks

Reviews