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 |
|---|---|
| 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.
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
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
following commit will add the new assertion
-BEGIN VERIFY SCRIPT-
sed -i "s/assert sha256sum_file(dump_output\['path'\]) != sha256sum_file(dump_output4\['path'\])/assert_not_equal(sha256sum_file(dump_output['path']), sha256sum_file(dump_output4['path']))/g" ./test/functional/feature_assumeutxo.py
git grep -l "assert .*!=" -- ':(exclude)*script.py' ./test/functional | xargs sed -i -e "/assert (.*!=.*)/ s/assert (/assert_not_equal(/g" -e "/assert (.*) !=/ s/assert /assert_not_equal(/g" -e "/assert [^(].*!=/ s/assert /assert_not_equal(/g" -e "/assert_not_equal(/ s/ !=/,/g" -e "/assert_not_equal(.*#/ s/ #/) #/g" -e "/assert_not_equal([^#]*.[^)]$/ s/$/)/g" -e "/assert_not_equal([^#]*.[(][)][)]$/ s/$/)/g" -e "/assert_not_equal([^#]*.[(][)]$/ s/$/)/g" -e "/assert_not_equal([^#]*([a-zA-Z=_]*)$/ s/$/)/g" -e "s/assert_not_equal(cblks/assert_not_equal((cblks/g" -e "/assert_not_equal((cblks/ s/)$/))/g" -e "s/assert_not_equal(amount/assert_not_equal((amount/g" -e "/assert_not_equal((amount/ s/None)$/None))/g"
-END VERIFY SCRIPT-
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
🐙 This pull request conflicts with the target branch and needs rebase.