Skip to content

Commit 5a85298

Browse files
rustaceanrobrustaceanrob
committed
refactor: test: Unroll && conditions in macros
Using `&&` in `BOOST_CHECK` is problematic as failures will not indicate which condition failed. By unrolling these checks, the user knows exactly which expression is the failing case. As an example, here is a line that would be particularly hard to debug if it failed: ``` src/test/net_tests.cpp BOOST_CHECK((*ret)[1] && (*ret)[1]->m_type == "headers" && std::ranges::equal((*ret)[1]->m_recv, MakeByteSpan(msg_data_2))); ``` If any one of these conditions fail, the whole expression fails, with no values printed or indication as to which condition failed. This is also required when using test macros that support value decomposition, which requires `&&` and `||` are `delete`. Examples include `BOOST_TEST`, doctest, Catch2, etc. ref: https://catch2-temp.readthedocs.io/en/latest/assertions.html#other-limitations ref: https://fekir.info/post/decomposing-an-expression/
1 parent 70d9ec7 commit 5a85298

19 files changed

Lines changed: 196 additions & 94 deletions

‎src/test/amount_tests.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,15 @@ BOOST_AUTO_TEST_CASE(GetFeeTest)
8181
// Previously, precision was limited to three decimal digits
8282
// due to only supporting satoshis per kB, so CFeeRate(CAmount(1), 1001) was equal to CFeeRate(0)
8383
// Since #32750, higher precision is maintained.
84-
BOOST_CHECK(CFeeRate(CAmount(1), 1001) > CFeeRate(0) && CFeeRate(CAmount(1), 1001) < CFeeRate(1));
85-
BOOST_CHECK(CFeeRate(CAmount(2), 1001) > CFeeRate(1) && CFeeRate(CAmount(2), 1001) < CFeeRate(2));
84+
BOOST_CHECK(CFeeRate(CAmount(1), 1001) > CFeeRate(0));
85+
BOOST_CHECK(CFeeRate(CAmount(1), 1001) < CFeeRate(1));
86+
BOOST_CHECK(CFeeRate(CAmount(2), 1001) > CFeeRate(1));
87+
BOOST_CHECK(CFeeRate(CAmount(2), 1001) < CFeeRate(2));
8688
// some more integer checks
87-
BOOST_CHECK(CFeeRate(CAmount(26), 789) > CFeeRate(32) && CFeeRate(CAmount(26), 789) < CFeeRate(33));
88-
BOOST_CHECK(CFeeRate(CAmount(27), 789) > CFeeRate(34) && CFeeRate(CAmount(27), 789) < CFeeRate(35));
89+
BOOST_CHECK(CFeeRate(CAmount(26), 789) > CFeeRate(32));
90+
BOOST_CHECK(CFeeRate(CAmount(26), 789) < CFeeRate(33));
91+
BOOST_CHECK(CFeeRate(CAmount(27), 789) > CFeeRate(34));
92+
BOOST_CHECK(CFeeRate(CAmount(27), 789) < CFeeRate(35));
8993
// Maximum size in bytes, should not crash
9094
CFeeRate(MAX_MONEY, std::numeric_limits<int32_t>::max()).GetFeePerK();
9195

‎src/test/argsman_tests.cpp‎

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -204,24 +204,33 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
204204
testArgs.SetupArgs({a, b, ccc, d});
205205
BOOST_CHECK(testArgs.ParseParameters(0, argv_test, error));
206206
testArgs.LockSettings([&](const common::Settings& s) {
207-
BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty());
207+
BOOST_CHECK(s.command_line_options.empty());
208+
BOOST_CHECK(s.ro_config.empty());
208209
});
209210

210211
BOOST_CHECK(testArgs.ParseParameters(1, argv_test, error));
211212
testArgs.LockSettings([&](const common::Settings& s) {
212-
BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty());
213+
BOOST_CHECK(s.command_line_options.empty());
214+
BOOST_CHECK(s.ro_config.empty());
213215
});
214216

215217
BOOST_CHECK(testArgs.ParseParameters(7, argv_test, error));
216218
// expectation: -ignored is ignored (program name argument),
217219
// -a, -b and -ccc end up in map, -d ignored because it is after
218220
// a non-option argument (non-GNU option parsing)
219-
BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc")
220-
&& !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d"));
221+
BOOST_CHECK(testArgs.IsArgSet("-a"));
222+
BOOST_CHECK(testArgs.IsArgSet("-b"));
223+
BOOST_CHECK(testArgs.IsArgSet("-ccc"));
224+
BOOST_CHECK(!testArgs.IsArgSet("f"));
225+
BOOST_CHECK(!testArgs.IsArgSet("-d"));
221226
testArgs.LockSettings([&](const common::Settings& s) {
222-
BOOST_CHECK(s.command_line_options.size() == 3 && s.ro_config.empty());
223-
BOOST_CHECK(s.command_line_options.contains("a") && s.command_line_options.contains("b") && s.command_line_options.contains("ccc")
224-
&& !s.command_line_options.contains("f") && !s.command_line_options.contains("d"));
227+
BOOST_CHECK(s.command_line_options.size() == 3);
228+
BOOST_CHECK(s.ro_config.empty());
229+
BOOST_CHECK(s.command_line_options.contains("a"));
230+
BOOST_CHECK(s.command_line_options.contains("b"));
231+
BOOST_CHECK(s.command_line_options.contains("ccc"));
232+
BOOST_CHECK(!s.command_line_options.contains("f"));
233+
BOOST_CHECK(!s.command_line_options.contains("d"));
225234

226235
BOOST_CHECK(s.command_line_options.at("a").size() == 1);
227236
BOOST_CHECK(s.command_line_options.at("a").front().get_str() == "");
@@ -329,7 +338,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
329338

330339
// Nothing else should be in the map
331340
testArgs.LockSettings([&](const common::Settings& s) {
332-
BOOST_CHECK(s.command_line_options.size() == 6 && s.ro_config.empty());
341+
BOOST_CHECK(s.command_line_options.size() == 6);
342+
BOOST_CHECK(s.ro_config.empty());
333343
});
334344

335345
// The -no prefix should get stripped on the way in.
@@ -397,8 +407,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArgEdgeCases)
397407
// Command line overrides, but doesn't erase old setting
398408
BOOST_CHECK(!testArgs.IsArgNegated("-bar"));
399409
BOOST_CHECK(testArgs.GetArg("-bar", "xxx") == "");
400-
BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1
401-
&& testArgs.GetArgs("-bar").front() == "");
410+
BOOST_CHECK(testArgs.GetArgs("-bar").size() == 1);
411+
BOOST_CHECK(testArgs.GetArgs("-bar").front() == "");
402412
}
403413

404414
BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
@@ -498,22 +508,22 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
498508
BOOST_CHECK(test_args.GetBoolArg("-iii", def) == def);
499509
}
500510

501-
BOOST_CHECK(test_args.GetArgs("-a").size() == 1
502-
&& test_args.GetArgs("-a").front() == "");
503-
BOOST_CHECK(test_args.GetArgs("-b").size() == 1
504-
&& test_args.GetArgs("-b").front() == "1");
505-
BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2
506-
&& test_args.GetArgs("-ccc").front() == "argument"
507-
&& test_args.GetArgs("-ccc").back() == "multiple");
511+
BOOST_CHECK(test_args.GetArgs("-a").size() == 1);
512+
BOOST_CHECK(test_args.GetArgs("-a").front() == "");
513+
BOOST_CHECK(test_args.GetArgs("-b").size() == 1);
514+
BOOST_CHECK(test_args.GetArgs("-b").front() == "1");
515+
BOOST_CHECK(test_args.GetArgs("-ccc").size() == 2);
516+
BOOST_CHECK(test_args.GetArgs("-ccc").front() == "argument");
517+
BOOST_CHECK(test_args.GetArgs("-ccc").back() == "multiple");
508518
BOOST_CHECK(test_args.GetArgs("-fff").size() == 0);
509519
BOOST_CHECK(test_args.GetArgs("-nofff").size() == 0);
510-
BOOST_CHECK(test_args.GetArgs("-ggg").size() == 1
511-
&& test_args.GetArgs("-ggg").front() == "1");
520+
BOOST_CHECK(test_args.GetArgs("-ggg").size() == 1);
521+
BOOST_CHECK(test_args.GetArgs("-ggg").front() == "1");
512522
BOOST_CHECK(test_args.GetArgs("-noggg").size() == 0);
513523
BOOST_CHECK(test_args.GetArgs("-h").size() == 0);
514524
BOOST_CHECK(test_args.GetArgs("-noh").size() == 0);
515-
BOOST_CHECK(test_args.GetArgs("-i").size() == 1
516-
&& test_args.GetArgs("-i").front() == "1");
525+
BOOST_CHECK(test_args.GetArgs("-i").size() == 1);
526+
BOOST_CHECK(test_args.GetArgs("-i").front() == "1");
517527
BOOST_CHECK(test_args.GetArgs("-noi").size() == 0);
518528
BOOST_CHECK(test_args.GetArgs("-zzz").size() == 0);
519529

‎src/test/arith_uint256_tests.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,14 @@ BOOST_AUTO_TEST_CASE( comparison ) // <= >= < >
267267
arith_uint256 TmpL;
268268
for (unsigned int i = 0; i < 256; ++i) {
269269
TmpL= OneL<< i;
270-
BOOST_CHECK( TmpL >= ZeroL && TmpL > ZeroL && ZeroL < TmpL && ZeroL <= TmpL);
271-
BOOST_CHECK( TmpL >= 0 && TmpL > 0 && 0 < TmpL && 0 <= TmpL);
270+
BOOST_CHECK(TmpL >= ZeroL);
271+
BOOST_CHECK(TmpL > ZeroL);
272+
BOOST_CHECK(ZeroL < TmpL);
273+
BOOST_CHECK(ZeroL <= TmpL);
274+
BOOST_CHECK(TmpL >= 0);
275+
BOOST_CHECK(TmpL > 0);
276+
BOOST_CHECK(0 < TmpL);
277+
BOOST_CHECK(0 <= TmpL);
272278
TmpL |= R1L;
273279
BOOST_CHECK( TmpL >= R1L ); BOOST_CHECK( (TmpL == R1L) != (TmpL > R1L)); BOOST_CHECK( (TmpL == R1L) || !( TmpL <= R1L));
274280
BOOST_CHECK( R1L <= TmpL ); BOOST_CHECK( (R1L == TmpL) != (R1L < TmpL)); BOOST_CHECK( (TmpL == R1L) || !( R1L >= TmpL));

‎src/test/base58_tests.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@ BOOST_AUTO_TEST_CASE(base58_DecodeBase58)
5858
std::vector<unsigned char> expected = ParseHex(test[0].get_str());
5959
std::string base58string = test[1].get_str();
6060
BOOST_CHECK_MESSAGE(DecodeBase58(base58string, result, 256), strTest);
61-
BOOST_CHECK_MESSAGE(result.size() == expected.size() && std::equal(result.begin(), result.end(), expected.begin()), strTest);
61+
BOOST_CHECK_MESSAGE(result.size() == expected.size(), strTest);
62+
BOOST_CHECK_MESSAGE(std::equal(result.begin(), result.end(), expected.begin()), strTest);
6263
}
6364

6465
BOOST_CHECK(!DecodeBase58("invalid"s, result, 100));

‎src/test/bip32_tests.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,8 @@ BOOST_AUTO_TEST_CASE(bip32_max_depth) {
197197
}
198198

199199
// But trying to derive a non-existent 256th depth will fail!
200-
BOOST_CHECK(key_parent.nDepth == 255 && pubkey_parent.nDepth == 255);
200+
BOOST_CHECK(key_parent.nDepth == 255);
201+
BOOST_CHECK(pubkey_parent.nDepth == 255);
201202
BOOST_CHECK(!key_parent.Derive(key_child, 0));
202203
BOOST_CHECK(!pubkey_parent.Derive(pubkey_child, 0));
203204
}

‎src/test/checkqueue_tests.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,8 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure)
233233
}
234234
auto result = control.Complete();
235235
if (i > 0) {
236-
BOOST_REQUIRE(result.has_value() && *result == static_cast<int>(17 * i));
236+
BOOST_REQUIRE(result.has_value());
237+
BOOST_REQUIRE(*result == static_cast<int>(17 * i));
237238
} else {
238239
BOOST_REQUIRE(!result.has_value());
239240
}

‎src/test/coinscachepair_tests.cpp‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ std::list<CoinsCachePair> CreatePairs(CoinsCachePair& sentinel)
2121
auto node{std::prev(nodes.end())};
2222
CCoinsCacheEntry::SetDirty(*node, sentinel);
2323

24-
BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh());
24+
BOOST_CHECK(node->second.IsDirty());
25+
BOOST_CHECK(!node->second.IsFresh());
2526
BOOST_CHECK_EQUAL(node->second.Next(), &sentinel);
2627
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node));
2728

@@ -63,7 +64,8 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration)
6364

6465
// Delete the nodes from the list to make sure there are no dangling pointers
6566
for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) {
66-
BOOST_CHECK(!it->second.IsDirty() && !it->second.IsFresh());
67+
BOOST_CHECK(!it->second.IsDirty());
68+
BOOST_CHECK(!it->second.IsFresh());
6769
}
6870
}
6971

@@ -105,9 +107,11 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion)
105107
nodes.erase(n2);
106108
// Check that n1 now points to n3, and n3 still points to n4
107109
// Also check that state was not altered
108-
BOOST_CHECK(n1->second.IsDirty() && !n1->second.IsFresh());
110+
BOOST_CHECK(n1->second.IsDirty());
111+
BOOST_CHECK(!n1->second.IsFresh());
109112
BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3));
110-
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
113+
BOOST_CHECK(n3->second.IsDirty());
114+
BOOST_CHECK(!n3->second.IsFresh());
111115
BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
112116
BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1));
113117

@@ -116,7 +120,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion)
116120
nodes.erase(n1);
117121
// Check that sentinel now points to n3, and n3 still points to n4
118122
// Also check that state was not altered
119-
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
123+
BOOST_CHECK(n3->second.IsDirty());
124+
BOOST_CHECK(!n3->second.IsFresh());
120125
BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
121126
BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
122127
BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel);
@@ -126,7 +131,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion)
126131
nodes.erase(n4);
127132
// Check that sentinel still points to n3, and n3 points to sentinel
128133
// Also check that state was not altered
129-
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
134+
BOOST_CHECK(n3->second.IsDirty());
135+
BOOST_CHECK(!n3->second.IsFresh());
130136
BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
131137
BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel);
132138
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3));
@@ -148,47 +154,53 @@ BOOST_AUTO_TEST_CASE(linked_list_set_state)
148154

149155
// Check that setting DIRTY inserts it into linked list and sets state
150156
CCoinsCacheEntry::SetDirty(n1, sentinel);
151-
BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh());
157+
BOOST_CHECK(n1.second.IsDirty());
158+
BOOST_CHECK(!n1.second.IsFresh());
152159
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
153160
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
154161
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
155162
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
156163

157164
// Check that setting FRESH on new node inserts it after n1
158165
CCoinsCacheEntry::SetFresh(n2, sentinel);
159-
BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
166+
BOOST_CHECK(n2.second.IsFresh());
167+
BOOST_CHECK(!n2.second.IsDirty());
160168
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
161169
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
162170
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
163171
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
164172

165173
// Check that we can set extra state, but they don't change our position
166174
CCoinsCacheEntry::SetFresh(n1, sentinel);
167-
BOOST_CHECK(n1.second.IsDirty() && n1.second.IsFresh());
175+
BOOST_CHECK(n1.second.IsDirty());
176+
BOOST_CHECK(n1.second.IsFresh());
168177
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
169178
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
170179
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
171180
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
172181

173182
// Check that we can clear state then re-set it
174183
n1.second.SetClean();
175-
BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh());
184+
BOOST_CHECK(!n1.second.IsDirty());
185+
BOOST_CHECK(!n1.second.IsFresh());
176186
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
177187
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
178188
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
179189
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
180190

181191
// Calling `SetClean` a second time has no effect
182192
n1.second.SetClean();
183-
BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh());
193+
BOOST_CHECK(!n1.second.IsDirty());
194+
BOOST_CHECK(!n1.second.IsFresh());
184195
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
185196
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
186197
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
187198
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
188199

189200
// Adding DIRTY re-inserts it after n2
190201
CCoinsCacheEntry::SetDirty(n1, sentinel);
191-
BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh());
202+
BOOST_CHECK(n1.second.IsDirty());
203+
BOOST_CHECK(!n1.second.IsFresh());
192204
BOOST_CHECK_EQUAL(n2.second.Next(), &n1);
193205
BOOST_CHECK_EQUAL(n1.second.Prev(), &n2);
194206
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);

‎src/test/key_tests.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,17 @@ BOOST_FIXTURE_TEST_SUITE(key_tests, BasicTestingSetup)
4141
BOOST_AUTO_TEST_CASE(key_test1)
4242
{
4343
CKey key1 = DecodeSecret(strSecret1);
44-
BOOST_CHECK(key1.IsValid() && !key1.IsCompressed());
44+
BOOST_CHECK(key1.IsValid());
45+
BOOST_CHECK(!key1.IsCompressed());
4546
CKey key2 = DecodeSecret(strSecret2);
46-
BOOST_CHECK(key2.IsValid() && !key2.IsCompressed());
47+
BOOST_CHECK(key2.IsValid());
48+
BOOST_CHECK(!key2.IsCompressed());
4749
CKey key1C = DecodeSecret(strSecret1C);
48-
BOOST_CHECK(key1C.IsValid() && key1C.IsCompressed());
50+
BOOST_CHECK(key1C.IsValid());
51+
BOOST_CHECK(key1C.IsCompressed());
4952
CKey key2C = DecodeSecret(strSecret2C);
50-
BOOST_CHECK(key2C.IsValid() && key2C.IsCompressed());
53+
BOOST_CHECK(key2C.IsValid());
54+
BOOST_CHECK(key2C.IsCompressed());
5155
CKey bad_key = DecodeSecret(strAddressBad);
5256
BOOST_CHECK(!bad_key.IsValid());
5357

0 commit comments

Comments
 (0)