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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29500.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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
Concept ACK, thanks for picking this up.
There are some places you are using assert_not_equal but not importing it and other ones you're importing it twice. Check CI.
Running Unit Tests for Test Framework Modules
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 906, in <module>
main()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 539, in main
run_tests(
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 580, in run_tests
test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3.12/unittest/loader.py", line 137, in loadTestsFromName
module = __import__(module_name)
^^^^^^^^^^^^^^^^^^^^^^^
File "/ci_container_base/test/functional/test_framework/address.py", line 14, in <module>
from .script import (
File "/ci_container_base/test/functional/test_framework/script.py", line 14, in <module>
from .key import TaggedHash, tweak_add_pubkey, compute_xonly_pubkey
File "/ci_container_base/test/functional/test_framework/key.py", line 16, in <module>
from test_framework.crypto import secp256k1
File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 316, in <module>
G = GE.lift_x(0x79BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 257, in lift_x
y = (FE(x)**3 + 7).sqrt()
^^^^^
File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 41, in __init__
assert_not_equal(den, 0)
^^^^^^^^^^^^^^^^
NameError: 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,
Importing twice? (from 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
git 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
test 2024-03-05T05:30:24.555000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 448, in run_test
self.test_reorg_2child_rbf()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 29, in wrapper
func(self)
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
assert_not_equal((child_1_conflict["txid"], child_1["txid"]))
TypeError: assert_not_equal() missing 1 required positional argument: 'thing2'
not sure why this ci is failing 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
Extra spaces after 0?
not sure why it is doing that I will check the script to see why
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.
that is strange I would think that the 3rd arg would make the tests fail in this case
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.
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/time.h:127:12: note: 'gmtime_r' declared here
struct tm *gmtime_r(const time_t * __restrict, struct tm * __restrict);
@kevkevinpal Did you use a tool to generate the verification script?
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))
This error message doesn't make sense when *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.
Yes that would not make sense let me see if I can improve the logging here
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
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/25253865668</sub>
rebased to 4358c6e
Are you still working on this?
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:
- assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
+ 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:
- assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
+ 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=""):
Nit: The 3rd argument can be called 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?
Almost ACK dcf9a45aab4c4a7f9f900d5dafcdfe0e9a598a38, modulo squash the two commits into one and rebase.
@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:
raise AssertionError(f"Both values are {thing1}{f', {error_message}' if error_message else ''}")
Would be more eager to ACK if code introduced by the PR followed bitcoin/test/functional/README.md as noted above. If you don't want to change the phrasing towards what's in the nit-part that's fine.
Went ahead an used your suggestion in be71af3
it now looks like this
2025-03-27T23:14:10.629000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
self.run_test()
File "/mnt/shared_drive/DEVDIR/bitcoin/./build_dir/test/functional/p2p_blockfilters.py", line 98, in run_test
assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
AssertionError: Both values are 05ba865bb920e2ff5763411a9dd3c225ace8ff4393469bb25acffd56df1bfb6e, node 0 chain did not reorganize
Thread #29500 (review): Thanks for taking my suggestion!
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
nit: Feels a bit weird to not have assert_equal() here and other directly adjacent places. Could touch up adjacent lines like this in separate commit?
Concept ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497
ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497
Code review ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497. Seems like a nice change, though needs to be rebased. I think this change is good because it makes tests more readable, prints more information on failures, avoids misusing the assert keyword, and makes the test framework api more consistent internally and consistent with other frameworks.
rebased to 29fa2b9
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:
def assert_not_equal(thing1, thing2, *, message=None):
if thing1 == thing2:
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:
2025-03-28T09:58:37.993000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
self.run_test()
File "/home/hodlinator/bitcoin/build/test/functional/p2p_blockfilters.py", line 98, in run_test
assert_not_equal(main_block_hash, stale_block_hash, message="node 0 chain did not reorganize")
File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
raise AssertionError(f"Both values are {thing1}{f', {message}' if message else ''}")
AssertionError: 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
<details><summary>Output examples</summary>
Current assert_equal()-behavior:
2025-03-28T21:15:36.379000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
self.run_test()
File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
assert_equal(
File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(0 == 123)
With first commit, chomping off util.py traceback entry you complained about:
2025-03-28T21:16:19.814000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
self.run_test()
File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
assert_equal(
AssertionError: not(0 == 123)
With second commit, replacing another traceback entry with proper context:
2025-03-28T21:17:39.494000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
self.run_test()
AssertionError: not(0 == 123)
Failing expression: /home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py:80 - 83
assert_equal(
123 if (False and
True) else 0,
123)
</details>
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.
No strong opinion, but I think we shouldn't spend too much time on the error message explaining the context because the traceback could contain too little context, or on the traceback containing too much (redundant) context by default. A full log of a test failure could be thousands of lines, so one additional line, even if could be redundant in a strict sense should be fine. Also, looking at the historic failures (in Python) at https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue%20%20label%3A%22CI%20failed%22%20, it was never(?) an issue to figure out what failed where. The problem usually is to figure out why something failed, and how to reproduce and fix it.
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)
Code review ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6. Just rebased and added f-string since last 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.
2025-03-31T07:28:44.958000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
2025-03-31T07:28:46.455000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 141, in run_test
self.test_locked_wallet()
~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 665, in test_locked_wallet
assert_not_equal(fundedTx["changepos"], 1)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
AssertionError: Both values are 1,
2025-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:
assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
Output was:
2025-04-01T08:15:47.847000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hodlinator/b2/test/functional/test_framework/test_framework.py", line 182, in main
self.run_test()
File "/home/hodlinator/b2/build/test/functional/p2p_blockfilters.py", line 66, in run_test
assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
File "/home/hodlinator/b2/test/functional/test_framework/util.py", line 85, in assert_not_equal
raise AssertionError(
AssertionError: Two or more values equal to 775ea4a5e5005376a49f2da6d20ce0449fbd627d766c8fc3108d22db0d41e116
This indicates that the message wasn't recognized as such. Naming the parameter has the desired effect:
assert_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)
def 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.
Code review ACK 7bb83f6718110cb3a29e72522fdc5515db21c7d0. Only change since last review is fixing error message formatting and passing it as a keyword argument
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
The error message arg can/could have been used here instead of the comment.
Post-merge ACK 7bb83f6718110cb3a29e72522fdc5515db21c7d0