tests: Use test utils each time that it is possible

vincenzopalazzo commented at 10:41 pm on July 28, 2022: none

This is one of my attempts to make less painful the #23127 and try to bring this simple refactoring in.

Issue related #23119

PR 1/N I think there are a couple of equal size later

DrahtBot added the label Tests on Jul 28, 2022

DrahtBot commented at 7:20 am on July 29, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)
#23127 (tests: Use test framework utils where possible by vincenzopalazzo)

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.

fanquake commented at 9:02 am on July 29, 2022: member

0test/functional/interface_zmq.py:367: error: unexpected indent  [syntax]
1Found 1 error in 1 file (errors prevented further checking)
2/tmp/cirrus-ci-build/test/functional/interface_zmq.py:367: unexpected indent at "assert_raises_rpc_error(-8, "Verbose results cannot contain mempool sequence values.", self.nodes[0].getrawmempool, True, True)"
3Python dead code detection found some issues

vincenzopalazzo force-pushed on Jul 29, 2022

vincenzopalazzo commented at 9:28 am on July 29, 2022: none

Thanks! Cherry-pick problem with an older version of the test/functional/interface_zmq.py file, lets see if the CI catch other stuff

vincenzopalazzo force-pushed on Jul 29, 2022

in test/functional/test_framework/util.py:55 in ad9dcabd5d outdated

52     if thing1 != thing2 or any(thing1 != arg for arg in args):
53-        raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
54+        msg = "not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)
55+        if err_msg is not None:
56+            msg = err_msg
57+        raise AssertionError(msg)

stickies-v commented at 10:41 am on August 1, 2022:

Less LoC and less code execution when err_msg is provided: (same for the other functions)

0        if err_msg is None:
1            err_msg = "not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)
2        raise AssertionError(err_msg)

vincenzopalazzo commented at 8:27 pm on August 1, 2022:

IMHO the only reason your solution is better than mine is that you avoid the copy of the string, otherwise is only a different way to a boolean condition :)

stickies-v commented at 9:32 pm on August 1, 2022:

Oh yeah I didn’t mean to make it sound like a big thing, just that in this order it’s a bit less code that executes a bit faster. Thanks for updating!

stickies-v commented at 9:40 pm on August 1, 2022:

Looks like you only updated it for assert_equal but it holds for all the other functions to which you added err_msg too.

vincenzopalazzo commented at 9:46 pm on August 1, 2022:

ops :) sorry too lazy! I will update the the others in a few hours :) Thanks for the catch

in test/functional/test_framework/util.py:50 in ad9dcabd5d outdated

46@@ -47,20 +47,44 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
47         raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
48 
49 
50-def assert_equal(thing1, thing2, *args):
51+def assert_equal(thing1, thing2, *args, err_msg=None):

stickies-v commented at 10:48 am on August 1, 2022:

In assert_raises_message (which doesn’t seem to be used anywhere) we already use the message name instead of err_msg. I don’t care for which one we use, but I think consistency would be preferable?

vincenzopalazzo commented at 8:30 pm on August 1, 2022:

I preferer to use err_msg because in the future you are free to add the message if you need it for other stuff, I like to leave the port open to extension.

What do you think?

stickies-v commented at 9:43 pm on August 1, 2022:

Sorry you’re right, I misunderstood what message was doing in assert_raises_message - had a closer look now. Please disregard.

in test/functional/test_framework/util.py:84 in ad9dcabd5d outdated

86+        raise AssertionError(msg)
87+
88+
89+def assert_true(thing, err_msg=None):
90+    if thing is not True:
91+        msg = "%s it is not True" % str(thing)

stickies-v commented at 10:51 am on August 1, 2022:

0        msg = "%s is not True" % str(thing)

in test/functional/feature_taproot.py:502 in ad9dcabd5d outdated

498@@ -495,11 +499,11 @@ def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=
499 
500     # Compute scriptPubKey and set useful defaults based on the inputs.
501     if witv0:
502-        assert tap is None
503+        assert_equal(tap, None)

stickies-v commented at 10:56 am on August 1, 2022:

edit: please disregard the below

err_msg defaults to None and we usually don’t specify it. Also, since some assert functions iterate over *args, I’d always use err_msg as a named argument, even when not required (as e.g. in assert_equal)

0        assert_equal(tap)

vincenzopalazzo commented at 8:31 pm on August 1, 2022:

I think not to being specific here causes confusion for people that are not full-time on bitcoin core and do not know how all the API work, if you are not strong in opposition I would like to keep this more explicit

stickies-v commented at 9:36 pm on August 1, 2022:

Sorry, I was too quick - I thought None was passed to err_msg but it’s just the equality operation of course. Please disregard.

in test/functional/feature_taproot.py:506 in ad9dcabd5d outdated

503+        assert_equal(tap, None)
504         conf["mode"] = "witv0"
505         if pkh is not None:
506             # P2WPKH
507-            assert script is None
508+            assert_equal(script, None)

stickies-v commented at 11:06 am on August 1, 2022:

== and is are not entirely equivalent in Python, with both differences in evaluation speed and outcome. Even though I think it’s fine to use the equality operator here, I think the proper way to do it would be to add assert_is and/or assert_is_none functions (e.g. unittest also has these: https://docs.python.org/3/library/unittest.html#assert-methods)

vincenzopalazzo commented at 8:34 pm on August 1, 2022:

I think I’m getting confused here :)

We need to converge on some code style here otherwise we cause only confusion #23127 (review)

cc @MarcoFalke

in test/functional/feature_backwards_compatibility.py:148 in ad9dcabd5d outdated

146         node_master.createwallet(wallet_name="w2", disable_private_keys=True)
147         wallet = node_master.get_wallet_rpc("w2")
148         info = wallet.getwalletinfo()
149-        assert info['private_keys_enabled'] == False
150-        assert info['keypoolsize'] == 0
151+        assert_equal(info['private_keys_enabled'], False)

stickies-v commented at 11:10 am on August 1, 2022:

Since you added assert_true, should we also have assert_false?

vincenzopalazzo commented at 8:34 pm on August 1, 2022:

Same #25732 (review)

vincenzopalazzo commented at 8:35 pm on August 1, 2022:

I would like use some helper function because IMHO this make the code more readble but this is up to up guys

stickies-v commented at 11:27 am on August 2, 2022:

On second thought, I think assert_equal here works well, since the previous code was indeed just comparing equality and not doing a boolean comparison. Resolved.

in test/functional/interface_zmq.py:75 in ad9dcabd5d outdated

71@@ -70,9 +72,9 @@ def receive_sequence(self):
72         label = chr(body[32])
73         mempool_sequence = None if len(body) != 32+1+8 else struct.unpack("<Q", body[32+1:])[0]
74         if mempool_sequence is not None:
75-            assert label == "A" or label == "R"
76+            assert_true(label == "A" or label == "R", err_msg="{} different from A or C".format(label))

stickies-v commented at 11:15 am on August 1, 2022:

Could optionally also implement assert_in (see https://docs.python.org/3/library/unittest.html#assert-methods)?

stickies-v commented at 11:16 am on August 1, 2022:

f-strings are easier to read imo:

0            assert_true(label == "A" or label == "R", err_msg=f"{label} different from A or C")

vincenzopalazzo commented at 8:40 pm on August 1, 2022:

ops, good catch, thanks!

stickies-v commented at 11:17 am on August 1, 2022: contributor

Approach ACK ad9dcabd5d9cef3f16accbc9dc1572cd82a99081

tests: Use test utils each time that it is possible


* feature_taproot: Introduce another assert called assert_true. 

* interface_zmq.py: Remove assert in favor of test utils functions

* feature_taproot: Fixed imports

* mining_getblocktemplate_longpoll.py: Replace assert with assert_equal

* p2p_addrfetch.py: Replace assert with assert_equal

* p2p_blockfilters.py: Replace assert with test utils function

* test framework: Adding possibility to specify messages in the assert functions

* feature_taproot.py: Use the err_msg introduced in the assert framework.

* feature_taproot.py: Use the err_msg introduced in the assert framework.

* feature_taproot.py: Use the err_msg introduced in the assert framework.

640edd9896

vincenzopalazzo commented at 8:41 pm on August 1, 2022: none

Thanks @stickies-v, you remember me that we have a pending decision on the API that we are using right now #23127 (review) maybe we can discuss it here!

vincenzopalazzo force-pushed on Aug 1, 2022

in test/functional/test_framework/util.py:81 in 640edd9896

83+            msg = err_msg
84+        raise AssertionError(msg)
85+
86+
87+def assert_true(thing, err_msg=None):
88+    if thing is not True:

stickies-v commented at 2:08 pm on August 2, 2022:

These functions are generally (e.g. https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertTrue) used to do a boolean comparison, which makes it functionally different than using assert_equal(expr, True)

It wouldn’t make a functional difference in the places where it’s currently used (where a simple assert would suffice), but otherwise, this is how I think it should be done:

0    if bool(thing) is not True:

vincenzopalazzo commented at 5:44 pm on August 2, 2022:

with this suggestion, you will create an error a runtime and no static checker can find it. We can use the only good thing of python language to write something like that

0def assert_true(thing: bool, err_msg=None):
1    if thing is not True:

In this way, the editor is able to catch the type mismatch! the cast of a type at runtime should be an antipattern in all modern language

stickies-v commented at 6:18 pm on August 2, 2022:

The only way bool(thing) throws an error is if thing is of a custom type that explicitly overrides __bool__() to return something other than True or False, which is… really bad code. I don’t think we need to handle that, and we can easily observe if it happens from the stacktrace.

Even though I’m a fan of type hints, I don’t think they change anything here - afaik we don’t use tooling like mypy that would enforce them? It also wouldn’t do the boolean comparison which was kinda my point: you don’t want assert_true() to just verify if something is equal to True/False (assert_equal can do that), it’s helpful if you need to check if the boolean of an object would evaluate to True/False. For example:

0result = some_func()  # some_func returns 0 if it failed, an int between 1-5 if successful
1assert_equal(result, True) # this would not work, because e.g. `3 == True` would fail
2assert_equal(bool(result), True)  # this would work, but you can't inspect `result` when the assertion fails
3assert_true(result)  # this would work if `assert_true` evaluates bool(result), because `bool(3) is True` would pass
4
5if(result):
6    do_something_else()

stickies-v commented at 6:23 pm on August 2, 2022:

But to reiterate, I’m advocating against implementing assert_true() at this moment, so I’ll leave it here

vincenzopalazzo commented at 6:37 pm on August 2, 2022:

You are right! The thing is worst than the exception because happens something like that

0➜  ~ python3
1Python 3.8.10 (default, Jun 22 2022, 20:18:18) 
2[GCC 9.4.0] on linux
3Type "help", "copyright", "credits" or "license" for more information.
4>>> bool(12)
5True
6>>> bool("Alibaba")
7True
8>>>

This can not hide bugs?

vincenzopalazzo commented at 6:43 pm on August 2, 2022:

I think to make all happy here is to use the assert_equal everywhere it is possible and move on.

This is code that I wrote one year ago, so I do not remember why I insert assert_true, but if I was not drunk I think because somewhere there is some comparison with custom error messages that check a boolean condition.

Anyway, I think if you agree we can move all on assert_equal and move on.

you don’t want assert_true() to just verify if something is equal to True/False

To sustain your thesis, I’m not able to see any real use case other than toy example of how this can be used

stickies-v commented at 6:53 pm on August 2, 2022:

How is that worse? You just need to use the right tool for the job. If you need to compare equality, you use assert_equal. If you need to do a boolean comparison, you use assert_true. If you don’t know your input type, you should probably validate/convert that first. If you don’t need to do a boolean comparison (i.e. if you wouldn’t want “Alibaba” to evaluate to True), then you just don’t use the boolean assertion?

The point of these functions is to check how something would be evaluated to a boolean, as in the example I gave where the program flow is based on if(result). We don’t seem to have that need in our test suite atm, so hence we shouldn’t have assert_true.

vincenzopalazzo commented at 7:21 pm on August 2, 2022:

Mh! I do not argue this on the PR because it is the wrong place but I disagree with this statement

How is that worse? You just need to use the right tool for the job.

Of course, bug is a bug, and it is there because there is a mistake in the code, so if a comparison between bool, I want to compare boolean value and not a string that always evaluates to true except for empty.

Is worse because if we were able to catch this bug easily we were still coding in C without struggling with rust compiler, if you do this case you should define some rules like: bool("False") -> False or True?

Imagine to have assert_true(json['foo']) and make an API change that move the type of a filed from string to int

you fall in case of

 0Python 3.8.10 (default, Jun 22 2022, 20:18:18) 
 1[GCC 9.4.0] on linux
 2Type "help", "copyright", "credits" or "license" for more information.
 3>>> bool(0)
 4False
 5>>> bool("0")
 6True
 7>>> bool(0.0)
 8False
 9>>> bool("0.0")
10True

It is not the right tool anyway at least without a good reason to allow this, and I do not see any till now

vincenzopalazzo commented at 7:29 pm on August 2, 2022:

A better example can be

take the from our assert_true(hash_str is None) with assert_true(hash_str)

0➜  ~ python3
1Python 3.8.10 (default, Jun 22 2022, 20:18:18) 
2[GCC 9.4.0] on linux
3Type "help", "copyright", "credits" or "license" for more information.
4>>> bool(None)
5False
6>>> bool("")
7False

This can be a bug not catch by the CI, and if it is an API change you can break the client that use a strongly typed language

stickies-v commented at 2:14 pm on August 2, 2022: contributor

I don’t want to bikeshed over this too much, it’s not a big deal. But my view (and it has changed a bit after reading the previous issues/PRs) is that:

we check values for being (not) None quite often. Even though == comparison would work fine in probably all cases, it’s not the Python way of doing things and given how often this is used I feel adding a assert_is_none() and assert_is_not_none() function is warranted. Since generally nothing except for None should be compared through is, I’d say we can leave out assert_is() and assert_is_none() (which I suggested earlier) until we actually need it (probably never).
even though boolean assert_true() and assert_false() functions can have their merit, upon re-reading I don’t see any in the current PR because we currently only ever pass it booleans. In all the cases where assert_true() is used, a simple assert <expression>, <err_msg> is functionally equivalent, shorter, and without the need to define assert_true(). In all places where assert_true() is used, the logs would just show False is not True, which does not help. I believe this is what @MarcoFalke was referring to, and I agree with him for this scenario (but not for the assert_equal() on which the comment thread was based). Having a boolean assertion function is useful as soon as you need to pass non-boolean types to it. If we do decide to implement an assert_true(expr, err_msg) function, it should be implemented to evaluate bool(expr) is not True instead of just expr is not True (as commented), in which case it offers benefit over assert_equal(bool(expr), True) because we can now log the value of expr instead of bool(expr). But again, it looks like we currently don’t have a need for this.
From my reading of previous issues, it seems like there is no reason to necessarily phase out all usage of plain assert statements here - the only reason to add bespoke assert_...() functions was to make it easier to include error messages. As such, I think it’s fine to keep plain assert statements where it’s easier until we have a clear reason (please tell me if I’ve missed that) to do so.

vincenzopalazzo commented at 5:51 pm on August 2, 2022: none

In this PR I have no strong opinion in whatever decision we take, and I just do not care too much about what type of assert we will use, as I see this will not bring too much value inside the code base.

I’m fine with assert_is_none(x), but it is possible to have the same result with assert_true(x is None, err_msg=f"wrong value: {x}") or assert_equal(x, None, err_msg=f"wrong value {x}")

more function we add, more code to maintain we have, and if there is no strong agreement I will preferer to remove all the asser_* functions and keep just assert_equal because if we add other functions here we also need to migrate the old code, and migrate test unit IMHO it is never a good idea

stickies-v commented at 6:39 pm on August 2, 2022: contributor

Yeah okay let’s do the common denominator approach now and potentially add more functions later on, I don’t want to push on the is thing too much. So I think in short, remove assert_true() in favour of generic assert statements, and use assert_equal for the other uses? I’ll re-review once that’s updated.

NorrinRadd commented at 12:23 pm on October 3, 2022: none

Dup from IRC: this PR has been quiet for 2 months. Is it ok if someone else completes the last recommendations?

Secondly, the latest ask is essentially the opposite of the the ask in the issue this PR is meant to address. Can we get some confirmation on what the approach should be? cc: @theStack

vincenzopalazzo commented at 12:44 pm on October 3, 2022: none

Dup from IRC: this PR has been quiet for 2 months. Is it ok if someone else completes the last recommendations?

Feel free to cherry-pick my commits and continue in a new PR it is totally fine, and also it useful that someone else will continue an unfinished work

fanquake commented at 12:54 pm on October 3, 2022: member

Feel free to cherry-pick my commits and continue in a new PR it is totally fine, @vincenzopalazzo does that mean you’re no-longer working on / maintaining this PR? If so, would you like to close it?

vincenzopalazzo commented at 1:15 pm on October 3, 2022: none

@fanquake Yes I think that there are people that can give a better love to this PR

vincenzopalazzo closed this on Oct 3, 2022

bitcoin locked this on Oct 3, 2023

tests: Use test utils each time that it is possible #25732

Conflicts