test: use assert_greater_than util #30019
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
l0rinc
left a comment
There was a problem hiding this comment.
I'm not sure I understand what's wrong with the original operators, this seems less readable to me
test/functional/rpc_blockchain.py
Outdated
There was a problem hiding this comment.
why so general, the previous one was a simple operator, why obfuscate it as such?
There was a problem hiding this comment.
that is fair I initially did it like this because there was one scenario where we used two < operators but I now switched to using assert_greater_than and now broke that statement up to two statements
In the functional tests there are lots of cases where we assert < which this util will replace, we also are adding the imports and the following commit will add the assertion
-BEGIN VERIFY SCRIPT-
git grep -l -E "assert .*<{1} [\=\<]{0}" -- ':(exclude)*script.py' ':(exclude)*rpc_createmultisig.py' ':(exclude)*feature_taproot.py' ':(exclude)*wallet_create_tx.py' ':(exclude)*p2p.py' ./test/functional | xargs sed -i.bak -e "/assert .*< / s/assert \(.*\) <\([^#]*\)/assert\2 < \1/"
git grep -l -E "assert .*<{1} [\=\<]{0}" -- ':(exclude)*script.py' ':(exclude)*rpc_createmultisig.py' ':(exclude)*feature_taproot.py' ':(exclude)*wallet_create_tx.py' ':(exclude)*p2p.py' ./test/functional | xargs sed -i.bak -e "/assert .*< / s/assert \(.*\) <\([^#]*\)/assert_greater_than(\1,\2)/g" -e "s/assert_greater_than(\(.*\)\(#.*\)/assert_greater_than(\1 \2/g" -e "/assert_greater_than(/ s/\ \ ,/,/g"
-END VERIFY SCRIPT-
For the cases which the scripted diff did not make sense these were manually updated, like if we had multiple operators on one line or if it was wrapped in () already
c51464c to
fe07516
Compare
l0rinc
left a comment
There was a problem hiding this comment.
The errors will improve this way, even though I wouldn't call the code more readable.
And to not make it less readable, I suggested a few other places where these changes could be applied.
| tx = self.nodes[0].gettransaction(txid=txid, verbose=True)['decoded'] | ||
| assert 0 < tx['locktime'] <= 201 | ||
| assert_greater_than(tx['locktime'], 0) | ||
| assert tx['locktime'] <= 201 |
There was a problem hiding this comment.
| assert tx['locktime'] <= 201 | |
| assert_greater_than(202, tx['locktime']) |
| # Tx A was accepted, Tx AB was not. | ||
| assert conflicted_AB_tx["confirmations"] < 0 | ||
| assert_greater_than(0, conflicted_AB_tx["confirmations"]) | ||
| assert conflicted_A_tx["confirmations"] > 0 |
There was a problem hiding this comment.
this looks like an actual assert_greater_than
| assert conflicted_A_tx["confirmations"] > 0 | |
| assert_greater_than(conflicted_A_tx["confirmations"], 0) |
| if port is None: | ||
| assert 0 < idx <= MAX_NODES | ||
| assert_greater_than(idx, 0) | ||
| assert idx <= MAX_NODES |
There was a problem hiding this comment.
| assert idx <= MAX_NODES | |
| assert_greater_than(MAX_NODES + 1, idx) |
There was a problem hiding this comment.
idx can equal MAX_NODES according to the code - alternatively could use assert_greater_than_or_equal, if that's more readable
|
|
||
| newbalance = alice.getbalance() | ||
| assert balance - newbalance < Decimal("0.001") #no more than fees lost | ||
| assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost |
There was a problem hiding this comment.
Do we need the Decimal here?
Seems basically the same as https://github.com/bitcoin/bitcoin/pull/30019/files?diff=split&w=1#diff-c1987c24fa3e5c788d16de5359500a19a1fea4f379e4288d076cf92be89541dcR478
There was a problem hiding this comment.
This is needed as this specific test is full of Decimal objects. This is what you get if you take it out.
❯ test/functional/wallet_abandonconflict.py
2024-05-07T21:36:52.495000Z TestFramework (INFO): PRNG seed is: 7791852828985173276
2024-05-07T21:36:52.495000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30
2024-05-07T21:36:55.004000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/wallet_abandonconflict.py", line 57, in run_test
assert_greater_than("0.001", balance - newbalance) #no more than fees lost
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/util.py", line 78, in assert_greater_than
if thing1 <= thing2:
^^^^^^^^^^^^^^^^
TypeError: '<=' not supported between instances of 'str' and 'decimal.Decimal'
2024-05-07T21:36:55.057000Z TestFramework (INFO): Stopping nodes
2024-05-07T21:36:55.165000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30
2024-05-07T21:36:55.165000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30/test_framework.log
2024-05-07T21:36:55.165000Z TestFramework (ERROR):
2024-05-07T21:36:55.165000Z TestFramework (ERROR): Hint: Call /Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/combine_logs.py '/var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30' to consolidate all logs
2024-05-07T21:36:55.165000Z TestFramework (ERROR):
2024-05-07T21:36:55.165000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-05-07T21:36:55.165000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-05-07T21:36:55.166000Z TestFramework (ERROR):
There was a problem hiding this comment.
I see you are a newcomer too. When reviewing code, better to open the files in the text editor and check for a broader context than what the github interface will present.
In my case, I use emacs with ediff, but all major IDEs will have a nice diff interface to inspect the PRs.
There was a problem hiding this comment.
I see you are a newcomer too.
this isn't necessary...
assert_greater_than("0.001", balance - newbalance)I wasn't suggesting comparing it to a string, of course, but to a decimal, as in the example I've provided, i.e.:
assert_greater_than(0.001, balance - newbalance)This seems to be working for me, is it failing for you?
There was a problem hiding this comment.
Oh, I see.
Still, better to leave to preserve potential bugs since this is refactoring.
| assert_equal(res['bogosize'], 16800), | ||
| assert_equal(res['bestblock'], node.getblockhash(HEIGHT)) | ||
| size = res['disk_size'] | ||
| assert size > 6400 |
There was a problem hiding this comment.
| assert size > 6400 | |
| assert_greater_than(size, 6400) |
| return sha256(b"".join(o.serialize() for o in txTo.vout)) | ||
|
|
||
| def TaprootSignatureMsg(txTo, spent_utxos, hash_type, input_index = 0, scriptpath = False, script = CScript(), codeseparator_pos = -1, annex = None, leaf_ver = LEAF_VERSION_TAPSCRIPT): | ||
| assert (len(txTo.vin) == len(spent_utxos)) |
There was a problem hiding this comment.
| assert (len(txTo.vin) == len(spent_utxos)) | |
| assert_equal(len(txTo.vin), len(spent_utxos)) |
edilmedeiros
left a comment
There was a problem hiding this comment.
Concept ACK.
I have run all the tests that have changed, it seems ok.
Thanks for the contribution. But, since you are already refactoring this out, I believe it would be way better to not leave any more assert behind that could be changed to assert_equal or assert_greater_than. Otherwise, looks like we are not improving the code base as there will be more than one style of asserts in some files.
See the diffs in the comments to find some suggestions.
To find others, grep each file for 'assert ' (with the space).
| assert bal1 == 0 | ||
| assert bal2 == self.moved |
There was a problem hiding this comment.
Change these to assert_equal is an easy gain here.
| if port is None: | ||
| assert 0 < idx <= MAX_NODES | ||
| assert_greater_than(idx, 0) | ||
| assert idx <= MAX_NODES |
|
|
||
| newbalance = alice.getbalance() | ||
| assert balance - newbalance < Decimal("0.001") #no more than fees lost | ||
| assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost |
There was a problem hiding this comment.
This is needed as this specific test is full of Decimal objects. This is what you get if you take it out.
❯ test/functional/wallet_abandonconflict.py
2024-05-07T21:36:52.495000Z TestFramework (INFO): PRNG seed is: 7791852828985173276
2024-05-07T21:36:52.495000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30
2024-05-07T21:36:55.004000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/wallet_abandonconflict.py", line 57, in run_test
assert_greater_than("0.001", balance - newbalance) #no more than fees lost
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/test_framework/util.py", line 78, in assert_greater_than
if thing1 <= thing2:
^^^^^^^^^^^^^^^^
TypeError: '<=' not supported between instances of 'str' and 'decimal.Decimal'
2024-05-07T21:36:55.057000Z TestFramework (INFO): Stopping nodes
2024-05-07T21:36:55.165000Z TestFramework (WARNING): Not cleaning up dir /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30
2024-05-07T21:36:55.165000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30/test_framework.log
2024-05-07T21:36:55.165000Z TestFramework (ERROR):
2024-05-07T21:36:55.165000Z TestFramework (ERROR): Hint: Call /Users/jose.edil/2-development/bitcoin/bitcoin-core/test/functional/combine_logs.py '/var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30' to consolidate all logs
2024-05-07T21:36:55.165000Z TestFramework (ERROR):
2024-05-07T21:36:55.165000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-05-07T21:36:55.165000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-05-07T21:36:55.166000Z TestFramework (ERROR):
|
|
||
| newbalance = alice.getbalance() | ||
| assert balance - newbalance < Decimal("0.001") #no more than fees lost | ||
| assert_greater_than(Decimal("0.001"), balance - newbalance) #no more than fees lost |
There was a problem hiding this comment.
I see you are a newcomer too. When reviewing code, better to open the files in the text editor and check for a broader context than what the github interface will present.
In my case, I use emacs with ediff, but all major IDEs will have a nice diff interface to inspect the PRs.
There was a problem hiding this comment.
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index cf48826e6a..415e4f1b95 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -15,6 +15,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
+ assert_greater_than_or_equal,
assert_raises_rpc_error,
)
from test_framework.wallet import MiniWallet
@@ -446,9 +447,9 @@ class ReplaceByFeeTest(BitcoinTestFramework):
num_txs_invalidated = len(root_utxos) + (num_tx_graphs * txs_per_graph)
if failure_expected:
- assert num_txs_invalidated > MAX_REPLACEMENT_LIMIT
+ assert_greater_than(num_txs_invalidated, MAX_REPLACEMENT_LIMIT)
else:
- assert num_txs_invalidated <= MAX_REPLACEMENT_LIMIT
+ assert_greater_than_or_equal(MAX_REPLACEMENT_LIMIT, num_txs_invalidated)
# Now attempt to submit a tx that double-spends all the root tx inputs, which
# would invalidate `num_txs_invalidated` transactions.There was a problem hiding this comment.
index ff67207c4e..2a4cc9dfba 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -85,6 +85,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
+ assert_greater_than_or_equal,
softfork_active,
assert_raises_rpc_error,
)
@@ -376,14 +377,14 @@ class SegWitTest(BitcoinTestFramework):
self.test_node.send_message(msg_headers())
self.test_node.announce_block_and_wait_for_getdata(block1, use_header=False)
- assert self.test_node.last_message["getdata"].inv[0].type == blocktype
+ assert_equal(self.test_node.last_message["getdata"].inv[0].type, blocktype)
test_witness_block(self.nodes[0], self.test_node, block1, True)
block2 = self.build_next_block()
block2.solve()
self.test_node.announce_block_and_wait_for_getdata(block2, use_header=True)
- assert self.test_node.last_message["getdata"].inv[0].type == blocktype
+ assert_equal(self.test_node.last_message["getdata"].inv[0].type, blocktype)
test_witness_block(self.nodes[0], self.test_node, block2, True)
# Check that we can getdata for witness blocks or regular blocks,
@@ -412,8 +413,8 @@ class SegWitTest(BitcoinTestFramework):
block = self.build_next_block()
self.update_witness_block_with_transactions(block, [])
# This gives us a witness commitment.
- assert len(block.vtx[0].wit.vtxinwit) == 1
- assert len(block.vtx[0].wit.vtxinwit[0].scriptWitness.stack) == 1
+ assert_equal(len(block.vtx[0].wit.vtxinwit), 1)
+ assert_equal(len(block.vtx[0].wit.vtxinwit[0].scriptWitness.stack), 1)
test_witness_block(self.nodes[0], self.test_node, block, accepted=True)
# Now try to retrieve it...
rpc_block = self.nodes[0].getblock(block.hash, False)
@@ -539,7 +540,7 @@ class SegWitTest(BitcoinTestFramework):
# Verify that if a peer doesn't set nServices to include NODE_WITNESS,
# the getdata is just for the non-witness portion.
self.old_node.announce_tx_and_wait_for_getdata(tx)
- assert self.old_node.last_message["getdata"].inv[0].type == MSG_TX
+ assert_equal(self.old_node.last_message["getdata"].inv[0].type, MSG_TX)
# Since we haven't delivered the tx yet, inv'ing the same tx from
# a witness transaction ought not result in a getdata.
@@ -807,7 +808,7 @@ class SegWitTest(BitcoinTestFramework):
block_3.vtx[0].vout[-1].nValue += 1
block_3.vtx[0].rehash()
block_3.hashMerkleRoot = block_3.calc_merkle_root()
- assert len(block_3.vtx[0].vout) == 4 # 3 OP_returns
+ assert_equal(len(block_3.vtx[0].vout), 4) # 3 OP_returns
block_3.solve()
test_witness_block(self.nodes[0], self.test_node, block_3, accepted=True)
@@ -838,7 +839,7 @@ class SegWitTest(BitcoinTestFramework):
block.solve()
block.vtx[0].wit.vtxinwit[0].scriptWitness.stack.append(b'a' * 5000000)
- assert block.get_weight() > MAX_BLOCK_WEIGHT
+ assert_greater_than(block.get_weight(), MAX_BLOCK_WEIGHT)
# We can't send over the p2p network, because this is too big to relay
# TODO: repeat this test with a block that can be relayed
@@ -850,7 +851,7 @@ class SegWitTest(BitcoinTestFramework):
assert_greater_than(MAX_BLOCK_WEIGHT, block.get_weight())
assert_equal(None, self.nodes[0].submitblock(block.serialize().hex()))
- assert self.nodes[0].getbestblockhash() == block.hash
+ assert_equal(self.nodes[0].getbestblockhash(), block.hash)
# Now make sure that malleating the witness reserved value doesn't
# result in a block permanently marked bad.
@@ -875,7 +876,7 @@ class SegWitTest(BitcoinTestFramework):
# Test that witness-bearing blocks are limited at ceil(base + wit/4) <= 1MB.
block = self.build_next_block()
- assert len(self.utxo) > 0
+ assert_greater_than(len(self.utxo), 0)
# Create a P2WSH transaction.
# The witness script will be a bunch of OP_2DROP's, followed by OP_TRUE.
@@ -896,7 +897,7 @@ class SegWitTest(BitcoinTestFramework):
for _ in range(NUM_OUTPUTS):
parent_tx.vout.append(CTxOut(child_value, script_pubkey))
parent_tx.vout[0].nValue -= 50000
- assert parent_tx.vout[0].nValue > 0
+ assert_greater_than(parent_tx.vout[0].nValue, 0)
parent_tx.rehash()
child_tx = CTransaction()
@@ -924,7 +925,7 @@ class SegWitTest(BitcoinTestFramework):
assert_equal(block.get_weight(), MAX_BLOCK_WEIGHT + 1)
# Make sure that our test case would exceed the old max-network-message
# limit
- assert len(block.serialize()) > 2 * 1024 * 1024
+ assert_greater_than(len(block.serialize()), 2 * 1024 * 1024)
test_witness_block(self.nodes[0], self.test_node, block, accepted=False, reason='bad-blk-we
ight')
@@ -934,7 +935,7 @@ class SegWitTest(BitcoinTestFramework):
block.vtx[0].vout.pop()
add_witness_commitment(block)
block.solve()
- assert block.get_weight() == MAX_BLOCK_WEIGHT
+ assert_equal(block.get_weight(), MAX_BLOCK_WEIGHT)
test_witness_block(self.nodes[0], self.test_node, block, accepted=True)
@@ -1098,7 +1099,7 @@ class SegWitTest(BitcoinTestFramework):
# This script is 19 max pushes (9937 bytes), then 64 more opcode-bytes.
long_witness_script = CScript([b'a' * MAX_SCRIPT_ELEMENT_SIZE] * 19 + [OP_DROP] * 63 + [OP_
TRUE])
- assert len(long_witness_script) == MAX_WITNESS_SCRIPT_LENGTH + 1
+ assert_equal(len(long_witness_script), MAX_WITNESS_SCRIPT_LENGTH + 1)
long_script_pubkey = script_to_p2wsh_script(long_witness_script)
block = self.build_next_block()
@@ -1122,7 +1123,7 @@ class SegWitTest(BitcoinTestFramework):
# Try again with one less byte in the witness script
witness_script = CScript([b'a' * MAX_SCRIPT_ELEMENT_SIZE] * 19 + [OP_DROP] * 62 + [OP_TRUE]
)
- assert len(witness_script) == MAX_WITNESS_SCRIPT_LENGTH
+ assert_equal(len(witness_script), MAX_WITNESS_SCRIPT_LENGTH)
script_pubkey = script_to_p2wsh_script(witness_script)
tx.vout[0] = CTxOut(tx.vout[0].nValue, script_pubkey)
@@ -1151,7 +1152,7 @@ class SegWitTest(BitcoinTestFramework):
for _ in range(10):
tx.vout.append(CTxOut(int(value / 10), script_pubkey))
tx.vout[0].nValue -= 1000
- assert tx.vout[0].nValue >= 0
+ assert_greater_than_or_equal(tx.vout[0].nValue, 0)
block = self.build_next_block()
self.update_witness_block_with_transactions(block, [tx])
@@ -1358,7 +1359,7 @@ class SegWitTest(BitcoinTestFramework):
temp_utxo.append(UTXO(tx.sha256, 0, tx.vout[0].nValue))
self.generate(self.nodes[0], 1) # Mine all the transactions
- assert len(self.nodes[0].getrawmempool()) == 0
+ assert_equal(len(self.nodes[0].getrawmempool()), 0)
# Finally, verify that version 0 -> version 2 transactions
# are standard
@@ -1622,7 +1623,7 @@ class SegWitTest(BitcoinTestFramework):
# Create a slight bias for producing more utxos
num_outputs = random.randint(1, 11)
random.shuffle(temp_utxos)
- assert len(temp_utxos) > num_inputs
+ assert_greater_than(len(temp_utxos), num_inputs)
tx = CTransaction()
total_value = 0
for i in range(num_inputs):
edilmedeiros
left a comment
There was a problem hiding this comment.
Another thing: I have the impression that the individual commits do not work individually. If so, think about squashing them all together. I don't think this will break the bank for other reviewers.
|
All right, better than my review would be to convert this to a scripted diff as you are doing in #29500. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Closing for now due to inactivity on a test-only change for more than 6 months. Leave a comment below, if you want this re-opened. |
In the functional tests there are lots of cases where we assert < which we now swap with assert_greater_than 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_greater_than to keep the PR smaller as suggested in #28528 (comment)
Similar change for assert_not_equal
I did not use the scripted-diff for the last commit since it either included a <= or there was already a () which did not match the pattern of the rest
if you run this command on this branch it should come back empty
git grep -nri -e "assert .*< " --and --not -e "assert .*!=" -- ./test/functional