test: Use test framework utils in functional tests #28528

osagie98 · 2023-09-25T04:14:06Z

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.

DrahtBot · 2023-09-25T04:14:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29007 (test: create deterministic addrman in the functional tests by stratospher)
#28926 (rpc: add 'getnetmsgstats' RPC by willcl-ark)
#28890 (rpc: Remove deprecated -rpcserialversion by maflcko)
#28550 (Covenant tools softfork by jamesob)
#28312 (test: fix keys_to_multisig_script (P2MS) helper for n/k > 16 by theStack)
#28307 (rpc, wallet: fix incorrect segwit redeem script size limit by furszy)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

osagie98 · 2023-09-25T05:16:58Z

Though the change is simple, it touches a ton of files. I can break it into multiple PRs if that makes sense.

maflcko · 2023-10-05T16:56:32Z

@theStack Added you as reviewer/mentor, because the issue was opened by you

theStack · 2023-10-05T21:40:41Z

@osagie98: Can you squash the commits and rebase on master? Happy to review.

osagie98 · 2023-10-06T05:53:43Z

@theStack done (I hope, still getting used to the GitHub workflow). Let me know if you'll need anything else

theStack · 2023-10-24T23:47:58Z

Hi @theStack, any chance you'd be able to review this?

Yes, planning to (finally) review this within the next 1-2 days.

theStack · 2023-10-25T15:53:48Z

test/functional/feature_coinstatsindex.py

@@ -226,7 +228,7 @@ def _test_coin_stats_index(self):

        self.generate(index_node, 1, sync_fun=self.no_op)
        res10 = index_node.gettxoutsetinfo('muhash')
-        assert res8['txouts'] < res10['txouts']
+        assert_greater_than(res10['txouts'], res8['txouts'])


I noticed that here (and at some other places), 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?

Yes, early on I kept only assert_greater_than() before I decided that assert_less_than()was needed. I can go back and fix those to make it clear.

Done. Force pushed to allow for the rebase.

BrandonOdiwuor

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

BrandonOdiwuor

There are still some instances of bare assert which could be replaced by the helpers such as in 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 bitcoin#23119.

osagie98 · 2023-12-11T04:28:30Z

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!

kevkevinpal · 2024-01-04T14:58:25Z

test/functional/wallet_miniscript.py

@@ -275,7 +276,8 @@ def signing_test(
        self.wait_until(lambda: txid in self.funder.getrawmempool())
        self.funder.generatetoaddress(1, self.funder.getnewaddress())
        utxo = self.ms_sig_wallet.listunspent(addresses=[addr])[0]
-        assert txid == utxo["txid"] and utxo["solvable"]
+        assert_equal(txid == utxo["txid"])


Suggested change

assert_equal(txid == utxo["txid"])

assert_equal(txid, utxo["txid"])

stratospher · 2024-01-04T15:44:40Z

test/functional/test_framework/util.py

@@ -67,6 +72,16 @@ def assert_greater_than_or_equal(thing1, thing2):
        raise AssertionError("%s < %s" % (str(thing1), str(thing2)))


+def assert_less_than(thing1, thing2):
+    if thing1 >= thing2:
+        raise AssertionError("%s .= %s" % (str(thing1), str(thing2)))


2103e12: here and for assert_less_than_or_equal too.

Suggested change

raise AssertionError("%s .= %s" % (str(thing1), str(thing2)))

raise AssertionError("%s >= %s" % (str(thing1), str(thing2)))

stratospher · 2024-01-04T15:49:53Z

test/functional/test_framework/util.py

@@ -57,6 +57,11 @@ def assert_equal(thing1, thing2, *args):
        raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))


+def assert_not_equal(thing1, thing2, *args):


2103e12: maybe just have 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:

$ git grep "assert ".*\!= feature_fee_estimation.py: assert fee_dat_current_content != fee_dat_initial_content feature_fee_estimation.py: assert fee_dat_current_content != fee_dat_initial_content mempool_accept_wtxid.py: assert child_one_wtxid != child_two_wtxid

DrahtBot · 2024-01-05T11:39:38Z

🐙 This pull request conflicts with the target branch and needs rebase.

maflcko · 2024-02-22T17:40:49Z

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?

DrahtBot added Tests CI failed labels Sep 25, 2023

osagie98 marked this pull request as ready for review September 25, 2023 05:16

DrahtBot removed the CI failed label Sep 25, 2023

maflcko requested a review from theStack September 26, 2023 09:49

This was referenced Sep 28, 2023

Covenant tools softfork #28550

Closed

bugfix: throw an error if an invalid parameter is passed to getnetworkhashps RPC #28554

Merged

DrahtBot added the Needs rebase label Oct 5, 2023

osagie98 force-pushed the replace-bare-asserts branch from 1163d61 to 995cc47 Compare October 6, 2023 05:51

DrahtBot removed the Needs rebase label Oct 6, 2023

DrahtBot mentioned this pull request Oct 6, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot added the Needs rebase label Oct 8, 2023

osagie98 force-pushed the replace-bare-asserts branch from 995cc47 to ad6d9e2 Compare October 12, 2023 04:16

DrahtBot removed the Needs rebase label Oct 12, 2023

DrahtBot mentioned this pull request Oct 12, 2023

test: Add Wallet Unlock Context Manager #28617

Merged

DrahtBot added the CI failed label Oct 24, 2023

DrahtBot mentioned this pull request Oct 25, 2023

test: replace random_bytes with random.randbytes #28727

Merged

DrahtBot added Needs rebase and removed CI failed labels Oct 25, 2023

theStack reviewed Oct 25, 2023

View reviewed changes

osagie98 force-pushed the replace-bare-asserts branch from 8f00a52 to 146d75c Compare October 28, 2023 17:47

DrahtBot added Needs rebase and removed Needs rebase labels Oct 28, 2023

BrandonOdiwuor reviewed Nov 15, 2023

View reviewed changes

BrandonOdiwuor reviewed Nov 16, 2023

View reviewed changes

osagie98 force-pushed the replace-bare-asserts branch from 146d75c to 2103e12 Compare December 11, 2023 04:24

DrahtBot added CI failed and removed Needs rebase labels Dec 11, 2023

This was referenced Dec 11, 2023

rpc: add 'getnetmsgstats' RPC #28926

Closed

rpc: Remove deprecated -rpcserialversion #28890

Merged

DrahtBot mentioned this pull request Dec 21, 2023

test: create deterministic addrman in the functional tests #29007

Merged

stratospher mentioned this pull request Dec 29, 2023

Together we review series Bitshala/BitcoinCore-PR-Review-Club#49

Open

kevkevinpal reviewed Jan 4, 2024

View reviewed changes

stratospher reviewed Jan 4, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jan 5, 2024

stratospher mentioned this pull request Jan 21, 2024

TWR_EP02_PR #28528_2024-01-04 final Bitshala/BitcoinCore-PR-Review-Club#52

Closed

maflcko closed this Feb 22, 2024

kevkevinpal mentioned this pull request Feb 28, 2024

test: create assert_not_equal util #29500

Open

This was referenced Apr 27, 2024

test: create assert_less_than util kevkevinpal/bitcoin#12

Open

test: use assert_greater_than util #30019

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Use test framework utils in functional tests #28528

test: Use test framework utils in functional tests #28528

osagie98 commented Sep 25, 2023

DrahtBot commented Sep 25, 2023 •

edited

osagie98 commented Sep 25, 2023

maflcko commented Oct 5, 2023

theStack commented Oct 5, 2023

osagie98 commented Oct 6, 2023

theStack commented Oct 24, 2023

theStack Oct 25, 2023 •

edited

osagie98 Oct 26, 2023 •

edited

osagie98 Oct 28, 2023

BrandonOdiwuor left a comment

BrandonOdiwuor left a comment

osagie98 commented Dec 11, 2023

kevkevinpal Jan 4, 2024

stratospher Jan 4, 2024

stratospher Jan 4, 2024

stratospher Jan 4, 2024

DrahtBot commented Jan 5, 2024

maflcko commented Feb 22, 2024

	assert_equal(txid == utxo["txid"])
	assert_equal(txid, utxo["txid"])

	raise AssertionError("%s .= %s" % (str(thing1), str(thing2)))
	raise AssertionError("%s >= %s" % (str(thing1), str(thing2)))

		@@ -57,6 +57,11 @@ def assert_equal(thing1, thing2, *args):
		raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))


		def assert_not_equal(thing1, thing2, *args):

test: Use test framework utils in functional tests #28528

test: Use test framework utils in functional tests #28528

Conversation

osagie98 commented Sep 25, 2023

DrahtBot commented Sep 25, 2023 • edited

Code Coverage

Reviews

Conflicts

osagie98 commented Sep 25, 2023

maflcko commented Oct 5, 2023

theStack commented Oct 5, 2023

osagie98 commented Oct 6, 2023

theStack commented Oct 24, 2023

theStack Oct 25, 2023 • edited

Choose a reason for hiding this comment

osagie98 Oct 26, 2023 • edited

Choose a reason for hiding this comment

osagie98 Oct 28, 2023

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

osagie98 commented Dec 11, 2023

kevkevinpal Jan 4, 2024

Choose a reason for hiding this comment

stratospher Jan 4, 2024

Choose a reason for hiding this comment

stratospher Jan 4, 2024

Choose a reason for hiding this comment

stratospher Jan 4, 2024

Choose a reason for hiding this comment

DrahtBot commented Jan 5, 2024

maflcko commented Feb 22, 2024

DrahtBot commented Sep 25, 2023 •

edited

theStack Oct 25, 2023 •

edited

osagie98 Oct 26, 2023 •

edited