In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable
This is motivated/uses logic from this PR which was closed #28528 This partially helps #23119
I’ve broken it up to just assert_not_equal to keep the PR smaller as suggested in #28528 (comment)
I can create follow up PR’s if this is wanted
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29500.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hodlinator, ryanofsky, janb84 |
| Concept ACK | theStack, rkrux |
| Approach ACK | BrandonOdiwuor |
| Stale ACK | vasild |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Would be nice to structure this as a scripted diff: one commit where you add the function and requires, another where you convert the assert !=s.
That would ensure programmatically that you hit all of the cases.
added scripted diff in dc087f1 with the following script
git grep -l "assert.*!=" ./test/functional
I will have to amend the commit as I see more spots I need to modify with the assert_not_equal util
There are some places you are using assert_not_equal but not importing it and other ones you’re importing it twice. Check CI.
0Running Unit Tests for Test Framework Modules
1Traceback (most recent call last):
2 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 906, in <module>
3 main()
4 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 539, in main
5 run_tests(
6 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 580, in run_tests
7 test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module)))
8 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
9 File "/usr/lib/python3.12/unittest/loader.py", line 137, in loadTestsFromName
10 module = __import__(module_name)
11 ^^^^^^^^^^^^^^^^^^^^^^^
12 File "/ci_container_base/test/functional/test_framework/address.py", line 14, in <module>
13 from .script import (
14 File "/ci_container_base/test/functional/test_framework/script.py", line 14, in <module>
15 from .key import TaggedHash, tweak_add_pubkey, compute_xonly_pubkey
16 File "/ci_container_base/test/functional/test_framework/key.py", line 16, in <module>
17 from test_framework.crypto import secp256k1
18 File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 316, in <module>
19 G = GE.lift_x(0x79BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798)
20 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
21 File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 257, in lift_x
22 y = (FE(x)**3 + 7).sqrt()
23 ^^^^^
24 File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 41, in __init__
25 assert_not_equal(den, 0)
26 ^^^^^^^^^^^^^^^^
27NameError: name 'assert_not_equal' is not defined
21@@ -21,6 +22,7 @@
22 from test_framework.p2p import P2PInterface
23 from test_framework.test_framework import BitcoinTestFramework
24 from test_framework.util import (
25+ assert_not_equal,
util and messages)
hmm yea I was trying to use this command to find all the places but it seems to be a bit more complex than I initially thought
0git grep -l "assert .*!=" -- ':(exclude)*script.py' ./test/functional | xargs sed -i -e "/assert .*!=/ s/assert /assert_not_equal(/g" -e "/assert_not_equal/ s/ !=/,/g" -e "/assert_not_equal/ s/$/)/g" -e "s/test_framework.util import (/test_framework.util import (\n assert_not_equal,/g" -e "s/import assert_equal/(\n assert_equal,\n assert_not_equal,\n)/g"
I might just manually add the imports and use the script to look for the assert ... != as suggested by @Empact
https://cirrus-ci.com/task/6627384883937280?logs=ci#L4021
0 test 2024-03-05T05:30:24.555000Z TestFramework (ERROR): Unexpected exception caught during testing
1 Traceback (most recent call last):
2 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
3 self.run_test()
4 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 448, in run_test
5 self.test_reorg_2child_rbf()
6 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 29, in wrapper
7 func(self)
8 File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 429, in test_reorg_2child_rbf
9 assert_not_equal((child_1_conflict["txid"], child_1["txid"]))
10 TypeError: assert_not_equal() missing 1 required positional argument: 'thing2'
ASan + LSan + UBSan + integer, no depends, USDT but the rest seem to be passing
not sure why this ci is failing
ASan + LSan + UBSan + integer, no depends, USDTbut the rest seem to be passing
It is a race in the test logic between the python code and the net thread, IIUC. Please file a new issue or a new pull to fix it.
not sure why this ci is failing
ASan + LSan + UBSan + integer, no depends, USDTbut the rest seem to be passingIt is a race in the test logic between the python code and the
netthread, IIUC. Please file a new issue or a new pull to fix it.
Opened a new issue for the CI error https://github.com/bitcoin/bitcoin/issues/29643
Approach ACK
Good job approaching this using Scripted diffs
637@@ -638,7 +638,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat
638 self.start(extra_args, stdout=log_stdout, stderr=log_stderr, *args, **kwargs)
639 ret = self.process.wait(timeout=self.rpc_timeout)
640 self.log.debug(self._node_msg(f'bitcoind exited with status {ret} during initialization'))
641- assert ret != 0 # Exit code must indicate failure
642+ assert_not_equal(ret, 0 ) # Exit code must indicate failure
fixed in 4358c6e
thanks for the review!
93@@ -94,7 +94,7 @@ def run_test(self):
94 self.nodes[0].syncwithvalidationinterfacequeue()
95
96 main_block_hash = self.nodes[0].getblockhash(1000)
97- assert main_block_hash != stale_block_hash, "node 0 chain did not reorganize"
98+ assert_not_equal(main_block_hash, stale_block_hash, "node 0 chain did not reorganize")
"node 0 chain did not reorganize"
This seems to be the 3rd argument to the assert_not_equal() call, i.e. *args. Will this piece be kicked in then? any(thing1 == arg for arg in args)
I don’t get why would we need to search for main_block_hash in the above error string.
I’ve updated the function to not accept the args param since it did not seem to be needed and instead allowed an optional 3rd param as the message
updated in 4488120
crACK f9ba6fd
I’m unable to run make successfully because of the following error on my system. I’ve noticed this in another PR as well and it is due to lack of a commit that was merged in later. Asked couple questions.
0/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/time.h:127:12: note: 'gmtime_r' declared here
1struct tm *gmtime_r(const time_t * __restrict, struct tm * __restrict);
274@@ -274,7 +275,7 @@ def run_test(self):
275 self.log.info(f"Restarted nodes: {self.restart_counts}; crashes on restart: {self.crashed_on_restart}")
276
277 # If no nodes were restarted, we didn't test anything.
278- assert self.restart_counts != [0, 0, 0]
279+ assert_not_equal(self.restart_counts, [0, 0, 0])
assert_equal is helpful because when a == 3 fails, it can be helpful to know what the value of a actually is without having to add debug code and rerun. If a != 3 fails, though, you already know that a == 3, so this doesn’t seem very helpful.
There are cases where you’re comparing a != b and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those seem pretty rare.
Concept -0 for me.
In thread #29500 (review):
Agree this is a valid argument against the PR. It is very seldom useful to print the values when they are equal.
I still prefer assert_not_equal() for:
assert_equal().assert which is skipped when running Python with -O.
Distinguishing itself from
assertwhich is skipped when running Python with-O.
It is not possible to run the functional tests with -O (they’d fail on current master), and it is unclear what reason there could be to do so in the first place. So i think using the flag as an argument for a code change seems weak.
If there is a reason for -O, it should be mentioned, all tests should be fixed, and it should be enforced with something like https://docs.astral.sh/ruff/rules/assert/.
If there is no reason for -O, it should be ignored or disallowed.
There are cases where you’re comparing a != b and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those seem pretty rare
I counted (by eyes, could be +/- a few):
assert_not_equal(peer.nServices & NODE_BLOOM, 0), andassert_not_equal(child_one_wtxid, child_two_wtxid)55@@ -56,6 +56,10 @@ def assert_equal(thing1, thing2, *args):
56 if thing1 != thing2 or any(thing1 != arg for arg in args):
57 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
58
59+def assert_not_equal(thing1, thing2, *args):
60+ if thing1 == thing2 or any(thing1 == arg for arg in args):
61+ raise AssertionError("%s" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
*args is non-empty: if you say assert_not_equal(a, 1, 2) and a is 1 or 2, it will report 1 == 1 == 2 or 2 == 1 == 2.
I’ve updated this to instead only take three arguments and the last one being an optional message since I did not see anywhere we were already asserting != in a chain.
This should clear up the message and allow the one location we do use a message to be used
updated in 4488120
@kevkevinpal Did you use a tool to generate the verification script?
No I did not I just manually used git grep and sed to create the script
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
Are you still working on this?
Yup just haven’t had much activity on it but rebased to 1d72266
Almost ACK 1d722660a6, modulo squash of the two commits.
This is a nice addition.
It is better to squash the two commits into one because IMO it does not make sense to first add the import in one commit and in another commit to use it. That way it becomes difficult to review whether an unnecessary import was added to some file. Also some python linter that tests each commit could be upset about the unused imports in the first commit.
I found it easier to read the diff itself instead of the “scripted diff” code (the line is 700+ chars long!). Further, I tweaked the diff to get the left and right sides align, so instead of:
0- assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
1+ assert_not_equal((cblks[3]['0'][33:65] < cblks[3]['1'][33:65]), (cblks[4]['0'][33:65] < cblks[4]['1'][33:65]))
I looked at:
0- assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
1+ ass_ne((cblks[3]['0'][33:65] < cblks[3]['1'][33:65]), (cblks[4]['0'][33:65] < cblks[4]['1'][33:65]))
I used git show 1d722660a6 |sed -E 's/^(.*)assert_not_equal([^,]*), (.*)$/\1ass_ne\2, \3/g' for that.
Needs rebase for CMake.
73@@ -74,6 +74,10 @@ def assert_equal(thing1, thing2, *args):
74 if thing1 != thing2 or any(thing1 != arg for arg in args):
75 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
76
77+def assert_not_equal(thing1, thing2, message=""):
error_message.
Concept ACK 1d722660a65522539872c09ae8a3ba8c9ca55b77 The util function looks quite different from the last time I reviewed, simpler and more readable now!
Couple points:
https://github.com/bitcoin/bitcoin/blob/master/test/lint/commit-script-check.sh#L11-L12
The resulting script should exactly transform the previous commit into the current one. Any remaining diff signals an error.
rebased to 6590b26
and I can address your latest comments soon
yes @rkrux you are right if I want to use a scripted diff then all the changes must be done using the script
I can remove the scripted diff and just squash them into a single diff since right now the script is getting very large already if that is preferred by others
@kevkevinpal what’s the status of this?
Just rebased and addressed @vasild comment
Almost ACK dcf9a45, modulo squash the two commits into one and rebase.
Just rebased and squashed the commits, I removed the scripted-diff because it was getting more difficult to review the diff than the actual code. But now all the changes are in d9cb032
if you do grep -nr assert\ .*!= ./test/functional/ there should only be one result
assert len(ss) == 175 - (in_type == SIGHASH_ANYONECANPAY) * 49 - (out_type != SIGHASH_ALL and out_type != SIGHASH_SINGLE) * 32 + (annex is not None) * 32 + scriptpath * 37
which we don’t need to add assert_not_equal to
26@@ -27,6 +27,7 @@
27 )
28 from test_framework.test_framework import BitcoinTestFramework
29 from test_framework.util import (
30+ assert_not_equal,
nit: Should appear after assert_equal alphabetically.
Same in test/functional/feature_dbcrash.py and others.
75@@ -76,6 +76,10 @@ def assert_equal(thing1, thing2, *args):
76 if thing1 != thing2 or any(thing1 != arg for arg in args):
77 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
78
79+def assert_not_equal(thing1, thing2, error_message=""):
80+ if thing1 == thing2:
81+ raise AssertionError("%s == %s %s" % (str(thing1), str(thing2), str(error_message)))
Should use f-strings: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/test/functional/README.md?plain=1#L40
nit: And possibly rephrase:
0 raise AssertionError(f"Both values are {thing1}{f', {error_message}' if error_message else ''}")
Went ahead an used your suggestion in be71af3
it now looks like this
02025-03-27T23:14:10.629000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
3 self.run_test()
4 File "/mnt/shared_drive/DEVDIR/bitcoin/./build_dir/test/functional/p2p_blockfilters.py", line 98, in run_test
5 assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
6 File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
7 raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
8AssertionError: Both values are 05ba865bb920e2ff5763411a9dd3c225ace8ff4393469bb25acffd56df1bfb6e, node 0 chain did not reorganize
69@@ -69,11 +70,11 @@ def run_test(self):
70 assert_equal(self.nodes[1].getblockcount(), 2000)
71
72 # Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
73- assert peer_0.nServices & NODE_COMPACT_FILTERS != 0
74+ assert_not_equal(peer_0.nServices & NODE_COMPACT_FILTERS, 0)
75 assert peer_1.nServices & NODE_COMPACT_FILTERS == 0
assert_equal() here and other directly adjacent places. Could touch up adjacent lines like this in separate commit?
assert keyword, and makes the test framework api more consistent internally and consistent with other frameworks.
75@@ -76,6 +76,10 @@ def assert_equal(thing1, thing2, *args):
76 if thing1 != thing2 or any(thing1 != arg for arg in args):
77 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
78
79+def assert_not_equal(thing1, thing2, error_message=""):
80+ if thing1 == thing2:
81+ raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
what is the point of the error_message, given that it is unused and highly fragile:
assert_not_equal("a", "b", "c") or assert_not_equal(1, 2, 3) will be wrong and confusing at best.
Thread #29500 (review):
It would be good if all assert_ functions supported error messages explaining why the condition should never fail, or what could be wrong if it has failed. (Built-in assert supports it too).
To avoid mixing the error message up with a value to be checked, one could do:
0def assert_not_equal(thing1, thing2, *, message=None):
1 if thing1 == thing2:
2 raise AssertionError(f"Both values are {thing1}{f', {message}' if message else ''}")
* enforces named parameters from that point.None as default value.The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:
02025-03-28T09:58:37.993000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
3 self.run_test()
4 File "/home/hodlinator/bitcoin/build/test/functional/p2p_blockfilters.py", line 98, in run_test
5 assert_not_equal(main_block_hash, stale_block_hash, message="node 0 chain did not reorganize")
6 File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
7 raise AssertionError(f"Both values are {thing1}{f', {message}' if message else ''}")
8AssertionError: Both values are 1b0ec65a9941d57790524ff90cc1cb93d2f8e70680c1c7486d9edf934d96a041, node 0 chain did not reorganize
assert_not_equal(main_block_hash, stale_block_hash, message=“node 0 chain did not reorganize”)
I’d say it is already self-explanatory to see assert_not_equal(main_block_hash, stale_block_hash) and the message “AssertionError: Both values are (the same)”. It should be clear that the chains are the same, when they should be different.
Even more so, given that the test logs before the assert: self.log.info("Reorg node 0 to a new chain.").
Unique and descriptive error messages make sense when it is the only piece of information given (for example in the context of an init error during startup of the program). Given that no other asserts in this file give the option, it would be best to defer this to a future where it is actually widely needed. However, given the extensive context in tests and test failures, I doubt it will ever be widely needed. Until then, it is just bike-shedding and overhead that has to be maintained and reviewed.
“AssertionError: Both values are (the same)”
Don’t know if you mean literally "Both values are (the same)" or f"Both values are {thing1}". Find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.
What it comes down to with custom error messages is being able to directly communicate with whoever is troubleshooting a failure. If one doesn’t even need to go to the logs to see what happened before, I consider it a small win. But agree it is bikeshedding-prone, so I would be okay with removing it.
Don’t know if you mean literally
"Both values are (the same)"orf"Both values are {thing1}". Find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.
Agree. Sorry for being unclear, I meant f"Both values are {thing1}" when I said “(the same)”.
re: #29500 (review)
I do like error_message parameter idea, fwiw. I could see these making tests more readable if they started being used for more important or more confusing checks. Tests are often written with some important checks directly related to the thing being tested, and other less important checks to see if other things are consistent. Having a place to indicate what it actually means when a check fails could be helpful for more important checks to distinguish them.
The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:
This is an annoying draw back that the stack trace won’t contain the line in the test that’s in error, but instead the line in the assert_not_equal helper. In Rust, one can decorate the helper function with the track_caller macro. I don’t see anything that is similar for C++ sadly.
We’re in Python here though. I spent a couple of hours with an LLM in mid-February to brute force this embryo: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2025/02/assert_ergonomics
Current assert_equal()-behavior:
02025-03-28T21:15:36.379000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
3 self.run_test()
4 File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
5 assert_equal(
6 File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
7 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
8AssertionError: not(0 == 123)
With first commit, chomping off util.py traceback entry you complained about:
02025-03-28T21:16:19.814000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
3 self.run_test()
4 File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
5 assert_equal(
6AssertionError: not(0 == 123)
With second commit, replacing another traceback entry with proper context:
02025-03-28T21:17:39.494000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
3 self.run_test()
4AssertionError: not(0 == 123)
5Failing expression: /home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py:80 - 83
6 assert_equal(
7 123 if (False and
8 True) else 0,
9 123)
The code needs more work, should be more defensive, forgot to output function name (run_test). And I wouldn’t bet on it being accepted anyway.
I don’t mind having an error message being logged and I do agree with the following points:
It is not a named arg, nor type-safe, so someone writing assert_not_equal(“a”, “b”, “c”) or assert_not_equal(1, 2, 3) will be wrong and confusing at best.
Unique and descriptive error messages make sense when it is the only piece of information given (for example in the context of an init error during startup of the program).
As I see from the usages of assert_not_equal, there is only 1 case where the error message is passed out of ~77 total usages. I believe a named argument for the error message could help in avoiding unintended usages of this utility function.
ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6
This change adds the missing opposite of assert_equal() and decreases usage of built-in assert for performing always-on checks during tests.
Concept ACK be71af3
I agree with @hodlinator on the import order NIT — let’s keep it consistent and maintain clean, high-quality code. #29500 (review)
Concept ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6
Requesting changes mainly because an additional comma is printed in case of an assertion error when the error message is not passed, which can be confusing for the reader later.
02025-03-31T07:28:44.958000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
12025-03-31T07:28:46.455000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3 File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
4 self.run_test()
5 ~~~~~~~~~~~~~^^
6 File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 141, in run_test
7 self.test_locked_wallet()
8 ~~~~~~~~~~~~~~~~~~~~~~~^^
9 File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 665, in test_locked_wallet
10 assert_not_equal(fundedTx["changepos"], 1)
11 ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
12 File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
13 raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
14AssertionError: Both values are 1,
152025-03-31T07:28:46.512000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
Updated to f3d4b6d
Added named variable for error_message and also made the assert able to take more than two variables to compare to each other. The error will now say
Two or more values equal to <value>: <error_message> or
Two or more values equal to <value>
if no error_message provided
82+ for i, val1 in enumerate(things):
83+ for val2 in things[i + 1:]:
84+ if val1 == val2:
85+ raise AssertionError(
86+ f"Two or more values equal to {val1}{f': {error_message}' if error_message else ''}"
87+ )
This is still wrong (the error message is not a named arg). Also, adding support for more values is unused and dead code.
My recommendation would be to just use assert_not_equal(thing1, thing2):. Any other attempt pushed to this pull so far was wrong or brittle.
fair point and thanks for the review
I’ve updated to 7bb83f6
(f"Both values are {thing1}{f', {error_message}' if error_message else ''}"error_message arg which is used in p2p_blockfilters75@@ -76,6 +76,16 @@ def assert_equal(thing1, thing2, *args):
76 if thing1 != thing2 or any(thing1 != arg for arg in args):
77 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
78
79+def assert_not_equal(*things, error_message=""):
Tested with:
0assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
Output was:
02025-04-01T08:15:47.847000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/hodlinator/b2/test/functional/test_framework/test_framework.py", line 182, in main
3 self.run_test()
4 File "/home/hodlinator/b2/build/test/functional/p2p_blockfilters.py", line 66, in run_test
5 assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
6 File "/home/hodlinator/b2/test/functional/test_framework/util.py", line 85, in assert_not_equal
7 raise AssertionError(
8AssertionError: Two or more values equal to 775ea4a5e5005376a49f2da6d20ce0449fbd627d766c8fc3108d22db0d41e116
This indicates that the message wasn’t recognized as such. Naming the parameter has the desired effect:
0assert_not_equal(stale_block_hash, stale_block_hash, error_message="node 0 chain did not reorganize")
A possible step forward would be to update callers to name the parameter. As I said before, I think the name should be shortened.
(Phantom space)
0def assert_not_equal(*things, error_message=""):
resolved in 7bb83f6
I went back to using two params and made error_message a named parameter that cannot be used as a positional argument
In the functional tests there are lots of cases where we assert != which
this new util will replace, we also are adding the imports and the new assertion
re-ACK 7bb83f6718110cb3a29e72522fdc5515db21c7d0
Remaining reservations from #29500 (review) (sorry for broken record):
error_message="" with the if error_message, instead of clear error_message=Noneerror_message= at call sites is quite verbose.
668@@ -668,7 +669,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat
669 self.start(extra_args, stdout=log_stdout, stderr=log_stderr, *args, **kwargs)
670 ret = self.process.wait(timeout=self.rpc_timeout)
671 self.log.debug(self._node_msg(f'bitcoind exited with status {ret} during initialization'))
672- assert ret != 0 # Exit code must indicate failure
673+ assert_not_equal(ret, 0) # Exit code must indicate failure