qa: assert_equals -> assert_true/assert

hodlinator commented at 7:45 AM on September 15, 2026: contributor

Instead of this in the functional tests:

    assert_equals([long expression pushing next arg to the right], True)
    assert_equals([other long expression pushing next arg to the right], False)

we get:

    assert_true([long expression without arg on the right side..])
    assert_false([other long expression without next arg to the right.])

The main point of this change is that assert_true()/assert_false() reduce cognitive tokens when scanning the code (equals to what...?). (As a consequence, assert_true()/assert_false() are also common in other test frameworks).

assert_equals() continues to be used in cases where we don't compare to boolean literals.

assert_true([expression]) is different from plain assert [expression] in that assert is truthy while assert_true requires boolean True. (assert can also be deactivated when executing Python code in optimized mode (see #30529 (review))).

A smaller scale attempt at introducing assert_true() was originally included in #31874 before being dropped.

DrahtBot added the label Tests on Sep 15, 2026

DrahtBot commented at 7:45 AM on September 15, 2026: 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/36257.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36192 (test: cover unsatisfiable mining timestamp by Sjors)
#36170 (net: require a dedicated bind for automatic Tor by l0rinc)
#36097 (mining: replace interrupt methods with cancellation arguments by xyzconstant)
#35940 (net: allow selecting BIP152 high-bandwidth peers with -addnode by w0xlt)
#35920 (net_processing: Ignore MSG_WITNESS_TX entries from INV messages by ajtowns)
#35837 (rpc: fail scanblocks when block filter range is unavailable by MicSm)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#35301 (Silent Payments: Implement bip352 (take 2) by Eunovo)
#34371 (wallet: allow importprunedfunds for spending transactions by 8144225309)
#34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33112 (wallet: relax external_signer flag constraints by Sjors)
#32857 (wallet: allow skipping script paths by Sjors)
#31668 (Added rescan option for import descriptors by saikiran57)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

self.nodes[0].createwallet('w8', False, False, '', True) in test/functional/wallet_createwallet.py

Possible places where comparison-specific test macros should replace generic comparisons:

[test/functional/wallet_reorgsrestore.py] assert(wallet0.getbalances()['mine']['trusted'] == 0) -> use assert_equal(wallet0.getbalances()['mine']['trusted'], 0) for clearer failure output.

in test/functional/test_framework/util.py:86 in 4065d7c607 outdated

  82 | @@ -83,6 +83,15 @@ def summarise_dict_differences(thing1, thing2):
  83 |              d2[k] = thing2[k]
  84 |      return d1, d2
  85 |  
  86 | +def assert_true(expression: bool):

l0rinc commented at 6:47 PM on September 15, 2026:

4065d7c qa: Add assert_true() / assert_false():

The first commit introduces dead code; there are no users of the new methods, so I have to go through the next commits to tell if these are fully correct or not.

But even just looking at these, I don't like that they do something other than what they claim: they're not actually asserting True, but rather truthiness, which doesn't seem to be documented here. Maybe renaming to truthy and falsy would be more predictable.

I would also like to see tests added to explain their behavior, since otherwise the actual test cases are the test for the new helper method, and both could be incorrect and we wouldn't know. Just a few simple ones with primitives would suffice.

hodlinator commented at 8:09 PM on September 15, 2026:

re #36257 (review):

Thanks for catching the accidental degradation in correctness! I originally used != True/!= False but the ruff linter recommended the truthy/falsy variants. is not seems to fit the bill without angering ruff.

You can patch the test_runner with this and run it to verify the latest push is more correct:

--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -30,6 +30,8 @@ import tempfile
 import re
 import logging
 from test_framework.util import (
+    assert_false,
+    assert_true,
     Binaries,
     export_env_build_path,
     get_binary_paths,
@@ -967,4 +969,52 @@ class RPCCoverage():
 
 
 if __name__ == '__main__':
-    main()
+    assert_false(False)
+    assert_true(True)
+
+    try:
+        assert_false(True)
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_false(True) should raise")
+
+    try:
+        assert_true(False)
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_true(False) should raise")
+
+
+    # Test falsyness
+    try:
+        assert_false(list())
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_false is falsy")
+
+    try:
+        assert_false([1, 2])
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_false is broken")
+
+    # Test truthyness
+    try:
+        assert_true([1, 2])
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_true is truthy")
+
+    try:
+        assert_true(list())
+    except AssertionError:
+        pass
+    else:
+        raise AssertionError("assert_true is broken")
+
+    print("assert_false/assert_true have expected behavior!")

</details>

None of the other util methods have these meta-tests AFAIK. If you insist do you have any suggestion for where to start adding them?

Regarding adding them as dead code, it's a natural effect of having to add them before the scripted diff. (Only with the exception of one wallet test in "qa: Fix weird case of 3-arg assert_equal() ahead of scripted diff").

l0rinc commented at 6:54 PM on September 15, 2026: contributor

I don't mind the change, the only reason I see to use explicit True and False is to flip these after a characterization test - but they could also flip the method name.

Based on https://mirror.b10c.me/bitcoin-bitcoin/34773#discussion_r3009767675 and https://mirror.b10c.me/bitcoin-bitcoin/34773#discussion_r3009771746, @maflcko might be interested.

qa: Add assert_true() / assert_false() 486d0027e5

qa: Fix weird case of 3-arg assert_equal() ahead of scripted diff

Before 2ef6679c2ca87bbe305f45ff3df3d19ec3f60595 introduced finalized2 we had:
assert_equal(finalized["complete"], True)
...so this is changing things back towards that.

acef9ecdae

qa: Fixup imports ahead of scripted diff f27214f16d

scripted-diff: Replace assert_equals([..., ]True/False[, ...]) with assert_true/assert_false

-BEGIN VERIFY SCRIPT-
sed -i -E "s/assert_equal\((.*)\, (True|False)\)( *(#.*))?$/assert_\L\2(\E\1)\3/g" $( git grep -lE "assert_equal\(.*, (True|False)\)"  -- test/functional/ )
sed -i -E "s/assert_equal\((True|False), (.*)$/assert_\L\1\E\(\2/g" $( git grep -lE "assert_equal\((True|False)"  -- test/functional/ )
-END VERIFY SCRIPT-

bd1776c028

qa: Remove no longer used assert_equals imports 01ff43d926

hodlinator force-pushed on Sep 15, 2026

hodlinator commented at 8:20 PM on September 15, 2026: contributor

I don't mind the change, the only reason I see to use explicit True and False is to flip these after a characterization test - but they could also flip the method name.

It was reviewing your characterization test flipping which made me flip and implement this. :)

maflcko commented at 9:19 AM on September 16, 2026: member

assert_true([expression]) is different from plain assert [expression] in that the latter can be deactivated when executing Python code in optimized mode (see #30529 (comment)).

This isn't wrong, but seems to be missing the point: The reason why assert_equal with True or False is used is not because assert may dropped (in theory, no one is doing this in practise?), but rather that the assert only checks truthy/falsy states, not type-safe equality.

So the only real remaining motivation for this is to reduce code bloat. No objection, but overall I am ~0 on this.

hodlinator commented at 10:06 AM on September 16, 2026: contributor

Thanks for having a look @maflcko. Re-orged the PR desc to add the truthyness nuance of assert and also to make the cognitive part more prominent as the motivation.

qa: assert_equals -> assert_true/assert_false #36257

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)