[openstack-dev] [nova] Proposal new hacking rules
jaypipes at gmail.com
Wed Nov 26 13:54:35 UTC 2014
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
>> (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
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.
More information about the OpenStack-dev