Replaces bare asserts with test framework utils across both the functional tests and the test framework itself.
Also adds the assert_not_equal, assert_less_than, and assert_less_than_or_equal util functions for greater readability.
Fixes #23119.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | BrandonOdiwuor |
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:
keys_to_multisig_script (P2MS) helper for n/k > 16 by theStack)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.
227@@ -226,7 +228,7 @@ def _test_coin_stats_index(self):
228
229 self.generate(index_node, 1, sync_fun=self.no_op)
230 res10 = index_node.gettxoutsetinfo('muhash')
231- assert res8['txouts'] < res10['txouts']
232+ assert_greater_than(res10['txouts'], res8['txouts'])
assert x < y is replaced by assert_greater_than(y, x) rather than the more obvious assert_less_than(x, y). Is that change intentional or is that a left-over from an earlier version, when assert_less_than was not introduced yet?
assert_greater_than() before I decided that assert_less_than()was needed. I can go back and fix those to make it clear.
Concept ACK
This PR improves functional tests (readability) by using/introducing complimentary util functions to assert ie assert_equal, assert_not_equal, assert_less_than, assert_less_than_or_equal
rpc_psbt.py, wallet_disable.py is this intentional?
Replaces bare asserts with test framework utils across both the
functional tests and the test framework itself.
Also adds the `assert_not_equal`, `assert_less_than`, and
`assert_less_than_or_equal` util functions for greater readability.
Fixes #23119.Hi @BrandonOdiwuor,
Sorry, I missed the notification of your messages. I missed a few asserts, so I went back to add them. I also initially chose not to change any assert x is True/False as I wasn’t sure if it would be useful, but I decided now to change those as well.
Please take another look whenever you’re free, thank you!
275@@ -275,7 +276,8 @@ def signing_test(
276 self.wait_until(lambda: txid in self.funder.getrawmempool())
277 self.funder.generatetoaddress(1, self.funder.getnewaddress())
278 utxo = self.ms_sig_wallet.listunspent(addresses=[addr])[0]
279- assert txid == utxo["txid"] and utxo["solvable"]
280+ assert_equal(txid == utxo["txid"])
0 assert_equal(txid, utxo["txid"])
71@@ -67,6 +72,16 @@ def assert_greater_than_or_equal(thing1, thing2):
72 raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
73
74
75+def assert_less_than(thing1, thing2):
76+ if thing1 >= thing2:
77+ raise AssertionError("%s .= %s" % (str(thing1), str(thing2)))
2103e12: here and for assert_less_than_or_equal too.
0 raise AssertionError("%s >= %s" % (str(thing1), str(thing2)))
56@@ -57,6 +57,11 @@ def assert_equal(thing1, thing2, *args):
57 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
58
59
60+def assert_not_equal(thing1, thing2, *args):
assert_not_equal(thing1, thing2). i couldn’t find an occurrence where we needed to assert_not_equal 3+ things. ex: assert_not_equal(2, 2, 3)
i think you can find more places where assert_not_equal can be used using the command @theStack mentioned here!
example first 3 lines output:
0$ git grep "assert ".*\!=
1feature_fee_estimation.py: assert fee_dat_current_content != fee_dat_initial_content
2feature_fee_estimation.py: assert fee_dat_current_content != fee_dat_initial_content
3mempool_accept_wtxid.py: assert child_one_wtxid != child_two_wtxid
Closing for now due to inactivity. I think it is pretty clear that a change like this won’t be going in in one go.
If someone wants to try this again, I’d recommend to try a smaller portion and see if reviewers like it. If not, then maybe it isn’t worth changing?