This PR removes when it is possible the native python assert, and use the test utils function, like assert_equal, assert_greater_than, assert_greater_than_or_equal.
In addition, this PR includes a new operator assert_not_equal, which IMO it is useful to test other conditions and add the error message as a parameter of the function to give more flexibility.
Fixes #23119
Edit laanwj: removed HTML comment from post
123@@ -123,52 +124,52 @@ def run_test(self):
124 wallet = node_v18.get_wallet_rpc("w1_v18")
125 info = wallet.getwalletinfo()
126 assert info['private_keys_enabled']
127- assert info['keypoolsize'] > 0
128+ assert_greater_than(info['keypoolsize'] > 0)
, instead of >
Will it be created new PRs to update other tests or should we concentrate all the changes here?
The plan is to divide the work into small chunks to make the price of PR review easy and fast.
Will it be created new PRs to update other tests or should we concentrate all the changes here?
The plan is to divide the work into small chunks to make the price of PR review easy and fast.
Cool, thanks!
I just created #23135 to help on it
Thanks, the important think it that we don’t make the double work :-)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Concept ACK
I think generally splitting across multiple pull requests works best for the monster sub-projects (AssumeUTXO, deglobalizing ChainstateManager, Process Separation etc).
In this case splitting across multiple commits within the same PR would probably have been sufficient to make it easily reviewable but not a big deal and if it encourages multiple contributors to self-organize around completing it then that’s great.
49@@ -50,6 +50,11 @@ def assert_equal(thing1, thing2, *args):
50 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
51
52
53+def assert_not_equal(thing1, thing2, *args):
54+ if thing1 == thing2 or any(thing1 != arg for arg in args):
Should it possibly be:
0 if thing1 == thing2 or any(thing1 == arg for arg in args):
?
Thanks @michaelfolkson,
In this case splitting across multiple commits within the same PR would probably have been sufficient to make it easily reviewable but not a big deal and if it encourages multiple contributors to self-organize around completing it then that’s great.
It sounds great to me, I will go with this solution. Thansk!
214+ assert_equal(self.nodes[1].getrawtransaction(tx_id, False, blockhash), self.nodes[2].getrawtransaction(tx_id, False, blockhash))
215 assert self.nodes[0].getrawtransaction(tx_id, False, blockhash) != self.nodes[2].gettransaction(tx_id)["hex"]
216- assert self.nodes[1].getrawtransaction(tx_id, False, blockhash) == self.nodes[2].gettransaction(tx_id)["hex"]
217- assert self.nodes[0].getrawtransaction(tx_id, False, blockhash) == tx.serialize_without_witness().hex()
218+ assert_equal(self.nodes[1].getrawtransaction(tx_id, False, blockhash), self.nodes[2].gettransaction(tx_id)["hex"])
219+ assert_equal( elf.nodes[0].getrawtransaction(tx_id, False, blockhash), tx.serialize_without_witness().hex())
Ready for review?
All the commit for the moment yes, I need to finish other files in the next week, but until now yes, it is ready
Concept ACK 9edb3cc5d08caaf61ff00c49a698956f4ddaba08, looked over the commits briefly and a) didn’t find any mistakes b) checked the changes are contained to where they should be
though I haven’t reviewed the huge list of changes in detail so can’t warrant calling it a full code review ACK.
though I haven’t reviewed the huge list of changes in detail so can’t warrant calling it a full code review ACK.
Sorry about that, I should add some tips on the PR. However, there is only a critical change in the test utils framework, now all the assert support the error message as the parameter this only because we can support also custom message defined with assert False, "always false"
Sorry about that, I should add some tips on the PR.
I think you misunderstood: It was not a comment on your work. I meant that I can’t call what I did a full code review. I should look in more detail, though generally getting the type of assert wrong will usually just result in the tests failing, so I’m reasonably confident in the correctness of the changes.
I think generally splitting across multiple pull requests works best for the monster sub-projects (AssumeUTXO, deglobalizing ChainstateManager, Process Separation etc).
I know I’ve previously said this (so apologies if this wasn’t helpful at the time) but I think the PR has got sufficiently big that a PR or two could maybe be spun out of this one to make it easier to review.
I also think the structure of commits could be improved, having a separate commit for each file doesn’t make much sense to me. Also the commit messages aren’t particularly informative for reviewers.
Commit messages such as “Define new assert functions in test utils”, “Replace assert with assert_equal across functional test suite” and “Use assert_greater_than instead of greater than asserts” would be more helpful than “filename.py: Use test framework”.
Reviewers can see what files are changed without looking at commit messages, what they want to see with the commit messages is what the commit is doing.
I think generally splitting across multiple pull requests works best for the monster sub-projects (AssumeUTXO, deglobalizing ChainstateManager, Process Separation etc).
I know I’ve previously said this (so apologies if this wasn’t helpful at the time) but I think the PR has got sufficiently big that a PR or two could maybe be spun out of this one to make it easier to review.
I also think the structure of commits could be improved, having a separate commit for each file doesn’t make much sense to me. Also the commit messages aren’t particularly informative for reviewers.
Commit messages such as “Define new assert functions in test utils”, “Replace assert with assert_equal across functional test suite” and “Use assert_greater_than instead of greater than asserts” would be more helpful than “filename.py: Use test framework”.
Reviewers can see what files are changed without looking at commit messages, what they want to see with the commit messages is what the commit is doing.
Hey, I agree with you.
Maybe this pull request could be splitted into three different ones.
For example: one pull request per each assertion type to refactor assert_equal, assert_greater_than and assert_greater_than_or_equal
I’d would like to help with the reviews!
Maybe this pull request could be splitted into three different ones. For example: one pull request per each assertion type to refactor assert_equal, assert_greater_than and
Each following step required the same trade-off with only personal preference, if you split the PR in more PRs, you have more work to do, and so on.
The change is not only in the change of the assert tp assert_* but this PR includes also some change inside the test framework, now the method accepts also an optional parameter err_msg
This required a big pr that include this change too.
In conclusion, I agree that small PR can be useful for other people, but at the end of the day, there is no real improvement inside the velocity of review.
This takes time, as with all types of refactoring. This time is chosen this way but the next time we can do in another way that we can set up before finish the work. Back in October, I chatted on IRC to choose which method is the best, so this is not my own choice.
43@@ -44,6 +44,7 @@
44 from test_framework.test_framework import BitcoinTestFramework
45 from test_framework.util import (
46 assert_equal,
47+ assert_not_equal,
48 assert_greater_than_or_equal,
49 assert_is_hex_string,
Nit. Maintain alphabetical import ordering within this file.
0 assert_greater_than_or_equal,
1 assert_is_hex_string,
2 assert_not_equal,
97+ assert_equal,
98+ assert_not_equal,
99+ assert_true,
100+ assert_greater_than,
101+ assert_greater_than_or_equal,
102+)
Nit.
0from test_framework.util import (
1 assert_equal,
2 assert_greater_than,
3 assert_greater_than_or_equal,
4 assert_not_equal,
5 assert_raises_rpc_error,
6 assert_true,
7)
523+ assert_true(hash_str not in mempool_view)
524 mempool_view.add(hash_str)
525 expected_sequence = mempool_sequence + 1
526 elif label == "R":
527- assert hash_str in mempool_view
528+ assert_true(hash_str in mempool_view)
45@@ -46,7 +46,7 @@ def run_test(self):
46 # blockchain sync later in the test when nodes are connected, due to
47 # timing issues.
48 for n in self.nodes:
49- assert n.getblockchaininfo()["initialblockdownload"] == False
50+ assert_equal(n.getblockchaininfo()["initialblockdownload"], False)
Seems there are too many ways to say the same thing. I think we shouldn’t be adding even more.
For boolean values, if the assertion fails, it is clear what value the bool was. There is no need to print the value and a constant.
Seems there are too many ways to say the same thing. I think we shouldn’t be adding even more.
Right, in this case, I use the assert_equal but I could use assert_true, and the reason that I used this is only for consistency across error messages and source code. However, if you like I can restore the pure assert
For boolean values, if the assertion fails, it is clear what value the bool was. There is no need to print the value and a constant.
I’d agree in a statically typed environment, but in Python that doesn’t really hold? I think there’s merit in always printing the value, to more quickly debug cases where e.g. the type of a response changes or becomes None/empty string?
591@@ -585,7 +592,7 @@ def bitflipper(expr):
592 """Return a callable that evaluates expr and returns it with a random bitflip."""
593 def fn(ctx):
594 sub = deep_eval(ctx, expr)
595- assert isinstance(sub, bytes)
596+ assert_true(isinstance(sub, bytes), err_msg="It is not a bytes instance")
isinstance(sub, bytes) says it all, I don’t think any more information needs to be added here.
1290@@ -1284,15 +1291,16 @@ def block_submit(self, node, txs, msg, err_msg, cb_pubkey=None, fees=0, sigops_w
1291 block.solve()
1292 block_response = node.submitblock(block.serialize().hex())
1293 if err_msg is not None:
1294- assert block_response is not None and err_msg in block_response, "Missing error message '%s' from block response '%s': %s" % (err_msg, "(None)" if block_response is None else block_response, msg)
1295+ assert_true(block_response is not None and err_msg in block_response,
1296+ err_msg= "Missing error message '%s' from block response '%s': %s" % (err_msg, "(None)" if block_response is None else block_response, msg))
Nit.
0 err_msg="Missing error message '%s' from block response '%s': %s" % (err_msg, "(None)" if block_response is None else block_response, msg))
1434@@ -1427,7 +1435,9 @@ def test_spenders(self, node, spenders, input_counts):
1435 for i in range(len(input_utxos)):
1436 if input_utxos[i].spender.need_vin_vout_mismatch:
1437 first_mismatch_input = i
1438- assert first_mismatch_input is None or first_mismatch_input > 0
1439+
1440+ if first_mismatch_input is not None:
1441+ assert_greater_than(first_mismatch_input, 0)
20@@ -21,6 +21,8 @@
21 )
22 from test_framework.util import (
23 assert_equal,
24+ assert_true,
25+ assert_greater_than,
26 assert_raises_rpc_error,
27 )
Nit. Sort imports.
0from test_framework.util import (
1 assert_equal,
2 assert_greater_than,
3 assert_raises_rpc_error,
4 assert_true,
5)
68@@ -67,9 +69,9 @@ def receive_sequence(self):
69 label = chr(body[32])
70 mempool_sequence = None if len(body) != 32+1+8 else struct.unpack("<Q", body[32+1:])[0]
71 if mempool_sequence is not None:
72- assert label == "A" or label == "R"
73+ assert_true(label == "A" or label == "R", err_msg="{} different from A or C".format(label))
Error message fix.
0 assert_true(label == "A" or label == "R", err_msg="{} different from A or R".format(label))
9@@ -10,7 +10,7 @@
10
11 from test_framework.blocktools import COINBASE_MATURITY
12 from test_framework.test_framework import BitcoinTestFramework
13-from test_framework.util import get_rpc_proxy
14+from test_framework.util import get_rpc_proxy, assert_equal
Nit. Sort imports (decided to point out each instance to save you the time).
0from test_framework.util import assert_equal, get_rpc_proxy
62@@ -63,6 +63,8 @@
63 from test_framework.test_framework import BitcoinTestFramework
64 from test_framework.util import (
65 assert_equal,
66+ assert_true,
67+ assert_greater_than_or_equal,
Nit. Sort imports.
0 assert_greater_than_or_equal,
1 assert_true,
730@@ -729,7 +731,7 @@ def test_end_to_end_block_relay(self, listeners):
731 # but invalid transactions.
732 def test_invalid_tx_in_compactblock(self, test_node, use_segwit=True):
733 node = self.nodes[0]
734- assert len(self.utxos)
735+ assert_true(len(self.utxos) > 0)
> 0 is to improve readability? The original reads better imo. Same comment L768.
I make this choice for consistency, across error messages and assert convention. I usually try to avoid missing pure assert and assert that came from utils code like the bitcoin test framework.
But I admit that in this case, it would be better assert_not_empity(self.utxos)
However, if you disagree I would restore the previous status with the pure assert
8@@ -9,7 +9,10 @@
9 """
10
11 from test_framework.test_framework import BitcoinTestFramework
12-from test_framework.util import assert_raises_rpc_error
13+from test_framework.util import (
14+ assert_raises_rpc_error,
15+ assert_equal,
16+)
Nit. Sort imports.
0from test_framework.util import (
1 assert_equal,
2 assert_raises_rpc_error,
3)
15@@ -16,6 +16,7 @@
16 from test_framework.util import (
17 assert_raises_rpc_error,
18 assert_equal,
19+ assert_true,
20 )
Nit. I can see this files imports aren’t alphabetical but may as well since touching these LOC.
0from test_framework.util import (
1 assert_equal,
2 assert_raises_rpc_error,
3 assert_true,
4)
16@@ -17,6 +17,8 @@
17 assert_greater_than,
18 assert_greater_than_or_equal,
19 assert_raises_rpc_error,
20+ assert_true,
21+ fail,
22 count_bytes,
23 find_vout_for_address,
Nit.
0 count_bytes,
1 fail,
2 find_vout_for_address,
94+def assert_true(thing, err_msg=None):
95+ if thing is not True:
96+ msg = "%s it is not True" % str(thing)
97+ if err_msg is not None:
98+ msg = err_msg
99+ raise AssertionError(msg)
Minor: avoid initializing a default message when an err_msg is provided.
0def assert_true(thing, err_msg=None):
1 if thing is not True:
2 msg = err_msg or "%s it is not True" % str(thing)
3 raise AssertionError(msg)
44@@ -45,19 +45,52 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
45 raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
46
47
48-def assert_equal(thing1, thing2, *args):
49+def assert_equal(thing1, thing2, *args, err_msg=None):
50 if thing1 != thing2 or any(thing1 != arg for arg in args):
51- raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
52+ msg = "not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)
LGTM:
0>>> thing1 = "foo"
1>>> thing2 = "bar"
2>>> args = ("baz", "qux")
3>>> msg = "not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)
4>>> msg
5'not(foo == bar == baz == qux)'
76+def assert_less_than(thing1, thing2, err_msg=None):
77+ if thing1 >= thing2:
78+ msg = "%s >= %s" % (str(thing1), str(thing2))
79+ if err_msg is not None:
80+ msg = err_msg
81+ raise AssertionError(msg)
Minor, you could avoid initializing msg when an err_msg is provided.
0def assert_true(thing, err_msg=None):
1 if thing is not True:
2 msg = err_msg or "%s it is not True" % str(thing)
3 raise AssertionError(msg)
0def assert_less_than(thing1, thing2, err_msg=None):
1 if thing1 >= thing2:
2 msg = err_msg or %s >= %s" % (str(thing1), str(thing2))
3 raise AssertionError(msg)
206@@ -206,7 +207,7 @@ def test_desc(self, node, address, multisig, typ, utxo):
207 assert_equal(info['desc'], descsum_create("wsh(multi(2,%s,%s))" % (key_descs[info['pubkeys'][0]], key_descs[info['pubkeys'][1]])))
208 else:
209 # Unknown type
210- assert False
211+ fail("Unknown type")
# Unknown type likely redundant now.
test/functional/test_runner.py --extended.
I don’t see why splitting into one commit per file does anything to help review. The contrary, I’d say. The UI already splits per file for you.
When I start to work on that I had the opposite advice, I think this could be a personal choice, I’m sorry if it makes things worse for you.
I’m fine with this splitting because each commit has only a personal portion of all the change, and usually, the UI starts to be very slow for a lot of changes. In addition, for disability reason, I don’t use the UI to review PR, but only to leave a comment, so I considered this approach more accessible too
I don’t see why splitting into one commit per file does anything to help review. The contrary, I’d say. The UI already splits per file for you.
When I start to work on that I had the opposite advice, I think this could be a personal choice, I’m sorry if it makes things worse for you.
I don’t think anyone suggested that it should be one commit per file right? I suggested splitting across commits rather than different PRs but not giving each file a separate commit.
I suggested this:
I also think the structure of commits could be improved, having a separate commit for each file doesn’t make much sense to me. Also the commit messages aren’t particularly informative for reviewers.
Commit messages such as “Define new assert functions in test utils”, “Replace assert with assert_equal across functional test suite” and “Use assert_greater_than instead of greater than asserts” would be more helpful than “filename.py: Use test framework”.
Reviewers can see what files are changed without looking at commit messages, what they want to see with the commit messages is what the commit is doing.
I personally think this isn’t serious enough (others can assess) that it means it is not yet in a merge-able state but yeah I agree with Kalle that splitting into one commit per file isn’t optimal.
I don’t think anyone suggested that it should be one commit per file right? I suggested splitting across commits rather than different PRs but not giving each file a separate commit.
Splitting between commits is a too general definition because splitting between commits brings the following problems:
This is the reason that I choose to divide the file change each one in a separate commit because you can fetch one commit and test in a clean env only on the functional test that you need.
IMHO this is the more clean way to help the reviewer and avoid introducing regression
This is also a good PR definition because to make small PRs you need just to cherry-pick the commits!
I don’t see any drawback with my solution, right?
P.S: In addition, this solution helps also to rebase the PRs and make the tests only on the file changed, that in this case is the single commit
65@@ -66,6 +66,11 @@ def assert_greater_than_or_equal(thing1, thing2):
66 raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
67
68
69+def assert_true(thing, message = None):
70+ if thing is not True:
71+ msg = "%s it is not True" if message is None else message
it needs to be removed from this sentence, also % str(thing) needs to be used to perform formatting so that the value of thing ends up in the message.
Good catch, thanks!
I move the following test framework to the following convention
0def assert_true(thing, err_msg=None):
1 if thing is not True:
2 msg = err_msg or "%s is not True" % str(thing)
3 raise AssertionError(msg)
I just removed it,
Also this commit introduce another utils function to tests assert_not_equal
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
# Title:
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
# Title:
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
# Title:
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
