On 11/26/2014 06:20 AM, Nicolas Trangez wrote:
On Mon, 2014-11-24 at 13:19 -0500, Jay Pipes wrote:
I think pointing out that the default failure
message for testtools.TestCase.assertEqual() uses the terms
"reference"
(expected) and "actual" is a reason why reviewers *should* ask patch
submitters to use (expected, actual) ordering.
Is there any reason for this specific ordering? Not sure about others,
but I tend to write equality comparisons like this
if var == 1:
instead of
if 1 == var:
(although I've seen the latter in C code before).
This gives rise to
assert var == 1
or, moving into `unittest` domain
assertEqual(var, 1)
reading it as 'Assert `var` equals 1', which makes me wonder why the
`assertEqual` API is defined the other way around (unlike how I'd write
any other equality check).
It's not about an equality condition.
It's about the message that is produced by
testtools.TestCase.assertEqual(), and the helpfulness of that message
when the order of the arguments is reversed.
This is especially true with large dict comparisons. If you get a
message like:
reference: <large_dict>
actual: <large_dict>
And the arguments are reversed, then you end up wasting time looking in
the test code instead of the real code for the thing that is different.
Anyway, like I said, it's not something that we can write a simple
hacking check for, and therefore, it's not something that should have
much time spent on. But I do recommend that reviewers bring it up,
especially if the patch author has been inconsistent in their usage of
(expected, actual) in multiple assertEqual() calls in their patch.
Best,
-jay
_______________________________________________
OpenStack-dev mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
