Test Framework: Stricter specification of `assert_equal` parameters #22583

Currently assert_equal is defined as follows:

def assert_equal(thing1, thing2, *args):
    if thing1 != thing2 or any(thing1 != arg for arg in args):
        raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))

[source]

It works correctly. But one consequence of this definition is that people can write:

• assert_equal(computation_result, 1) or
• assert_equal(1, computation_result)

which I find undesirable because sometimes it may not be that clear what is expectation and what is the actual result. For new contributors it may be an issue too as they typically don't know Bitcoin Core codebase that well.

For these reasons, it would make sense to me to change the method signature as follows:

def assert_equal(thing1, thing2, *args):

->

def assert_equal(expected, actual, *args):  # (1)

Unfortunately, it seems that people use it more in the following way:

def assert_equal(actual, expected, *args): # (2)

which seems slightly worse in the sense that one checks then equality of the second parameter with the first one, the third one, the fourth one, etc.

What you do think? Would it make sense to change the original assert_equal method signature with either (1) or (2) one?

Edit: If there is an agreement that this is a good idea, I'm willing to create a PR for this.

I think this is a good idea. It would require some non-trivial refactoring, but would certainly make tests more readable. My only concern is that keeping this consistent long term might be a hassle, and that might not be worth the effort.

My only concern is that keeping this consistent long term might be a hassle, and that might not be worth the effort.

Some IDEs these days show method parameters as hints (Visual Studio, Visual Studio Code, PyCharm) and you can see the parameter names (expected / actual) immediately and then there is no hassle because you can see what parameter you are passing in. For people with ordinary text editors, it may be non-obvious but then again everybody writes tests somehow today and my guess is that they follow some convention already. And even if everything fails and somebody will write assert_equal "wrong", it's not an end of the world (for me :-))

Thanks for the comment!

Edit: The reason I filed this issue is that I got burned by this once and it took me more time to understand something. So there is some real world motivation.

@MarcoFalke @laanwj 👍 / 👎 / ¯_(ツ)_/¯ ?

¯(ツ)/¯

FWIW, this subject has also been discussed shortly in a PR review club meeting from July 2019: https://bitcoincore.reviews/15443#l-107 (with no solution or call for action though, just the perception that the arg order in our tests are mostly "actual, expected")

I tend to agree with tylerchamber's statement:

I think this is a good idea. It would require some non-trivial refactoring, but would certainly make tests more readable. My only concern is that keeping this consistent long term might be a hassle, and that might not be worth the effort.

Thanks for the info.

I still think it would be nice to rename assert_equal parameters but given that people are not excited about this I don't think it makes sense to pursue it any longer.